Dispose pattern refactor #30437

chsienki · 2018-10-10T22:21:25Z

Refactor pattern lookup logic:

Use BindMethodGroupInvocation to perform lookup, rather than manually iterating with LookupMembersInType
Share the pattern logic with GetPinnableReference lookup
Update tests to match new logic

This change aims to reduce the duplication we have for looking up pattern based methods, and attempt to set a path for future lookups. It makes the dispose lookup work in the same way we currently lookup GetPinnableReference, meaning we support any candidate that would be equivalent to writing c.Dispose() in code: essentially we attempt to perform binding on what we'll eventually lower to and report the results.

Previously we gave warnings about e.g. ambiguous candidates, or inaccessible methods; because we're actually doing an invocation binding these are now reported as errors as if the user had actually invoked it. This is actually consistent with the behavior we see when looking up Deconstruct and GetPinnableReference, so while different (and possibly not as nice?) it's at least consistent with what we do in other pattern lookups.

We'll still report a warning (and an overall error) when binding succeeded but the thing that was bound doesn't fit the pattern.

- Use BindMethodGroupInvocation to perform lookup, rather than manually iterating with LookupMembersInType - Share the pattern logic with GetPinnableReference lookup - Update tests to match new logic

- Fail refextensionmethod tests + add prototype comment noting it doesn't work.

chsienki · 2018-10-12T17:09:43Z

@dotnet-bot retest ubuntu_16_mono_debug_prtest please

chsienki · 2018-10-12T17:10:05Z

@dotnet-bot retest windows_release_unit64_prtest please

jcouv · 2018-10-12T18:06:29Z

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

@@ -500,7 +497,14 @@ static void Main()
        }
    }
 }";
-            CreateCompilation(source).VerifyDiagnostics();
+            CreateCompilation(source).VerifyDiagnostics(


VerifyDiagnostics [](start = 38, length = 17)

📝 this scared me (looked like a breaking change), but this is actually restoring the existing behavior (master produces error CS1674: 'C2': type used in a using statement must be implicitly convertible to 'System.IDisposable') #Resolved

jcouv · 2018-10-12T18:13:06Z

    public void UsingPatternWithParamsTest()

It would be be great to capture the new standard rules of binding to a pattern into a speclet (under docs/features), if you don't have one already.

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/UsingStatementTests.cs:1108 in 8755cf0. [](commit_id = 8755cf0, deletion_comment = False)

jcouv · 2018-10-12T18:14:24Z

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

+        Success,
+
+        /// <summary>
+        /// A member was looked up, but it was not a method


looked up [](start = 25, length = 9)

nit: "found" instead of "looked up" (also below) #Resolved

jcouv · 2018-10-12T19:57:33Z

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

                diagnostics.Add(declarationSyntax, useSiteDiagnostics);

                if (!iDisposableConversion.IsImplicit)
                {
-                   disposeMethod = TryFindDisposePatternMethod(declTypeExpr, declarationSyntax, diagnostics);
+                   disposeMethod = TryFindDisposePatternMethod(declaration.InitializerOpt, declarationSyntax, diagnostics);


declaration.InitializerOpt [](start = 63, length = 26)

I'm surprised that we look at the initializer expression, rather than the declared type.
using (TypeWithDispose x = new TypeWithoutDispose()) ... (with a user-defined conversion from one type to another) would be a relevant scenario.

You may need to introduce a placeholder bound node (a node with just a type) to adapt to the method API.
Update: you may not need a placeholder. Maybe declarations[0].DeclaredType would work?

Yes, it feels like we should use the declared type Today, the second using is an error:

using (FileStream x = new FileStream("", FileMode.Open)) { } using (object y = new FileStream("", FileMode.Open)) { } // error

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

Fixed + added tests to cover.

jcouv · 2018-10-12T20:01:36Z

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

                {
-                    diagnostics.Add(ErrorCode.WRN_PatternIsAmbiguous, syntaxExpr.Location, typeExpr.Type, messageID.Localize(),
-                                           resolution.OverloadResolutionResult.Results[0].Member, resolution.OverloadResolutionResult.Results[1].Member);
+                    // bound to something uncallable


uncallable [](start = 42, length = 10)

The comment seems out of date, as it seems to better describe return PatternLookupResult.NotCallable; #Closed

jcouv · 2018-10-12T20:08:44Z

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

+                                                  out var disposeMethod);
+
+
+            if (disposeMethod?.ReturnsVoid == false || result == PatternLookupResult.NotAMethod)


result == PatternLookupResult.NotAMethod [](start = 55, length = 40)

What about other possible result values? (such as PatternLookupResult.NotCallable)
Is it better to report such cases as pattern mismatch or no conversion to IDisposable?

In all cases other than NotAMethod, the bindmethodgroup will add applicable errors. This case handles when Dispose wasn't a method (so we didn't try and bind), or when x.Dispose() was a valid call, but didn't fit the pattern: I'm clarifying the logic in the speclet.

For such situations in other contexts, I used enums with 3 values: Success, Failed, FailedNotReported. Consider whether that is applicable here, or if we care about the more detailed results with specific semantics.

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

jcouv · 2018-10-12T20:10:56Z

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

-            finally
+
+            if (HasOptionalOrVariableParameters(patterMethodSymbol) ||
+                patterMethodSymbol.ReturnsVoid ||


patterMethodSymbol [](start = 16, length = 18)

typo: "pattern" #Closed

jcouv · 2018-10-12T20:14:26Z

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

@@ -3301,110 +3219,89 @@ internal virtual ImmutableArray<LabelSymbol> Labels
        /// Perform a lookup for the specified method on the specified type, searching further if the first resolved symbol doesn't match


specified type [](start = 61, length = 14)

comment may be out-of-date

jcouv · 2018-10-12T20:19:58Z

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

+                                                  out var disposeMethod);
+
+
+            if (disposeMethod?.ReturnsVoid == false || result == PatternLookupResult.NotAMethod)


if [](start = 12, length = 2)

nit: double empty line above #Closed

jcouv · 2018-10-12T20:22:03Z

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

-                }
-
-                return patterMethodSymbol;
+            if (result != PatternLookupResult.Success)


