New Feature: Constant Interpolated Strings (Initial PR) by kevinsun-dev · Pull Request #45396 · dotnet/roslyn

kevinsun-dev · 2020-06-23T19:43:51Z

This feature is about enabling string constants to be made out of interpolated strings, where the interpolated string itself is made up of constants.

public class C
{
    const string S1 = $"Hello world";
    const string S2 = $"Hello{" "}World";
    const string S3 = $"{S1} Kevin, welcome to the team!";
}

Added in this PR:

Support simple constant interpolated strings
Updated tests

This is going to break so many things...

gafter · 2020-06-26T17:07:36Z

I would think the simplest way of constructing the resulting string, including handling of escaping, would be to use string.Format on the format string and all of the values of the interpolations. I think that could be done after processing everything rather than at the same time, separating the constant folding implementation from the rest of the semantic analysis. #Closed

jaredpar · 2020-06-29T21:26:00Z

src/Compilers/CSharp/Portable/Binder/Binder_InterpolatedString.cs

            var objectType = GetSpecialType(SpecialType.System_Object, diagnostics, node);
            var intType = GetSpecialType(SpecialType.System_Int32, diagnostics, node);
+
+            // PROTOTYPE follow up with Neal on the implementation 


Suggested change

// PROTOTYPE follow up with Neal on the implementation

// PROTOTYPE(CONSTISTR) follow up with Neal on the implementation

