Correctly record the dispose method to be invoked in operations #49481

chsienki · 2020-11-18T21:41:05Z

Correctly record the dispose method in operations that will be used when not interface based
Fix CFG to correctly handle struct based disposal and list the correct method
Update and add tests

Fixes #49267

chsienki · 2020-11-18T21:41:58Z

@dotnet/roslyn-compiler for review

Note: this is targeting @333fred's IOperation refactoring branch rather than master to reduce merge issues

333fred

Done review pass (commit 2)

333fred · 2020-11-19T00:08:16Z

src/Compilers/Core/Portable/Operations/OperationInterfaces.xml

@@ -485,6 +485,11 @@
        </summary>
      </Comments>
    </Property>
+    <Property Name="DisposeMethod" Type="IMethodSymbol?">


It doesn't feel like making this only be populated if it's not IDisposable.Dispose is particularly useful, it feels more like an implementation detail bleeding through. I think we should just always populate this, and if it's IDiposable then we populate it with IDisposable.Dispose. #Closed

333fred · 2020-11-19T00:15:11Z

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

@@ -3862,7 +3862,7 @@ void processResource(IOperation resource, ArrayBuilder<(IVariableDeclarationOper

                LeaveRegion();

-                AddDisposingFinally(resource, knownToImplementIDisposable: true, iDisposable, isAsynchronous);
+                AddDisposingFinally(resource, knownToImplementIDisposable: true, iDisposable, disposeMethod, isAsynchronous);


Well, it's certainly not know to implement IDisposable if it's a ref struct. Please update the comments above and the parameter name. #Closed

- Always populate the dispose method in binding, even if its for I{Async}Disposable - Remove knownToImplementIDisposable - Just use the precalcualted dispose method in IOperation rather than calculating it - Use the presence of the DisposeMethod to calculate the expected output in CFG - Correctly report invalid operations in CFG - Update tests

chsienki · 2020-11-20T19:25:34Z

@333fred / @AlekseyTs assuming tests are good, this should be good to review now.

By always populating the dispose method in binding I was able to remove a bunch of logic in the operation and lowering to just use what we found, instead of calculating it again.

chsienki · 2020-11-20T21:26:00Z

Note to self: run VB tests locally too. 🤦‍♂️

333fred · 2020-11-20T21:52:28Z

Note on the determinism test: I haven't merged Aleksey's fix from master into this branch yet, so it's flaky.

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

333fred · 2020-11-20T21:57:55Z

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

-            AppendNewBlock(endOfFinally);
+                var isPatternDispose = !disposeMethod.ContainingType.Equals(iDisposable, SymbolEqualityComparer.Default);
+                var isNull = resource.GetConstantValue() == ConstantValue.Null;
+                var requiresConversion = !isPatternDispose && !isAsynchronous && !isNull && !_compilation.HasImplicitConversion(resource.Type, iDisposable) && !isNonNullableValueType(resource.Type);


!_compilation.HasImplicitConversion(resource.Type, iDisposable) [](start = 92, length = 63)

If it has an implicit conversion, we probably still need to represent that implicit conversion, right? Or am I misunderstanding the expansion?

If it has an implicit conversion we'll represent that later on. This checks if we're doing a runtime conversion check. That only happens in foreach on enumerators. The simplest example is with a dynamic enumerator: we try and convert it to IDisposable first then check it for null. I'll give the var a more descriptive name.

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

333fred · 2020-11-20T22:28:30Z

Done review pass (commit 9)

…cfg_pattern_dispose

src/Compilers/Core/Portable/Operations/OperationInterfaces.xml

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

AlekseyTs · 2020-11-26T01:37:57Z

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

@@ -125,7 +125,7 @@ internal static BoundStatement BindUsingStatementOrDeclarationFromParts(SyntaxNo
                if (declarationTypeOpt.IsDynamic())
                {
                    iDisposableConversion = Conversion.ImplicitDynamic;
-                    disposeMethodOpt = null;
+                    disposeMethodOpt = originalBinder.Compilation.GetWellKnownDisposeMethod(isAsync: hasAwait);


originalBinder.Compilation.GetWellKnownDisposeMethod(isAsync: hasAwait); [](start = 39, length = 72)

Same comment as for other call-sites #Closed

AlekseyTs · 2020-11-26T01:39:24Z

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

@@ -185,6 +185,7 @@ bool populateDisposableConversionOrDisposeMethod(bool fromExpression, out Conver

                if (iDisposableConversion.IsImplicit)
                {
+                    disposeMethodOpt = originalBinder.Compilation.GetWellKnownDisposeMethod(hasAwait);


originalBinder.Compilation.GetWellKnownDisposeMethod(hasAwait); [](start = 39, length = 63)

Same comment #Closed

AlekseyTs · 2020-11-26T01:47:31Z

src/Compilers/CSharp/Portable/Operations/CSharpOperationFactory.cs

+                                    : compilation.GetSpecialType(SpecialType.System_IDisposable);
+
+                var enumeratorImplementsDisposable = enumeratorInfoOpt.NeedsDisposal
+                                                     && compilation.Conversions.ClassifyImplicitConversionFromType(enumeratorInfoOpt.GetEnumeratorMethod.ReturnType,


enumeratorInfoOpt.GetEnumeratorMethod [](start = 115, length = 37)

It looks like we dropped a null check for this value. #Closed

It was never supposed to be able to contain a null value. The enumeratorInfo has nullable enabled.

It was never supposed to be able to contain a null value. The enumeratorInfo has nullable enabled.

It looks to me that definition of ForEachEnumeratorInfo is in nullable disabled region.

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

AlekseyTs · 2020-11-26T01:50:14Z

src/Compilers/VisualBasic/Test/IOperation/IOperation/IOperationTests_IUsingStatement.vb

@@ -1008,7 +1008,7 @@ IExpressionStatementOperation (OperationKind.ExpressionStatement, Type: null) (S

        <CompilerTrait(CompilerFeature.IOperation, CompilerFeature.Dataflow)>
        <Fact()>
-        Public Sub UsingFlow_14()
+        Public Sub UsingFlow_01()


UsingFlow_01 [](start = 19, length = 12)

Why was that renamed? Is it possible that it corresponded to C# test with the same name? #Closed

Long ago there were UsingFlow_{1-13} in C# and then {14,15} in VB, but C# has since added significantly more tests with {14...} so it makes no sense to have a random set of numbered VB tests. We can move these to the end of the C# tests, but the likelihood is just that they'll clash again.

Ideally we'd give them descriptive names tbh

We can move these to the end of the C# tests, but the likelihood is just that they'll clash again.

There is no requirement for test methods to be sorted in any way. The test was not affected by the change as well. I find the rename unnecessary.

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

AlekseyTs · 2020-11-26T01:50:30Z

src/Compilers/VisualBasic/Test/IOperation/IOperation/IOperationTests_IUsingStatement.vb

@@ -1152,7 +1152,7 @@ Block[B8] - Exit

        <CompilerTrait(CompilerFeature.IOperation, CompilerFeature.Dataflow)>
        <Fact()>
-        Public Sub UsingFlow_15()
+        Public Sub UsingFlow_02()


UsingFlow_02 [](start = 19, length = 12)

Why was this renamed? #Closed

AlekseyTs · 2020-11-26T01:52:24Z

src/Compilers/VisualBasic/Test/IOperation/IOperation/IOperationTests_IUsingStatement.vb

+
+        <CompilerTrait(CompilerFeature.IOperation, CompilerFeature.Dataflow)>
+        <Fact()>
+        Public Sub UsingFlow_04()


UsingFlow_04 [](start = 19, length = 12)

Is there a similar C# test? Consider using the same name. #Closed

AlekseyTs · 2020-11-26T01:57:26Z

src/Compilers/VisualBasic/Test/IOperation/IOperation/IOperationTests_IUsingStatement.vb

@@ -1236,5 +1236,171 @@ Block[B6] - Exit
            VerifyFlowGraphAndDiagnosticsForTest(Of MethodBlockSyntax)(source, expectedFlowGraph, expectedDiagnostics)
        End Sub

+        <CompilerTrait(CompilerFeature.IOperation, CompilerFeature.Dataflow)>
+        <Fact()>
+        Public Sub UsingFlow_03()


UsingFlow_03 [](start = 19, length = 12)

Is there a similar C# test? Consider using the same name. #Closed

AlekseyTs · 2020-11-26T01:58:52Z

Done with review pass (commit 20). Overall, it feels to me that a lot of changes could be avoided. Let's discuss offline in more details. #Closed

AlekseyTs · 2020-11-26T16:51:16Z

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

-                        return new AwaitOperation(invocation, semanticModel: null, value.Syntax, _compilation.GetSpecialType(SpecialType.System_Void), isImplicit: true);
-                    }
+                // resource.Dispose();
+                resource = new InvocationOperation(disposeMethod, resource, isVirtual: true,


new InvocationOperation(disposeMethod, resource, isVirtual: true, [](start = 27, length = 65)

Can pattern-based expose use an extension method? In that scenario, the resource should be the first parameter instead. Do we need to handle omitted optional and param array parameters? #Closed

No, pattern disposal does not support extensions methods. We should report the params and optional arguments, however

We should report the params and optional arguments, however

See how we deal with arguments for GetEnumerator and MoveNext in VisitForEachLoop

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

@AlekseyTs I have a fix for this, but its a little involved, as we now need to pass the binder down through the BoundUsingStatement like we do for Foreach. Given the mild complexity, do you want me to add it to this PR, or wait and open a separate one?

Given the mild complexity, do you want me to add it to this PR, or wait and open a separate one?

I think a separate PR is fine.

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

AlekseyTs · 2020-12-02T00:45:53Z

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

+                // {
+                //   ResourceType resource = expr;
+                //   try { statement; }
+                //   finally { if (resource != null) resource.Dispose(); }


resource [](start = 53, length = 8)

Is this accurate for a nullable value type? We are not unwrapping the Nullable<T>? #Closed

Updated the comment to show what we do in lowering.

Updated the comment to show what we do in lowering.

Why would you do this? Is this comment for lowering? Is this comment for lowering of a specific compiler?

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

I understood your comment to mean that we should be showing the unwrapping of the nullable in CFG, but clearly I misunderstood you.

I understood your comment to mean that we should be showing the unwrapping of the nullable in CFG, but clearly I misunderstood you.

I asked you about behavior that you are describing in a new comment specifically for a pattern-based case. I couldn't find a test for it, hence the question. Clearly the comment looked strange to me, but you started looking at completely different scenario.

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

Apologies, as I said, I misunderstood what you were asking.

AlekseyTs · 2020-12-02T01:08:58Z

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

@@ -963,7 +964,7 @@ private MethodSymbol GetGetEnumeratorMethod(NamedTypeSymbol collectionType, Diag
            return getEnumeratorMethod;
        }

-        private ForEachEnumeratorInfo.Builder GetDefaultEnumeratorInfo(ForEachEnumeratorInfo.Builder builder, DiagnosticBag diagnostics, TypeSymbol collectionExprType)
+        private ForEachEnumeratorInfo.Builder GetDefaultEnumeratorInfo(ForEachEnumeratorInfo.Builder builder, DiagnosticBag diagnostics, TypeSymbol collectionExprType, bool isAsync)


, bool isAsync [](start = 166, length = 14)

It looks like this parameter is not used. #Closed

AlekseyTs · 2020-12-02T01:30:01Z

Done with review pass (commit 24) #Closed

AlekseyTs · 2020-12-02T01:37:38Z

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

                //
                // {
                //   ResourceType resource = expr;
                //   try { statement; }
                //   finally { ((IDisposable)resource).Dispose(); }
                // }
                //
+                // Otherwise, when ResourceType is a non-nullable value type that implements
+                // disposal via pattern, the expansion is:


Are we testing this scenario? #Closed

Yes in UsingDeclaration_Flow_20 #Closed

AlekseyTs · 2020-12-02T01:38:07Z

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

                //
                // {
                //   ResourceType resource = expr;
                //   try { statement; }
                //   finally { if (resource != null) ((IDisposable)resource).Dispose(); }
                // }
                //
+                // Otherwise, when Resource type is a nullable value type or


when Resource type is a nullable value type [](start = 30, length = 43)

Are we testing this scenario? #Closed

Yes, in UsingFlow_05 but actually, it doesn't appear to be correctly reporting what we do. It matches the comments in the Builder, but that isnt how we lower the code, as you pointed out we unwrap the nullable, which we're not representing here. https://sharplab.io/#v2:EYLgtghgzgLgpgJwDQBMQGoA+ABATAAgAUBYAKAG8z9r9sAWfAWQApGBPAEQEsoAHAeygRgAGzgB+fFwB2vAK4wk+YP34jlASjIB6AFQAeYDJQA+Xdsqka12gAZ8zAOJwY3PoOFjmG/OMkz5GC0rG2pLUNDgfABefBgEOTgAbioIgF9U6jS9fW0jaVNzMkymTh4BIVEJfGdXco8q7xiTOIALBH4Ad3xpORERFNIM0jJYBIBjGFK3Cs84fBBaAEYAOgBJGYaxMnCabABmWgZNqDgm6JaYdq6evoGyNKA=

Oh, interesting. Actually we're representing in ControlFlowGraph what the spec says we should be doing:
http://sourceroslyn.io/#Microsoft.CodeAnalysis.CSharp/Lowering/LocalRewriter/LocalRewriter_UsingStatement.cs,277

Except in lowering we purposefully deviate from the spec to improve perf.

@AlekseyTs what do you think? Should the CFG represent the spirit of the code in the spec, or what we actually lower to? I guess I lean towards representing the spec? because like the async stuff we give a general representation of what it's lowered to, not the implementation specific details.

It looks to me that UsingFlow_05 and the sharplab link is about non-pattern disposal and this change is not supposed to make any changes for these scenarios. I am fine with the current behavior of the builder for them. The question I have is about pattern-based disposal for nullable value types. This is what this comment is about. Are we testing the scenario?

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

It is not currently possible to perform pattern disposal on a nullable value types: see #34701

I will add a test to show the error recovery scenario, and update the comment in CFG to correctly reflect that.

AlekseyTs · 2020-12-03T02:01:05Z

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

+                // {
+                //   ResourceType resource = expr;
+                //   try { statement; }
+                //   finally { if (resource.HasValue()) ((IDisposable)resource.GetValueOrDefault()).Dispose(); }


finally { if (resource.HasValue()) ((IDisposable)resource.GetValueOrDefault()).Dispose(); } [](start = 21, length = 91)

Where do you see this in flow graph tests? #Closed

Mentioned above, we don't. The test shows something different: do we want to make the CFG represent the actual lowered value or what the spec says?

Mentioned above, we don't. The test shows something different: do we want to make the CFG represent the actual lowered value or what the spec says?

Does the comment accurately reflect CFG builder's behavior? If so, we don't want to make any changes for the comment. We also do not want to make any changes for scenarios that are not about pattern-based disposal, which is supposed to be the focus of this PR.

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

Yes apologies. I was confused by your comment, I thought you were saying that the comment, and thus the tests are incorrect for those scenarios.

AlekseyTs

LGTM (commit 26), assuming that we will deal with the arguments in a separate PR.

chsienki · 2020-12-07T22:42:35Z

@dotnet/roslyn-compiler / @333fred for a second review please :)

333fred

LGTM (commit 26)

…c-foreach-binding * upstream/features/ioperation: Correctly record the dispose method to be invoked in operations (dotnet#49481)

Correctly record the dispose method to be invoked in operations

257342a

chsienki added Area-Compilers Feature - IOperation IOperation labels Nov 18, 2020

chsienki requested a review from a team as a code owner November 18, 2020 21:41

Fix whitespace

64becc9

333fred reviewed Nov 19, 2020

View reviewed changes

chsienki marked this pull request as draft November 19, 2020 19:38

chsienki added 3 commits November 19, 2020 18:26

Explicitly record if we're doing pattern based disposal

dbaebf2

Remove uneeded method look up in local rewriter

06fd3df

chsienki marked this pull request as ready for review November 20, 2020 19:24

chsienki added 2 commits November 20, 2020 11:49

Remove comment

3500038

Remove nullable annotation + add debug.assert

eac0131

Fix VB dispose tests

378f453

333fred reviewed Nov 20, 2020

View reviewed changes

src/Compilers/Core/Portable/Operations/ControlFlowGraphBuilder.cs Outdated Show resolved Hide resolved

333fred reviewed Nov 20, 2020

View reviewed changes

src/Compilers/Core/Portable/Operations/ControlFlowGraphBuilder.cs Outdated Show resolved Hide resolved

333fred reviewed Nov 20, 2020

View reviewed changes

Remove unused code

0dbc267

333fred reviewed Nov 20, 2020

View reviewed changes

src/Compilers/Core/Portable/Operations/ControlFlowGraphBuilder.cs Outdated Show resolved Hide resolved

chsienki added 2 commits November 20, 2020 14:46

PR Feedback

55b486a

Merge remote-tracking branch 'upstream/features/ioperation' into fix_…

98cf4f0

…cfg_pattern_dispose

AlekseyTs reviewed Nov 20, 2020

View reviewed changes

src/Compilers/Core/Portable/Operations/OperationInterfaces.xml Outdated Show resolved Hide resolved

AlekseyTs reviewed Nov 20, 2020

View reviewed changes

src/Compilers/Core/Portable/Operations/OperationInterfaces.xml Outdated Show resolved Hide resolved

AlekseyTs reviewed Nov 20, 2020

View reviewed changes

src/Compilers/Core/Portable/Operations/ControlFlowGraphBuilder.cs Outdated Show resolved Hide resolved

AlekseyTs reviewed Nov 20, 2020

View reviewed changes

src/Compilers/Core/Portable/Operations/ControlFlowGraphBuilder.cs Outdated Show resolved Hide resolved

AlekseyTs reviewed Nov 26, 2020

View reviewed changes

PR Feedback

430214d

chsienki mentioned this pull request Dec 1, 2020

Extension based GetEnumerator does not report default argument in ControlFlowGraph #49709

Closed

chsienki added 3 commits December 1, 2020 16:07

Fix tests

af2dc06

PR feedback

016fe90

Restore comments

5995966

AlekseyTs reviewed Dec 2, 2020

View reviewed changes

PR Feedback

4410ecc

AlekseyTs reviewed Dec 3, 2020

View reviewed changes

Add test for nullable value type pattern disposal

a9e8edb

AlekseyTs approved these changes Dec 3, 2020

View reviewed changes

333fred approved these changes Dec 8, 2020

View reviewed changes

chsienki merged commit e929e00 into dotnet:features/ioperation Dec 8, 2020

chsienki mentioned this pull request Dec 8, 2020

CFG pattern dispose with args #49842

Merged

chsienki mentioned this pull request Jan 14, 2021

CFG does not handle pattern dispose correctly #49267

Closed

chsienki linked an issue Jan 14, 2021 that may be closed by this pull request

CFG does not handle pattern dispose correctly #49267

Closed

Correctly record the dispose method to be invoked in operations #49481

Correctly record the dispose method to be invoked in operations #49481

Conversation

chsienki commented Nov 18, 2020

chsienki commented Nov 18, 2020

333fred left a comment

Choose a reason for hiding this comment

333fred Nov 19, 2020 • edited Loading

Choose a reason for hiding this comment

333fred Nov 19, 2020 • edited Loading

Choose a reason for hiding this comment

chsienki commented Nov 20, 2020

chsienki commented Nov 20, 2020

333fred commented Nov 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

333fred commented Nov 20, 2020

AlekseyTs Nov 26, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 26, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Nov 26, 2020 • 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 Nov 26, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 26, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 26, 2020 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented Nov 26, 2020 • edited Loading

AlekseyTs Nov 26, 2020 • 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

Choose a reason for hiding this comment

AlekseyTs Dec 2, 2020 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Dec 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented Dec 2, 2020 • edited Loading

AlekseyTs Dec 2, 2020 • edited Loading

Choose a reason for hiding this comment

chsienki Dec 3, 2020 • edited by 333fred Loading

Choose a reason for hiding this comment

AlekseyTs Dec 2, 2020 • 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

Choose a reason for hiding this comment

AlekseyTs Dec 3, 2020 • edited Loading

Choose a reason for hiding this comment

chsienki Dec 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs left a comment

Choose a reason for hiding this comment

chsienki commented Dec 7, 2020

333fred left a comment

Choose a reason for hiding this comment

333fred Nov 19, 2020 •

edited

Loading

333fred Nov 19, 2020 •

edited

Loading

AlekseyTs Nov 26, 2020 •

edited

Loading

AlekseyTs Nov 26, 2020 •

edited

Loading

AlekseyTs Nov 26, 2020 •

edited

Loading

AlekseyTs Nov 26, 2020 •

edited

Loading

AlekseyTs Nov 26, 2020 •

edited

Loading

AlekseyTs Nov 26, 2020 •

edited

Loading

AlekseyTs Nov 26, 2020 •

edited

Loading

AlekseyTs commented Nov 26, 2020 •

edited

Loading

AlekseyTs Nov 26, 2020 •

edited

Loading

AlekseyTs Dec 2, 2020 •

edited

Loading

AlekseyTs Dec 2, 2020 •

edited

Loading

AlekseyTs commented Dec 2, 2020 •

edited

Loading

AlekseyTs Dec 2, 2020 •

edited

Loading

chsienki Dec 3, 2020 •

edited by 333fred

Loading

AlekseyTs Dec 2, 2020 •

edited

Loading

AlekseyTs Dec 3, 2020 •

edited

Loading

chsienki Dec 3, 2020 •

edited

Loading