Add support for interpolated string handler conversions #53602

333fred · 2021-05-21T21:41:58Z

This PR allows interpolated strings to convert to types marked with InterpolatedStringBuilderAttribute, allows the RefKind to mismatch, and implements support for the better conversion rules.

Test plan: #51499

This PR allows interpolated strings to convert to types marked with InterpolatedStringBuilderAttribute, allows the RefKind to mismatch, and implements support for the better conversion rules.

333fred · 2021-05-24T16:54:00Z

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

+                objectType = GetSpecialType(SpecialType.System_Object, diagnostics, unconvertedInterpolatedString.Syntax);
+                conversionDiagnostics = BindingDiagnosticBag.GetInstance(withDiagnostics: true, withDependencies: false);
+            }
+            else if (isHandlerConversion)


Most of this function is unchanged, just extracted to be called from multiple places. The new bit is the addition of isHandlerConversion as a parameter, this feature check, and the addition of a prototype comment around dynamic below.

333fred · 2021-05-24T16:55:25Z

src/Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/OverloadResolution.cs

@@ -2105,19 +2105,19 @@ private static ParameterSymbol GetParameter(int argIndex, MemberAnalysisResult r
                return (m1ModifierCount < m2ModifierCount) ? BetterResult.Left : BetterResult.Right;
            }

-            // Otherwise, prefer methods with 'val' parameters over 'in' parameters.
-            return PreferValOverInParameters(arguments, m1, m1LeastOverriddenParameters, m2, m2LeastOverriddenParameters);
+            // Otherwise, prefer methods with 'val' parameters over 'in' parameters and over 'ref' parameters when the argument is an interpolated string handler.


This needs to be documented in the spec, but I'd propose we do so. #Pending

//PROTOTYPE to track?

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

333fred · 2021-05-25T17:55:36Z

@chsienki @AlekseyTs there's a couple of globalization issues I have to fix up in the tests around windows vs unix/linux and PEVerify, but this is otherwise ready for review. Please take a look.

chsienki · 2021-05-26T03:24:05Z

@333fred Apologies I got sidetracked by a bunch of different stuff today, but I'll take a look at this first thing tomorrow.

chsienki · 2021-05-26T18:30:40Z

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

+                    syntax,
+                    BindUnconvertedInterpolatedStringToHandlerType(unconvertedSource, (NamedTypeSymbol)destination, diagnostics, isHandlerConversion: true),
+                    conversion,
+                    @checked: false,


Does this conversion cover the whole expression? If I have arithmetic in the interpolation holes for example? #Pending

Any expression is valid in the interpolation holes, we don't treat anything special.

Oh, I see what you're saying. It doesn't apply to anything inside interpolations, but we should probably use the value of CheckOverflowAtRuntime anyway.

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_Call.cs

src/Compilers/CSharp/Portable/Symbols/NamedTypeSymbol.cs

chsienki · 2021-05-26T20:19:24Z

Done with review pass (commit 7)

333fred · 2021-05-26T20:38:46Z

@chsienki did you have any more comments? I don't think there's anything I can't address in the next PR.

In reply to: 849090010

333fred · 2021-05-26T20:58:25Z

@AlekseyTs for second review.

AlekseyTs · 2021-05-28T17:19:04Z

src/Compilers/Core/Portable/MetadataReader/PEModule.cs

@@ -1002,6 +1002,11 @@ internal bool HasCodeAnalysisEmbeddedAttribute(EntityHandle token)
            return FindTargetAttribute(token, AttributeDescription.CodeAnalysisEmbeddedAttribute).HasValue;
        }

+        internal bool HasInterpolatedStringBuilderAttribute(EntityHandle token)


HasInterpolatedStringBuilderAttribute

HasInterpolatedStringHandlerAttribute? #Closed

AlekseyTs · 2021-05-28T17:22:54Z

src/Compilers/Core/Portable/Symbols/Attributes/CommonTypeEarlyWellKnownAttributeData.cs