Consider replacing this with (object)patternMethodSymbol != null ~~in the if below~~ here. That way it's clear that patternMethodSymbol is not null when it is dereferenced in if below. #Closed

jcouv · 2018-10-12T20:23:29Z

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

        {
-            Debug.Assert(lookupResult.IsClear);
+            // PROTOTYPE: try and resolve the method using binding 


try and resolve the method using binding [](start = 26, length = 41)

📝 I didn't understand this PROTOTYPE comment. Could you clarify?

We were originally doing lookup by manually iterating the members. This was a prototype to try and find the pattern by instead assuming it exists and attempt to bind the method group invocation. In implementing it, I've become fairly convinced this is the right direction, so i'll remove the prototype comment.

jcouv · 2018-10-12T20:23:57Z

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

+                       indexed: false,
+                       bindingDiagnostics);
+
+                BoundMethodGroup bmg = new BoundMethodGroup(syntaxNode, default, receiver, "Dispose", ImmutableArray<MethodSymbol>.Empty, LookupResult.GetInstance(), BoundMethodGroupFlags.SearchExtensionMethods, false);


default [](start = 72, length = 7)

Consider naming default and false arguments. #Closed

jcouv · 2018-10-12T20:24:16Z

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

+                       indexed: false,
+                       bindingDiagnostics);
+
+                BoundMethodGroup bmg = new BoundMethodGroup(syntaxNode, default, receiver, "Dispose", ImmutableArray<MethodSymbol>.Empty, LookupResult.GetInstance(), BoundMethodGroupFlags.SearchExtensionMethods, false);


"Dispose" [](start = 91, length = 9)

Should be methodName. Please add test to demonstrate this issue. #Closed

jcouv · 2018-10-12T20:26:28Z

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

+                       indexed: false,
+                       bindingDiagnostics);
+
+                BoundMethodGroup bmg = new BoundMethodGroup(syntaxNode, default, receiver, "Dispose", ImmutableArray<MethodSymbol>.Empty, LookupResult.GetInstance(), BoundMethodGroupFlags.SearchExtensionMethods, false);


nit: acronym variables are not common in compiler code. Let's give a proper name.
nit: var could help avoid redundant type name. #Closed

jcouv · 2018-10-12T20:28:10Z

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

+                       indexed: false,
+                       bindingDiagnostics);
+
+                BoundMethodGroup bmg = new BoundMethodGroup(syntaxNode, default, receiver, "Dispose", ImmutableArray<MethodSymbol>.Empty, LookupResult.GetInstance(), BoundMethodGroupFlags.SearchExtensionMethods, false);


bmg [](start = 33, length = 3)

Never mind, it looks like bmg is not referenced. Let's remove it. #Closed

Oops. left that in by mistake. Good catch, thanks #Closed

jcouv · 2018-10-12T20:39:18Z

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

+            if (disposeMethod?.ReturnsVoid == false || result == PatternLookupResult.NotAMethod)
+            {
+                ReportPatternWarning(diagnostics, expr.Type, disposeMethod, syntaxNode, MessageID.IDS_Disposable);
+                disposeMethod = null;
            }


} [](start = 12, length = 1)