``` #Closed

This has been done, you can remove the comment :)

In reply to: 447263586 [](ancestors = 447263586)

Indeed, the comment has been removed.

In reply to: 447335348 [](ancestors = 447335348,447263586)

jaredpar · 2020-06-29T21:27:31Z

src/Compilers/CSharp/Portable/Binder/Binder_InterpolatedString.cs

                                alignment = GenerateConversionForAssignment(intType, BindValue(interpolation.AlignmentClause.Value, diagnostics, Binder.BindValueKind.RValue), diagnostics);
                                var alignmentConstant = alignment.ConstantValue;
+                                constantEnabled = false;
                                if (alignmentConstant != null && !alignmentConstant.IsBad)


One potential benefit of going the string.Format route is that it could allow us to consider supporting alignment as that would come for free. We should keep that in mind when discussing that aspect of the implementation. #Closed

Yup, talked it over with Fred. The alignment support for strings that can only be composed of constants may be outweighed by this implementation's default support for ropes.

In reply to: 447264298 [](ancestors = 447264298)

jaredpar · 2020-06-29T21:34:32Z

src/Compilers/CSharp/Portable/Binder/Binder_InterpolatedString.cs

+            ConstantValue? resultConstant = null;
+            bool constantEnabled = true;


Think the code would read better if the names of these paired values matched better. For instance resultConstant and isResultConstant #Closed

Thanks for the suggestion, changes made.

In reply to: 447267474 [](ancestors = 447267474)

jaredpar · 2020-06-29T21:37:01Z

src/Compilers/CSharp/Portable/Binder/Binder_InterpolatedString.cs

                            builder.Add(new BoundStringInsert(interpolation, value, alignment, format, null));
+                            if (value.ConstantValue != null)
+                            {
+                                if (!value.ConstantValue.IsString || value.ConstantValue.IsBad || value.ConstantValue.IsNull)


Right now the logic to determine if this value is constant is spread out to four places: the format check, the alignment check, the constant check and the type of constant. Consider just combining it into a single check here:

if (value.ConstantValue?.IsString == true && !value.ConstantValue.IsBad && !value.ConstantValue.IsNull && interpolation.FormatClaues is null && interpolation.AlignmentClause is null) { ... }

#Closed

The first 3 can be collapsed to a single pattern: value.ConstantValue is { IsString: true, IsBad: false, IsNull: false }

In reply to: 447268625 [](ancestors = 447268625)

And the last 2 as well: interpolation is { FormatClauses : null, AlignmentClause: null }

In reply to: 447343672 [](ancestors = 447343672,447268625)

We can also bail out early if !constantEnabled.

In reply to: 447343824 [](ancestors = 447343824,447343672,447268625)

Thanks for all the suggestions, I've incorporated them into commit 16.

In reply to: 447347863 [](ancestors = 447347863,447343824,447343672,447268625)

jaredpar · 2020-06-29T21:38:01Z

src/Compilers/CSharp/Portable/Binder/Binder_InterpolatedString.cs

                            builder.Add(new BoundStringInsert(interpolation, value, alignment, format, null));
+                            if (value.ConstantValue != null)
+                            {
+                                if (!value.ConstantValue.IsString || value.ConstantValue.IsBad || value.ConstantValue.IsNull)


Why is null special cased here as a non-constant? That is a legal string constant value. Please add a PROTOTYPE comment to follow up on that. #Closed

If we're talking about value.ConstantValue, that was null checked to prevent a whole host of NullPointerExceptions. If we're taking about the extra case in the if statement beneath it, I'd honestly forgotten if it was merely out of an abundance of caution or not. Removed it in commit 16, lets see if it works.

In reply to: 447269087 [](ancestors = 447269087)

333fred · 2020-06-30T00:55:54Z

src/Compilers/CSharp/Portable/BoundTree/BoundExpression.cs


+    internal partial class BoundInterpolatedString
+    {
+        public override ConstantValue? ConstantValue


What actually uses this property? #Closed

Ah, nevermind.

In reply to: 447344161 [](ancestors = 447344161)

333fred · 2020-06-30T00:57:16Z

src/Compilers/CSharp/Test/Symbol/Compilation/SemanticModelAPITests.cs


        [WorkItem(976, "https://github.com/dotnet/roslyn/issues/976")]
-        [Fact]
+        [Fact(Skip = "PROTOTYPE(CONSTISTR)")]


What is failing about this test? Consider recording that in the skip comment. #Closed

Recorded the reason in the skip comment, is this the correct format?

In reply to: 447344626 [](ancestors = 447344626)

That's fine, thanks.

In reply to: 447771212 [](ancestors = 447771212,447344626)

333fred · 2020-06-30T00:57:24Z

src/EditorFeatures/CSharpTest/CodeActions/IntroduceVariable/IntroduceVariableTests.cs


        [WorkItem(1065661, "http://vstfdevdiv:8080/DevDiv2/DevDiv/_workitems/edit/1065661")]
-        [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsIntroduceVariable)]
+        [Fact(Skip = "PROTOTYPE(CONSTISTR)"), Trait(Traits.Feature, Traits.Features.CodeActionsIntroduceVariable)]


Same here. #Closed

333fred · 2020-06-30T00:58:35Z

src/Compilers/CSharp/Test/Semantic/Semantics/ConstantTests.cs

        }
+
+        [Fact]
+        public void ConstantInterpolatedStringsSimple()


Please add a prototype comment for testing invalid scenarios as well. #Closed

Added tests for invalid scenarios.

In reply to: 447344980 [](ancestors = 447344980)

333fred · 2020-06-30T01:08:33Z

Done review pass (commit 15) #Closed

333fred · 2020-06-30T20:29:56Z

src/Compilers/CSharp/Test/Semantic/Semantics/ConstantTests.cs

+        }
+
+        [Fact]
+        public void ConstantInterpolatedStringsError()


We'll need more tests, but it's a good start. The rest can wait for test plan review.

Think there are some we can add in advance of the test plan review: used in attributes, default parameter values, used across assembly boundaries, used as fields, etc ... Also the always popular: lang ver checks.

Those can be done in a separate PR but should get them done before a test plan review

333fred

LGTM (commit 17)

jaredpar · 2020-06-30T20:40:40Z

src/Compilers/CSharp/Test/Semantic/Semantics/ConstantTests.cs

+        }
+
+        [Fact]
+        public void ConstantInterpolatedStringsError()


Think there are some we can add in advance of the test plan review: used in attributes, default parameter values, used across assembly boundaries, used as fields, etc ... Also the always popular: lang ver checks.

Those can be done in a separate PR but should get them done before a test plan review

Kevin Sun added 11 commits June 23, 2020 12:41

Working Constant Interpolated Strings ($"simple")

ee83c0d

This is going to break so many things...

Ensure nullable

cbb66cb

Testing

396cfa4

Testing Break

0a012b5

Hidden Constants

f0e7611

Please?

719cc66

Added Null Check

d293ef8

Matching Format Specifications, added boolean specifier

c0386e7

Added Skips and Changes

5861313

Update SemanticModelAPITests.vb

67a27a7

Update RecordParsing.cs

336a44c

Kevin Sun added 2 commits June 26, 2020 10:55

Escaped Constants

b1870a3

Updated Tests

4030f80

kevinsun-dev changed the title ~~Working Constant Interpolated Strings ($"simple")~~ New Feature: Constant Interpolated Strings (Initial PR) Jun 29, 2020

kevinsun-dev added Area-Compilers Language-C# labels Jun 29, 2020

kevinsun-dev marked this pull request as ready for review June 29, 2020 15:27

kevinsun-dev requested review from a team as code owners June 29, 2020 15:27

kevinsun-dev self-assigned this Jun 29, 2020

Small changes

c973647

333fred added the Feature - Const Interpolated Strings label Jun 29, 2020

Uncommented Tests

7defc47

jaredpar reviewed Jun 29, 2020

View reviewed changes

333fred reviewed Jun 30, 2020

View reviewed changes

Kevin Sun added 2 commits June 30, 2020 08:09

Requested PR changes

033b4ef

Add Tests for Invalid

fb4f686

kevinsun-dev requested a review from 333fred June 30, 2020 19:57

333fred reviewed Jun 30, 2020

View reviewed changes

333fred approved these changes Jun 30, 2020

View reviewed changes

jaredpar approved these changes Jun 30, 2020

View reviewed changes

kevinsun-dev merged commit d381843 into dotnet:features/interpolated-string-constants Jun 30, 2020

	// PROTOTYPE follow up with Neal on the implementation
	// PROTOTYPE(CONSTISTR) follow up with Neal on the implementation
	``` #Closed

		ConstantValue? resultConstant = null;
		bool constantEnabled = true;