+
+        #region InterpolatedStringHandlerAttribute
+        private bool _hasInterpolatedStringHandlerAttribute;
+        public bool HasInterpolatedStringHandlerAttribute


HasInterpolatedStringHandlerAttribute

What is the reason for putting this into EarlyWellKnownAttributeData? #Closed

This is covered in the actual decode code, and has a corresponding test to make sure we bind to the correct attribute constructor.

AlekseyTs · 2021-05-28T17:23:36Z

src/Compilers/Core/Portable/Symbols/Attributes/CommonTypeEarlyWellKnownAttributeData.cs

+
+        #region InterpolatedStringHandlerAttribute
+        private bool _hasInterpolatedStringHandlerAttribute;
+        public bool HasInterpolatedStringHandlerAttribute


HasInterpolatedStringHandlerAttribute

Should this be moved to C# specific type? #Closed

I'll have to add one as we don't have one for C# currently, but that's doable.

AlekseyTs · 2021-05-28T17:39:13Z

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

+                    BindUnconvertedInterpolatedStringToHandlerType(unconvertedSource, (NamedTypeSymbol)destination, diagnostics, isHandlerConversion: true),
+                    conversion,
+                    @checked: false,
+                    explicitCastInCode: false,


explicitCastInCode: false

This looks suspicious. #Closed

Chris mentioned this earlier. I'm planning on addressing in the next PR, unless you think this needs to be addressed now.

Oh, sorry, that was on a different line. Yes, this is incorrect and I'll add a test.

Chris mentioned this earlier.

This is not about checked/unchecked, which has no impact on this conversion. This is about explicitCastInCode, which is likely to be significant.

AlekseyTs · 2021-05-28T17:51:42Z

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

-                }
+        private BoundInterpolatedString BindUnconvertedInterpolatedStringToHandlerType(BoundUnconvertedInterpolatedString unconvertedInterpolatedString, NamedTypeSymbol interpolatedStringHandlerType, BindingDiagnosticBag diagnostics, bool isHandlerConversion)
+        {
+            diagnostics.Add(interpolatedStringHandlerType.GetUseSiteInfo(), unconvertedInterpolatedString.Syntax.Location);


Add

I think we have a ReportUseSite helper that takes a symbol and a syntax node #Closed

AlekseyTs · 2021-05-28T17:56:16Z

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

+                objectType = GetSpecialType(SpecialType.System_Object, diagnostics, unconvertedInterpolatedString.Syntax);
+                conversionDiagnostics = BindingDiagnosticBag.GetInstance(withDiagnostics: true, withDependencies: false);
+            }
+            else if (isHandlerConversion)


else if (isHandlerConversion)

Consider making this a separate if rather than an else if. This will avoid dependency on value of needToCheckConversionToObject and any future changes for it. needToCheckConversionToObject depends on isHandlerConversion, not the other way around. #Closed

AlekseyTs · 2021-05-28T18:17:09Z

src/Compilers/CSharp/Portable/Errors/ErrorFacts.cs

@@ -510,6 +510,7 @@ internal static bool PreventsSuccessfulDelegateConversion(ErrorCode code)
                case ErrorCode.ERR_QueryRangeVariableSameAsTypeParam:
                case ErrorCode.ERR_DeprecatedCollectionInitAddStr:
                case ErrorCode.ERR_DeprecatedSymbolStr:
+                case ErrorCode.ERR_FeatureInPreview: // LangVersion should never change how we bind things


case ErrorCode.ERR_FeatureInPreview:

Isn't there a chance to change existing behavior if we modify this list? The motivation feels reasonable though. Another thing to consider is what happens when the particular feature is moved to its final language version. Also, I believe there are many other language specific errors, including the custom ones. And we will be adding them in the future. Does this mean this function requires special maintanense long term? #Closed

Does this mean this function requires special maintanense long term?

Yes, I think it does. In particular, the way this manifests is that, when you use an interpolated string conversion, overload resolution can potentially pick a different overload that doesn't use the conversion (even if that conversion wasn't used in the return type, just somewhere in the body). /cc @cston.