For binding fixed pattern, we do some extra checks (copied below). Just want to confirm that we're intentionally not doing those checks for using pattern.
It would be good to have a test for static Dispose method (that's not an extension).

if (HasOptionalOrVariableParameters(patterMethodSymbol) || patterMethodSymbol.ReturnsVoid || !patterMethodSymbol.RefKind.IsManagedReference() || !(patterMethodSymbol.ParameterCount == 0 || patterMethodSymbol.IsStatic && patterMethodSymbol.ParameterCount == 1)) { // the method does not fit the pattern additionalDiagnostics.Add(ErrorCode.WRN_PatternBadSignature, initializer.Syntax.Location, initializer.Type, "fixed", patterMethodSymbol); return null; }

Generalized pattern lookup has two phases: lookup and verification. We're sharing the lookup phases, but each pattern has its own unique verification. In this case dispose is a bit more lenient than fixed.

Added a test for the static method case.

jcouv

Done with review pass (iteration 3).

cston · 2018-10-15T23:27:20Z

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

                }
-                else
+
+                // we have succeeded or almost succeded to bind the method


succeded [](start = 47, length = 8)

Typo

cston · 2018-10-16T14:49:21Z

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

                //         using (C1 c = new C1())
-                Diagnostic(ErrorCode.WRN_PatternIsAmbiguous, "C1 c = new C1()").WithArguments("C1", "disposable", "C1.Dispose()", "C1.Dispose()").WithLocation(13, 16),
-                // (13,16): error CS1674: 'C1': type used in a using statement must have a public void-returning Dispose() instance method.
+                Diagnostic(ErrorCode.ERR_AmbigCall, "C1 c = new C1()").WithArguments("C1.Dispose()", "C1.Dispose()").WithLocation(13, 16),


ERR_AmbigCall [](start = 37, length = 13)

We're now reporting that "the call is ambiguous" rather than the pattern method is ambiguous. Are all of uses of FindPatternMethodRelaxed for calls to the pattern method?

Yes, this is consistent with how GetPinnableReference works today, and that's what we're basing the generalized pattern lookup off at the moment (it may change after review/LDM).

jcouv · 2018-10-16T23:42:17Z

Removed the "personal review" label since it looks like the PR is ready for review. #Resolved

- Make a bound local to be the receiver - Add tests for ref/in extension methods - Add tests for implict conversions

- Add a static pattern dispose test - Fix a typo

chsienki · 2018-11-01T17:40:45Z

@dotnet-bot retest ubuntu_16_mono_debug_prtest please

chsienki · 2018-11-02T20:50:49Z

@dotnet-bot retest ubuntu_16_mono_debug_prtest please

chsienki · 2018-11-05T20:34:01Z

@dotnet/roslyn-compiler for another round of reviews please

RikkiGibson · 2018-11-05T21:09:10Z

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

-        {
-            AnalyzedArguments arguments = AnalyzedArguments.GetInstance();
-            MethodSymbol patternMethod = null;
+                if (patternMethodCall.Kind != BoundKind.Call)


Just for my edification--what other BoundKinds could be found here that would result in returning NotCallable?

It could also be a DynamicInvocation if any of the arguments are dynamic. In the dispose case it can't be (we never pass params) but in the general case its something we have to consider.

I should add a test for an optional dynamic arg though, as that should work ok.

cston · 2018-11-05T21:44:59Z

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

-            BoundTypeExpression declTypeExpr = declarations[0].DeclaredType;
+            BoundLocalDeclaration declaration = declarations[0];
+            TypeSymbol declType = declaration.DeclaredType.Type;
+            BoundLocal declarationLocal = new BoundLocal(declaration.Syntax, declaration.LocalSymbol, null, declType);


BoundLocal declarationLocal [](start = 12, length = 27)

Can be moved inside if (!iDisposableConversion.IsImplicit) { ... }.

cston · 2018-11-05T21:51:12Z

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

+        /// <returns>The <see cref="MethodSymbol"/> of the Dispose method if one is found, otherwise null.</returns>
+        internal MethodSymbol TryFindDisposePatternMethod(BoundExpression expr, SyntaxNode syntaxNode, DiagnosticBag diagnostics)
+        {
+            if(expr?.Type is null)


expr [](start = 15, length = 4)

Is expr ever null?

It shouldn't be. I've added an assert to declare the precondition.

- Reduce scope of local - Remove null check ( + add debug assert)

cston · 2018-11-07T23:48:53Z

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

@@ -281,6 +281,64 @@ static void Main()
            CreateCompilation(source).VerifyDiagnostics();
        }

+        [Fact]
+        public void UsingPatternLessDerivedAssignement()


Assignement [](start = 43, length = 11)

Typo

cston · 2018-11-07T23:51:18Z

public void Dispose() { }

Consider testing Dispose<T>().

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/UsingStatementTests.cs:99 in dac5bb9. [](commit_id = dac5bb9, deletion_comment = False)

chsienki · 2018-11-08T20:33:00Z

@dotnet-bot retest ubuntu_16_mono_debug_prtest please

chsienki · 2018-11-08T20:33:40Z

@dotnet-bot retest windows_debug_vs-integration_prtest please

* Refactor pattern lookup logic: - Use BindMethodGroupInvocation to perform lookup, rather than manually iterating with LookupMembersInType - Share the pattern logic with GetPinnableReference lookup - Update existing tests to match new logic - Add new tests

chsienki added 2 commits October 10, 2018 15:08

Use well known deconstruct name rather than string in deconstruct search

99289c4

Refactor pattern lookup logic:

693cb94

- Use BindMethodGroupInvocation to perform lookup, rather than manually iterating with LookupMembersInType - Share the pattern logic with GetPinnableReference lookup - Update tests to match new logic

chsienki added Area-Compilers New Language Feature - enhanced using Using pattern and declaration labels Oct 10, 2018

chsienki requested a review from a team as a code owner October 10, 2018 22:21

chsienki added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Oct 10, 2018

Simplify pattern binding:

8755cf0

- Fail refextensionmethod tests + add prototype comment noting it doesn't work.

jcouv reviewed Oct 12, 2018

View reviewed changes

jcouv self-assigned this Oct 12, 2018

jcouv reviewed Oct 12, 2018

View reviewed changes

chsienki added 2 commits October 15, 2018 13:26

Address PR feedback

9751168

Edit doc text

9f7fd70

cston reviewed Oct 15, 2018

View reviewed changes

cston reviewed Oct 16, 2018

View reviewed changes

jcouv removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Oct 16, 2018

chsienki added 2 commits October 31, 2018 14:39

Bind dispose on the expression type, not the initializer:

c39832b

- Make a bound local to be the receiver - Add tests for ref/in extension methods - Add tests for implict conversions

PR Feedback:

0268259

- Add a static pattern dispose test - Fix a typo

RikkiGibson reviewed Nov 5, 2018

View reviewed changes

RikkiGibson approved these changes Nov 5, 2018

View reviewed changes

cston reviewed Nov 5, 2018

View reviewed changes

PR Feedback:

dac5bb9

- Reduce scope of local - Remove null check ( + add debug assert)

cston reviewed Nov 7, 2018

View reviewed changes

cston approved these changes Nov 7, 2018

View reviewed changes

Add extra tests

ac0ec50

chsienki merged this pull request into dotnet:features/enhanced-using Nov 8, 2018

		out var disposeMethod);


		if (disposeMethod?.ReturnsVoid == false \|\| result == PatternLookupResult.NotAMethod)

		@@ -3301,110 +3219,89 @@ internal virtual ImmutableArray<LabelSymbol> Labels
		/// Perform a lookup for the specified method on the specified type, searching further if the first resolved symbol doesn't match

Dispose pattern refactor #30437

Dispose pattern refactor #30437

Conversation

chsienki commented Oct 10, 2018

chsienki commented Oct 12, 2018

chsienki commented Oct 12, 2018

jcouv Oct 12, 2018 • edited

Choose a reason for hiding this comment

jcouv commented Oct 12, 2018

jcouv Oct 12, 2018 • edited

Choose a reason for hiding this comment

jcouv Oct 12, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Oct 12, 2018 • edited

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

jcouv Oct 12, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Oct 12, 2018 • edited

Choose a reason for hiding this comment

jcouv Oct 12, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Oct 12, 2018 • edited

Choose a reason for hiding this comment

jcouv Oct 12, 2018 • edited

Choose a reason for hiding this comment

jcouv Oct 12, 2018 • edited

Choose a reason for hiding this comment

jcouv Oct 12, 2018 • edited

Choose a reason for hiding this comment

chsienki Oct 15, 2018 • edited by jcouv

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv left a 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

jcouv commented Oct 16, 2018 • edited

chsienki commented Nov 1, 2018

chsienki commented Nov 2, 2018

chsienki commented Nov 5, 2018

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

Choose a reason for hiding this comment

cston commented Nov 7, 2018

chsienki commented Nov 8, 2018

chsienki commented Nov 8, 2018

jcouv Oct 12, 2018 •

edited

jcouv Oct 12, 2018 •

edited

jcouv Oct 12, 2018 •

edited

jcouv Oct 12, 2018 •

edited

jcouv Oct 12, 2018 •

edited

jcouv Oct 12, 2018 •

edited

jcouv Oct 12, 2018 •

edited

jcouv Oct 12, 2018 •

edited

jcouv Oct 12, 2018 •

edited

jcouv Oct 12, 2018 •

edited

jcouv Oct 12, 2018 •

edited

chsienki Oct 15, 2018 •

edited by jcouv

jcouv commented Oct 16, 2018 •

edited