Interpolated string handlers in the CFG by 333fred · Pull Request #57483 · dotnet/roslyn

333fred · 2021-10-30T00:26:36Z

Closes #54718. Implements support for interpolated string handlers in the CFG. For ease of review, I recommend reviewing commits 1 and 2 separately: commit 1 implements lowering for interpolated string handlers without supporting containing context. Commit 2 adds the machinery to set up the containing context and uses it everywhere we have arguments in the IOperation machinery.

Support for arguments from context is not in this commit, it will be in the next one.

…andlers.

333fred · 2021-10-30T00:47:07Z

@AlekseyTs @chsienki for review of this PR.

333fred · 2021-10-30T00:57:37Z

#57484 covers the API proposal for IsInitialized.

* Fix a few bugs revealed by the IOperation hook. * Substitute `IInstanceReferenceOperation`s for the handler capture where appropriate.

* upstream/main: (51 commits) Disable sql when built from source Fix Reduce indirections Reduce indirections Rename files Fix nullability warning Update cloud cache in the same way Ensure only one storage service factory gets created per workspace. Remove processed typename check Simplify Equals method Build CodeAction description into table and only generate Dev17 hash Avoid recomputing expensive persistence location data that does not change. Options Refactoring - Round 3 (dotnet#57254) Merge pull request dotnet#57509 from dotnet/dev/rigibson/fix-publishdata Classify method group assignments as methods (dotnet#57410) Simplify NRT Simplify Remove unnecessary workaround code. Simplify ...

333fred · 2021-11-02T21:42:45Z

@AlekseyTs @chsienki please take another look.

AlekseyTs · 2021-11-03T22:58:57Z

+                {
+                    return args.Any(
+                        static (arg, unwrapper) => unwrapper(arg) switch
+                {


{

The alignment looks wrong. #Closed

AlekseyTs · 2021-11-03T23:05:17Z

+                    IInterpolatedStringHandlerArgumentPlaceholderOperation { PlaceholderKind: not InterpolatedStringArgumentPlaceholderKind.TrailingValidityArgument } => true,
+                    IConversionOperation
+                    {
+                        Operand: IInterpolatedStringHandlerArgumentPlaceholderOperation { PlaceholderKind: not InterpolatedStringArgumentPlaceholderKind.TrailingValidityArgument }


IInterpolatedStringHandlerArgumentPlaceholderOperation

This looks fragile. I think when user-defined conversions are involved, the original operand might not be the immediate operand of a top level conversion. #Closed

AlekseyTs · 2021-11-03T23:06:16Z

+                    IInterpolatedStringHandlerArgumentPlaceholderOperation { PlaceholderKind: not InterpolatedStringArgumentPlaceholderKind.TrailingValidityArgument } => true,
+                    IConversionOperation
+                    {
+                        Operand: IInterpolatedStringHandlerArgumentPlaceholderOperation { PlaceholderKind: not InterpolatedStringArgumentPlaceholderKind.TrailingValidityArgument }


not InterpolatedStringArgumentPlaceholderKind.TrailingValidityArgument

Can this check ever fail? #Closed

AlekseyTs · 2021-11-03T23:08:53Z

+                return arguments.Any(
+                    static arg => arg.Value switch
+                    {
+                        IInterpolatedStringHandlerCreationOperation { HandlerCreation: IObjectCreationOperation { Arguments: { } creationArgs } } =>


IInterpolatedStringHandlerCreationOperation

I do not see any benefit from repeating this type check for every case within the switch. It feels like the code will be easier to follow when the type check is moved outside and performed once. #Closed

AlekseyTs · 2021-11-03T23:26:31Z

+                    PushOperand(VisitRequired(UnwrapArgumentDoNotCaptureDirectly(arguments[i])));
+                }
+
+                bool visitArgument<T>(IOperation originalArgument, ImmutableArray<T> nestedArguments, Func<T, IOperation> unwrapper) where T : IOperation


IOperation

IInterpolatedStringHandlerCreationOperation? #Closed

AlekseyTs · 2021-11-03T23:29:58Z

+            return _placeholderDictionary[key];
+        }
+
+        private void AddPlaceholder(IOperation key, IOperation value)


AddPlaceholder

AddPlaceholderReplacement? #Closed

AlekseyTs · 2021-11-03T23:30:30Z

            }
        }

+        private IOperation GetPlaceholder(IOperation key)


GetPlaceholder

GetPlaceholderReplacement? #Closed

These are no longer present.

AlekseyTs · 2021-11-03T23:30:54Z

+            (_placeholderDictionary ??= PooledDictionary<IOperation, IOperation>.GetInstance()).Add(key, value);
+        }
+
+        private void RemovePlaceholder(IOperation key)


RemovePlaceholder

RemovePlaceholderReplacement? #Closed

AlekseyTs · 2021-11-03T23:35:00Z

+
+                    IOperation visitedArgument = VisitRequired(originalArgument);
+                    Debug.Assert(visitedArgument is IFlowCaptureReferenceOperation);
+                    visitedArguments.Add(visitedArgument);


visitedArguments.Add(visitedArgument);

I think the logic would be simpler to follow if we were to limit the purpose of this method to visiting IInterpolatedStringHandlerCreationOperation argument and returning it. The caller can handle adition of the result to the list. #Closed

AlekseyTs · 2021-11-05T18:41:12Z

                if (method != null)
                {
-                    var args = disposeMethod is object ? VisitArguments(disposeArguments) : ImmutableArray<IArgumentOperation>.Empty;
+                    var args = VisitArray(disposeArguments, UnwrapArgument, RewriteArgumentFromArray);


var args = VisitArray(disposeArguments, UnwrapArgument, RewriteArgumentFromArray);

It is not obvious why we need to make changes here, especially removal of the conditional logic looks suspicious #Closed

I think that in the long term it would be better to keep calling VisitArguments with appropriate value for the second parameter.

AlekseyTs · 2021-11-05T18:48:38Z

                if (arguments != null)
                {
-                    return VisitArguments(arguments);
+                    return VisitArray(arguments, UnwrapArgument, RewriteArgumentFromArray);


VisitArray

I think that in the long term it would be better to keep calling VisitArguments with appropriate value for the second parameter. #Closed

AlekseyTs · 2021-11-05T18:52:06Z

        {
            EvalStackFrame frame = PushStackFrame();
            EvalStackFrame argumentsFrame = PushStackFrame();
-            ImmutableArray<IArgumentOperation> visitedArgs = VisitArguments(operation.Arguments);


VisitArguments

It is not clear why we simply didn't pass false as the second argument. #Closed

Because the first approach needed this version of the call, and I didn't fully revert this bit when I took the new approach.

AlekseyTs · 2021-11-05T19:09:37Z

+            int outParameterFlowCapture = -1;
+            IOperation? outParameterPlaceholder = null;
+
+            if (operation.HandlerCreationHasSuccessParameter)


if (operation.HandlerCreationHasSuccessParameter)

We can call collectAppendCalls before this if and, if the set is empty, not enter the if, leaving outParameterFlowCapture == -1. Then we suppress creation of conditional branch in the if below when outParameterFlowCapture == -1. Then we can adjust VisitInterpolatedStringHandlerArgumentPlaceholder to create IDiscardOperation in this case. This way we can eliminate an unnecessary branch, see InterpolatedStringHandlerConversionFlow_16 for example. This work doesn't look prohibiteviley complex or expensive to me. #Closed

I filed #57611 to track doing this work at some point in the future.

AlekseyTs · 2021-11-05T19:22:06Z

Done with review pass (commit 8)

333fred · 2021-11-05T22:52:04Z

@AlekseyTs addressed feedback.

In reply to: 962158460

AlekseyTs · 2021-11-08T15:51:21Z

It looks like the build is broken

AlekseyTs · 2021-11-08T16:01:56Z

            ImmutableArray<IArgumentOperation> visitedArguments = VisitArguments(arguments);

We probably want to pass false as the second parameter here.

In reply to: 963303268

Refers to: src/Compilers/Core/Portable/Operations/ControlFlowGraphBuilder.cs:5721 in 3ecd8ad. [](commit_id = 3ecd8ad, deletion_comment = False)

AlekseyTs · 2021-11-08T16:06:28Z

+
+#if DEBUG
+            Debug.Assert(_evalStack[maxStackDepth + 1].frameOpt != null);
+            if (_currentInterpolatedStringHandlerArgumentContext?.ApplicableCreationOperations.Contains(operation) ?? false)


tentiall

== true? #Closed

AlekseyTs · 2021-11-08T16:25:02Z

+
+#if DEBUG
+            Debug.Assert(_evalStack[maxStackDepth + 1].frameOpt != null);
+            if (_currentInterpolatedStringHandlerArgumentContext?.ApplicableCreationOperations.Contains(operation) ?? false)


of appe

It looks like we are leaving the room for the Contains to not find the match. I.e. we will not assert here, at the same time, it looks like we will use _currentInterpolatedStringHandlerArgumentContext regardless. #Closed

AlekseyTs · 2021-11-08T16:56:40Z

Done with review pass (commit 10)

AlekseyTs · 2021-11-08T18:48:02Z

+{
+    internal static class SpanExtensions
+    {
+        public static bool Any<T>(this ReadOnlySpan<T> span, Func<T, bool> predicate)


Any

Do we still use this helper? #Resolved

AlekseyTs

LGTM (commit 12), assuming CI is passing.

333fred · 2021-11-09T00:29:39Z

/azp run

azure-pipelines · 2021-11-09T00:30:13Z

Azure Pipelines successfully started running 4 pipeline(s).

333fred added 2 commits October 29, 2021 17:15

Support IInterpolatedStringHandlerCreationOperation in CFG.

2e9ac12

Support for arguments from context is not in this commit, it will be in the next one.

Handle arguments from the containing context in interpolated string h…

a1309dc

…andlers.

333fred requested a review from a team as a code owner October 30, 2021 00:26

ghost added the Area-Compilers label Oct 30, 2021

333fred commented Oct 30, 2021

View reviewed changes

Comment thread src/Compilers/CSharp/Test/IOperation/IOperation/IOperationTests_IInterpolatedStringOperation.cs Outdated

333fred requested review from AlekseyTs and chsienki October 30, 2021 00:47

Add case for addition as well

7cb6ee4

runfoapp bot mentioned this pull request Nov 1, 2021

Timeout on Microsoft.VisualStudio.LanguageServices.CSharp.UnitTests.dll tests #55638

Open

chsienki self-assigned this Nov 1, 2021

chsienki reviewed Nov 1, 2021

View reviewed changes

Comment thread src/Compilers/Core/Portable/Operations/ControlFlowGraphBuilder.cs

chsienki reviewed Nov 1, 2021

View reviewed changes

Comment thread src/Compilers/Core/Portable/Operations/OperationNodes.cs Outdated

chsienki reviewed Nov 1, 2021

View reviewed changes

Comment thread src/Compilers/Core/Portable/Operations/ControlFlowGraphBuilder.cs Outdated

chsienki reviewed Nov 1, 2021

View reviewed changes

Comment thread src/Compilers/Core/Portable/Operations/ControlFlowGraphBuilder.cs Outdated

333fred added 3 commits November 2, 2021 14:38

Additonal CFG Support:

c178b21

* Fix a few bugs revealed by the IOperation hook. * Substitute `IInstanceReferenceOperation`s for the handler capture where appropriate.

Update test after merge.

3e45d0c

Make fields readonly where applicable.

3c58e1c

AlekseyTs reviewed Nov 3, 2021

View reviewed changes

AlekseyTs reviewed Nov 5, 2021

View reviewed changes

333fred mentioned this pull request Nov 5, 2021

Remove extraneous branch for empty strings in interpolated string handler CFG #57611

Open

PR feedback.

ace0f18

Merge upstream/main into interpolated-cfg

3ecd8ad

AlekseyTs reviewed Nov 8, 2021

View reviewed changes

Fix build break after merge.

c45fee6

AlekseyTs reviewed Nov 8, 2021

View reviewed changes

Minor reformatting.

74edc2f

AlekseyTs approved these changes Nov 8, 2021

View reviewed changes

333fred added 2 commits November 8, 2021 10:52

Remove unused helper.

66b85ca

Use IsDefaultOrEmpty instead of IsEmpty

c53c0e3

chsienki approved these changes Nov 8, 2021

View reviewed changes

333fred enabled auto-merge (squash) November 9, 2021 01:39

333fred merged commit 573f06d into dotnet:main Nov 9, 2021

ghost added this to the Next milestone Nov 9, 2021

333fred deleted the interpolated-cfg branch November 9, 2021 03:29

333fred mentioned this pull request Nov 10, 2021

IFlowCaptureReferenceOperation.IsInitialization #57484

Closed

allisonchou modified the milestones: Next, 17.1.P2 Nov 30, 2021

Conversation

333fred commented Oct 30, 2021

Uh oh!

Uh oh!

333fred commented Oct 30, 2021

Uh oh!

333fred commented Oct 30, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

333fred commented Nov 2, 2021

Uh oh!

AlekseyTs Nov 3, 2021 • 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!

AlekseyTs Nov 3, 2021 • 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!

AlekseyTs Nov 3, 2021 • 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!

AlekseyTs Nov 3, 2021 • 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!

AlekseyTs Nov 3, 2021 • 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!

AlekseyTs Nov 3, 2021 • 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!

AlekseyTs Nov 3, 2021 • 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 Nov 5, 2021

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Nov 3, 2021 • 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!

AlekseyTs Nov 3, 2021 • 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!

AlekseyTs Nov 5, 2021 • 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!

AlekseyTs Nov 5, 2021

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Nov 5, 2021 • 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!

AlekseyTs Nov 5, 2021 • 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 Nov 5, 2021

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Nov 5, 2021 • 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 Nov 5, 2021

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Nov 5, 2021

Uh oh!

AlekseyTs Nov 3, 2021 •

edited

Loading

AlekseyTs Nov 3, 2021 •

edited

Loading

AlekseyTs Nov 3, 2021 •

edited

Loading

AlekseyTs Nov 3, 2021 •

edited

Loading

AlekseyTs Nov 3, 2021 •

edited

Loading

AlekseyTs Nov 3, 2021 •

edited

Loading

AlekseyTs Nov 3, 2021 •

edited

Loading

AlekseyTs Nov 3, 2021 •

edited

Loading

AlekseyTs Nov 3, 2021 •

edited

Loading

AlekseyTs Nov 5, 2021 •

edited

Loading

AlekseyTs Nov 5, 2021 •

edited

Loading

AlekseyTs Nov 5, 2021 •

edited

Loading

AlekseyTs Nov 5, 2021 •

edited

Loading

AlekseyTs commented Nov 8, 2021 •

edited by 333fred

Loading

AlekseyTs Nov 8, 2021 •

edited

Loading

AlekseyTs Nov 8, 2021 •

edited

Loading

AlekseyTs Nov 8, 2021 •

edited by 333fred

Loading