Perhaps the thing to do is make this change in dev16.11 (and include the other FeatureIn error codes as well) and do a smoke test on VS to see whether it's a big breaking change?

Does this mean this function requires special maintanense long term?

Yes, I think it does.

I think it will be hard to keep track. We should consider the following:

Adjusting the code so that it can intersept all current and future language version errors. I think they are already specially recognizable, since the required language version is supplied in a special way fo IDE to recognize.

Adding a dedicated method in this class that returns true for a language version error and using that helper here. Adding relevant comments and asserts in some strategic places to guide devs into the direction of this new helper.

I filed #53761 to follow up on this. I'll revert the change in this PR for now, as we discussed offline, and I've added a specific test to cover the difference in behavior.

AlekseyTs · 2021-05-28T20:41:22Z

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PENamedTypeSymbol.cs

@@ -141,6 +141,7 @@ private class UncommonProperties
            internal string lazyDefaultMemberName;
            internal NamedTypeSymbol lazyComImportCoClassType = ErrorTypeSymbol.UnknownResultType;
            internal ThreeState lazyHasEmbeddedAttribute = ThreeState.Unknown;
+            internal ThreeState lazyHasInterpolatedStringBuilderAttribute = ThreeState.Unknown;


lazyHasInterpolatedStringBuilderAttribute

Builder vs.Handler #Closed

AlekseyTs · 2021-05-28T21:02:49Z

    /// Patch refKinds for arguments that match 'In' parameters to have effective RefKind.

Should the doc comment be adjusted?

In reply to: 850668203 #Closed

Refers to: src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_Call.cs:506 in ff3c946. [](commit_id = ff3c946, deletion_comment = False)

AlekseyTs · 2021-05-28T21:14:26Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter.cs

+
+                case BoundKind.Conversion:
+                    var conversion = ((BoundConversion)expr);
+                    return expr is BoundConversion { Conversion: { IsInterpolatedStringHandler: true }, Type: { IsValueType: true } };


return expr is BoundConversion { Conversion: { IsInterpolatedStringHandler: true }, Type: { IsValueType: true } };

Is the actual local introduced by ILGen layer or elswhere in the LocalRewriter? #Closed

I mean the local that is passed by ref at the end. I just realized that it is probably the builder local that we are always creating. So we are just pretending that there is a ref modifier for the corresponding argument. Correct?

So we are just pretending that there is a ref modifier for the corresponding argument

That is correct.

AlekseyTs · 2021-05-28T21:20:29Z

Done with review pass (commit 7)

In reply to: 850675628

AlekseyTs

LGTM (commit 8)

dotnet-issue-labeler bot added the Area-Compilers label May 21, 2021

333fred changed the title ~~interpolated conversion~~ Add support for interpolated string handler conversions May 21, 2021

Add support for interpolated string handler conversions

8f52216

This PR allows interpolated strings to convert to types marked with InterpolatedStringBuilderAttribute, allows the RefKind to mismatch, and implements support for the better conversion rules.

333fred force-pushed the interpolated-conversion branch from 35a62af to 8f52216 Compare May 24, 2021 16:51

333fred commented May 24, 2021

View reviewed changes

333fred added 3 commits May 24, 2021 10:14

Add missing NamedTypeSymbol overrides.

3c5ec21

Fix tests.

cca5823

Fixup tests.

e7cc163

333fred marked this pull request as ready for review May 25, 2021 17:54

333fred requested a review from a team as a code owner May 25, 2021 17:54

333fred requested review from chsienki and AlekseyTs May 25, 2021 17:55

333fred added 3 commits May 25, 2021 11:14

Globalization, formatting, and PEVerify fixes.

c669026

Another attempt to get the verification working.

ba04c4c

Use invariant culture, correct missing theory variations.

ff3c946

chsienki reviewed May 26, 2021

View reviewed changes

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_Call.cs Show resolved Hide resolved