Conversation

kevinsun-dev commented Jun 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gafter commented Jun 26, 2020 • edited by 333fred Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jaredpar Jun 29, 2020 • edited by 333fred Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredpar Jun 29, 2020 • edited by 333fred Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredpar Jun 29, 2020 • edited by 333fred Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredpar Jun 29, 2020 • edited by 333fred Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

333fred Jun 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaredpar Jun 29, 2020 • edited by 333fred Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

333fred Jun 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

333fred Jun 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

333fred Jun 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

333fred Jun 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

333fred commented Jun 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

333fred left a comment

Choose a reason for hiding this comment

kevinsun-dev commented Jun 23, 2020 •

edited

Loading

gafter commented Jun 26, 2020 •

edited by 333fred

Loading

jaredpar Jun 29, 2020 •

edited by 333fred

Loading

jaredpar Jun 29, 2020 •

edited by 333fred

Loading

jaredpar Jun 29, 2020 •

edited by 333fred

Loading

jaredpar Jun 29, 2020 •

edited by 333fred

Loading

333fred Jun 30, 2020 •

edited

Loading

jaredpar Jun 29, 2020 •

edited by 333fred

Loading

333fred Jun 30, 2020 •

edited

Loading

333fred Jun 30, 2020 •

edited

Loading

333fred Jun 30, 2020 •

edited

Loading

333fred Jun 30, 2020 •

edited

Loading

333fred commented Jun 30, 2020 •

edited

Loading