chsienki reviewed May 26, 2021

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/NamedTypeSymbol.cs Show resolved Hide resolved

chsienki approved these changes May 26, 2021

View reviewed changes

AlekseyTs reviewed May 28, 2021

View reviewed changes

PR Feedback.

41945e5

AlekseyTs approved these changes Jun 1, 2021

View reviewed changes

Add missing header and skip test in IOperation

5517e97

333fred enabled auto-merge (squash) June 1, 2021 17:48

333fred merged commit 66267da into dotnet:features/interpolated-string Jun 1, 2021

333fred deleted the interpolated-conversion branch June 1, 2021 18:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for interpolated string handler conversions #53602

Add support for interpolated string handler conversions #53602

333fred commented May 21, 2021 •

edited by jcouv

Loading

333fred May 24, 2021

333fred May 24, 2021 •

edited

Loading

chsienki May 26, 2021

333fred commented May 25, 2021

chsienki commented May 26, 2021

chsienki May 26, 2021 •

edited by 333fred

Loading

333fred May 26, 2021

333fred May 26, 2021

chsienki commented May 26, 2021

333fred commented May 26, 2021

333fred commented May 26, 2021

AlekseyTs May 28, 2021 •

edited

Loading

AlekseyTs May 28, 2021 •

edited

Loading

333fred May 28, 2021

AlekseyTs May 28, 2021 •

edited

Loading

333fred May 28, 2021

AlekseyTs May 28, 2021 •

edited

Loading

333fred May 28, 2021

333fred May 28, 2021

AlekseyTs May 28, 2021

AlekseyTs May 28, 2021 •

edited

Loading

AlekseyTs May 28, 2021 •

edited

Loading

AlekseyTs May 28, 2021 •

edited

Loading

333fred May 28, 2021

AlekseyTs May 28, 2021

333fred May 28, 2021

AlekseyTs May 28, 2021 •

edited

Loading

AlekseyTs commented May 28, 2021 •

edited

Loading

AlekseyTs May 28, 2021 •

edited

Loading

AlekseyTs May 28, 2021

333fred May 28, 2021

AlekseyTs commented May 28, 2021 •

edited

Loading

AlekseyTs left a comment

Add support for interpolated string handler conversions #53602

Add support for interpolated string handler conversions #53602

Conversation

333fred commented May 21, 2021 • edited by jcouv Loading

Choose a reason for hiding this comment

333fred May 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

333fred commented May 25, 2021

chsienki commented May 26, 2021

chsienki May 26, 2021 • edited by 333fred Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chsienki commented May 26, 2021

333fred commented May 26, 2021

333fred commented May 26, 2021

AlekseyTs May 28, 2021 • edited Loading

Choose a reason for hiding this comment

AlekseyTs May 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs May 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs May 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs May 28, 2021 • edited Loading

Choose a reason for hiding this comment

AlekseyTs May 28, 2021 • edited Loading

Choose a reason for hiding this comment

AlekseyTs May 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs May 28, 2021 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented May 28, 2021 • edited Loading

AlekseyTs May 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented May 28, 2021 • edited Loading

AlekseyTs left a comment

Choose a reason for hiding this comment

333fred commented May 21, 2021 •

edited by jcouv

Loading

333fred May 24, 2021 •

edited

Loading

chsienki May 26, 2021 •

edited by 333fred

Loading

AlekseyTs May 28, 2021 •

edited

Loading

AlekseyTs May 28, 2021 •

edited

Loading

AlekseyTs May 28, 2021 •

edited

Loading

AlekseyTs May 28, 2021 •

edited

Loading

AlekseyTs May 28, 2021 •

edited

Loading

AlekseyTs May 28, 2021 •

edited

Loading

AlekseyTs May 28, 2021 •

edited

Loading

AlekseyTs May 28, 2021 •

edited

Loading

AlekseyTs commented May 28, 2021 •

edited

Loading

AlekseyTs May 28, 2021 •

edited

Loading

AlekseyTs commented May 28, 2021 •

edited

Loading