Pattern Using #27729

fayrose · 2018-06-11T20:22:07Z

Description

A user has a class method Dispose() but has not or does not want to implement the IDisposable interface. This allows the user to simply implement a public void-returning instance method called Dispose that bypasses the official interface check and thus allows this type to be utilized in a using statement.

An outline of a spec is in dotnet/csharplang#1623

…tance method

agocke · 2018-06-11T20:28:13Z

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

+            MessageID messageID;
+            switch (methodName)
+            {
+                case WellKnownMemberNames.GetDisposeMethodName:


I think this should just be called DisposeMethodName since the pattern seems to be to use the exact name of the method.

agocke · 2018-06-11T20:34:04Z

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

+                    }
+                    result = null;
+                }
+                else if (messageID == MessageID.IDS_Disposable && !result.ReturnsVoid && warningsOnly)


How is this if statement different from the above? It seems like more of this code could be shared and it would prevent us fram having to switch on the messageID, which is a bit of a code smell. Seems like either this code is similar enough that we could get more sharing, or it's different enough that we might as well give up on sharing move this check elsewhere.

Will move these to the individual Satisfies___Pattern methods in UsingStatementBinder.cs and ForEachLoopBinder.cs

agocke · 2018-06-11T20:35:05Z

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

+                {
+                    lookupResult.Clear();
+
+                HashSet<DiagnosticInfo> useSiteDiagnostics = null;


Looks like indentation got screwed up here.

agocke · 2018-06-11T20:52:21Z

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

@@ -82,7 +81,28 @@ internal override BoundStatement BindUsingStatementParts(DiagnosticBag diagnosti
                if (!iDisposableConversion.IsImplicit)
                {
                    TypeSymbol expressionType = expressionOpt.Type;
-                    if ((object)expressionType == null || !expressionType.IsErrorType())
+                    MethodSymbol disposeMethod = expressionType == null ? null : SatisfiesDisposePattern(expressionOpt.Type, diagnostics);


Seems like this code should be outside the iDisposable.IsImplicit check. That is, we should do the pattern check before the conversion is classified, in case we don't need to do a conversion at all.

Doesn't look like this has been addressed.

agocke · 2018-06-11T20:55:26Z

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

+        /// <param name="exprType">Type of the expression over which to iterate</param>
+        /// <param name="diagnostics">Populated with warnings if there are near misses</param>
+        /// <returns>True if a matching method is found (still need to verify return type).</returns>
+        private MethodSymbol SatisfiesDisposePattern(TypeSymbol exprType, DiagnosticBag diagnostics)


I think this method should take the method from FindPatternMethod as a parameter and return a bool to indicate if it matches. Alternatively, it should probably be renamed TryFindDisposeMethod.

Can I change the return type to a bool and add an out parameter for DisposeMethod?

agocke · 2018-06-11T20:56:25Z

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

+                    MethodSymbol disposeMethod = expressionType == null ? null : SatisfiesDisposePattern(expressionOpt.Type, diagnostics);
+                    if ((object)disposeMethod != null)
+                    {
+                        if (!disposeMethod.ReturnsVoid)


This seems like a check for SatisfiesDisposePattern. If the signature didn't match, how did it satisfy the check?

agocke · 2018-06-11T20:57:40Z

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

@@ -132,9 +175,27 @@ internal override BoundStatement BindUsingStatementParts(DiagnosticBag diagnosti
                expressionOpt,
                iDisposableConversion,
                boundBody,
+                null, // This ensures that a pattern-matched Dispose statement is not used.


Use a named argument, like methodOpt: null, for boolean literals and null.

agocke · 2018-06-11T20:58:02Z

src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml

@@ -934,6 +934,7 @@
    <Field Name="ExpressionOpt" Type="BoundExpression" Null="allow"/>
    <Field Name="IDisposableConversion" Type="Conversion" />
    <Field Name="Body" Type="BoundStatement"/>
+    <Field Name="MethodOpt" Type="MethodSymbol" Null="allow"/>


Maybe DisposeMethodOpt?

agocke · 2018-06-11T20:58:51Z

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

@@ -292,7 +292,11 @@ private BoundStatement RewriteUsingStatementTryFinally(SyntaxNode syntax, BoundB
            BoundExpression disposeCall;

            MethodSymbol disposeMethodSymbol;
-            if (Binder.TryGetSpecialTypeMember(_compilation, SpecialMember.System_IDisposable__Dispose, syntax, _diagnostics, out disposeMethodSymbol))
+            if (methodOpt != null)


Cast to object or use !(methodOpt is null)

agocke · 2018-06-11T21:02:45Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenUsingStatementTests.cs

+    IL_0008:  ldloc.0
+    IL_0009:  brfalse.s  IL_0011
+    IL_000b:  ldloc.0
+    IL_000c:  callvirt   ""void C1.Dispose()""


This seems weird to me. I could maybe understand a struct constrained callvirt, but this is a regular class. I don't understand why this isn't a call. Can you look at EmitCallExpression in the debugger for this test and see why it's choosing to use CallKind.CallVirt instead of CallKind.Call?

Actually, I think I understand this. I bet that we don't exactly know that the local is a temp for the object creation expression, so we're doing a null test. It would be good to confirm that CanUseCallOnRefTypeReceiver is returning false here.

I'd spoken briefly with Jared about this earlier, who said that callvirt was the correct option. A bit confused as to which I should stick with.

Yeah, if there's any work to do here it's probably not part of this feature. I think we could get cleverer with some codegen, but let's not do that now.

fayrose · 2018-06-12T19:22:17Z

test windows_debug_unit32_prtest please

agocke · 2018-06-12T19:30:46Z

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

                iDisposableConversion = originalBinder.Conversions.ClassifyImplicitConversionFromExpression(expressionOpt, iDisposable, ref useSiteDiagnostics);
                diagnostics.Add(expressionSyntax, useSiteDiagnostics);

                if (!iDisposableConversion.IsImplicit)
                {
-                    TypeSymbol expressionType = expressionOpt.Type;
-                    if ((object)expressionType == null || !expressionType.IsErrorType())
+                    if ((object)disposeMethod != null)


This entire if statement can be outside the top-level if. We don't need to do the conversion if we find a successful pattern.

agocke · 2018-06-12T19:31:16Z

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

-                    if ((object)expressionType == null || !expressionType.IsErrorType())
+                    if ((object)disposeMethod != null)
+                    {
+                        if (!disposeMethod.ReturnsVoid)


This looks like it's checked in TryDisposePattern. This is probably dead code.

agocke · 2018-06-12T19:31:44Z

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

@@ -76,13 +75,34 @@ internal override BoundStatement BindUsingStatementParts(DiagnosticBag diagnosti
                expressionOpt = this.BindTargetExpression(diagnostics, originalBinder);

                HashSet<DiagnosticInfo> useSiteDiagnostics = null;
+                TypeSymbol expressionType = expressionOpt.Type;
+                MethodSymbol disposeMethod = expressionType == null ? null : TryFindDisposePattern(expressionOpt.Type, diagnostics);


I would call this TryGetDisposePatternMethod since we're actually trying to get a specific method, not a pattern.

agocke · 2018-06-12T19:35:45Z

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

                if (declType.IsDynamic())
                {
                    iDisposableConversion = Conversion.ImplicitDynamic;
                }
+                else if ((object)disposeMethod != null)
+                {
+                    BoundStatement boundBody2 = originalBinder.BindPossibleEmbeddedStatement(_syntax.Statement, diagnostics);


This code, the code above, and the last return look identical. This could all probably be shared by moving the disposeMethod variable to the top of the method and flowing the variable to the final return.

agocke · 2018-06-12T19:39:26Z

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

+        {
+            LookupResult lookupResult = LookupResult.GetInstance();
+            SyntaxNode exp = _syntax.Expression != null ? (SyntaxNode) _syntax.Expression : (SyntaxNode) _syntax.Declaration;
+            MethodSymbol disposeMethod = FindPatternMethod(exprType, WellKnownMemberNames.DisposeMethodName, lookupResult, exp, warningsOnly: true, diagnostics: diagnostics, _syntax.SyntaxTree);


diagnostics: diagnostics is a redundant named variable.

agocke · 2018-06-12T19:40:38Z

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

+        private MethodSymbol TryFindDisposePattern(TypeSymbol exprType, DiagnosticBag diagnostics)
+        {
+            LookupResult lookupResult = LookupResult.GetInstance();
+            SyntaxNode exp = _syntax.Expression != null ? (SyntaxNode) _syntax.Expression : (SyntaxNode) _syntax.Declaration;


I would just call this syntax. exp implies it is an expression, but a declaration is not an expression.

agocke · 2018-06-12T19:43:47Z

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

+            {
+                disposeCall = BoundCall.Synthesized(syntax, disposedExpression, methodOpt);
+            }
+            else if (Binder.TryGetSpecialTypeMember(_compilation, SpecialMember.System_IDisposable__Dispose, syntax, _diagnostics, out disposeMethodSymbol))


You could rewrite this in one if as follows:

if (!(methodOpt is null)) || Binder.TryGetSpecialTypeMember(_compilation, SpecialMember.System_IDisposable__Dispose, syntax, _diagnostics, out methodOpt))

agocke · 2018-06-12T19:54:10Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenUsingStatementTests.cs

+    IL_0008:  ldloc.0
+    IL_0009:  brfalse.s  IL_0011
+    IL_000b:  ldloc.0
+    IL_000c:  callvirt   ""void C1.Dispose()""


Yeah, if there's any work to do here it's probably not part of this feature. I think we could get cleverer with some codegen, but let's not do that now.

agocke · 2018-06-12T19:57:40Z

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

                Diagnostic(ErrorCode.ERR_NoConvToIDisp, "Main").WithArguments("method group"));
        }

+        [Fact]
+        public void UsingPatternTest()


It looks like there may be a duplicate test above that used to be failing but is now succeeding. You could probably remove a few identical tests.

agocke · 2018-06-12T19:58:50Z

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

+                Diagnostic(ErrorCode.ERR_NoConvToIDisp, "C1 c = new C1()").WithArguments("C1").WithLocation(12, 16)
+            );
+        }
+


All these tests seem to test the declaration form, but we should also test the expression form, i.e. using (expr). You could probably test both forms in the same test, just use two using blocks.

gafter · 2018-06-12T21:27:04Z

src/Compilers/CSharp/Portable/CSharpResources.resx

@@ -2939,7 +2942,7 @@ A catch() block after a catch (System.Exception e) block can catch non-CLS excep
    <value>Anonymous methods, lambda expressions, and query expressions inside structs cannot access instance members of 'this'. Consider copying 'this' to a local variable outside the anonymous method, lambda expression or query expression and using the local instead.</value>
  </data>
  <data name="ERR_NoConvToIDisp" xml:space="preserve">
-    <value>'{0}': type used in a using statement must be implicitly convertible to 'System.IDisposable'</value>
+    <value>'{0}': type used in a using statement must have a public void-returning Dispose() instance method.</value>
  </data>


This is not a requirement. The type could implement IDisposable using explicit interface implementation, in which case it would not have a public method like that. Perhaps

<value>'{0}': type used in a using statement must be implicitly convertible to 'System.IDisposable' or have a public void-returning Dispose() instance method.</value>

#Resolved

gafter · 2018-06-12T21:32:39Z

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

+            switch (methodName)
+            {
+                case WellKnownMemberNames.DisposeMethodName:
+                    messageID = MessageID.IDS_Disposable;


IDS_Disposable [](start = 42, length = 14)

Would it make sense for the MessageID to be passed in? (just asking) #Resolved

gafter · 2018-06-12T21:35:22Z

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

+                    {
+                        ReportPatternWarning(diagnostics, patternType, member, syntaxExpr, messageID);
+                    }
+                    return null;


return [](start = 20, length = 6)

Nit: insert blank line before this, please. #Resolved

gafter · 2018-06-12T21:35:37Z

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

+                    candidateMethods.Add((MethodSymbol)member);
+                }
+            }
+            MethodSymbol patternMethod = PerformPatternOverloadResolution(patternType, candidateMethods, syntaxExpr, warningsOnly, diagnostics, syntaxTree, messageID);


MethodSymbol [](start = 12, length = 12)

Nit: insert blank line before this, please. #Resolved

gafter · 2018-06-12T21:35:57Z

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

+        /// The overload resolution portion of FindForEachPatternMethod.
+        /// </summary>
+        private MethodSymbol PerformPatternOverloadResolution
+            (TypeSymbol patternType, ArrayBuilder<MethodSymbol> candidateMethods,


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

Please put the open paren at the end of the previous line. #Resolved

gafter · 2018-06-12T21:37:12Z

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

+                    {
+                        diagnostics.Add(ErrorCode.WRN_PatternStaticOrInaccessible, syntaxExpression.Location, patternType, messageID.Localize(), result);
+                    }
+                    result = null;


result [](start = 20, length = 6)

Nit: empty line before this, please. #Resolved

agocke · 2018-06-12T22:51:06Z

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

+                        if ((object)expressionType == null || !expressionType.IsErrorType())
+                        {
+                            Error(diagnostics, ErrorCode.ERR_NoConvToIDisp, expressionSyntax, expressionOpt.Display);
+                        } else


The else here looks wrong. hasErrors should always be set to true. The if (disposeMethod is null) should be enough to ensure we're in an error case.

agocke · 2018-06-12T22:52:36Z

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

-                    hasErrors = true;
+                    if (disposeMethod is null)
+                    {
+                        if ((object)expressionType == null || !expressionType.IsErrorType())


We should be consistent and use expressionType is null here.

agocke · 2018-06-12T22:54:52Z

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

@@ -110,14 +121,20 @@ internal override BoundStatement BindUsingStatementParts(DiagnosticBag diagnosti
                    iDisposableConversion = originalBinder.Conversions.ClassifyImplicitConversionFromType(declType, iDisposable, ref useSiteDiagnostics);
                    diagnostics.Add(declarationSyntax, useSiteDiagnostics);

-                    if (!iDisposableConversion.IsImplicit)
+                    if (!iDisposableConversion.IsImplicit && (object)disposeMethod is null)


If you use is the (object) cast is unnecessary.

agocke · 2018-06-12T22:55:23Z

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

+                            {
+                                Error(diagnostics, ErrorCode.ERR_NoConvToIDisp, declarationSyntax, declType);
+                            }
+                            else


Else is also wrong here.

agocke · 2018-06-12T22:56:17Z

src/Compilers/CSharp/Test/Emit/PDB/PDBTests.cs

@@ -5404,7 +5404,7 @@ .maxstack  1
    IL_001c:  ldloc.2
    IL_001d:  brfalse.s  IL_0026
    IL_001f:  ldloc.2
-    IL_0020:  callvirt   ""void System.IDisposable.Dispose()""
+    IL_0020:  callvirt   ""void System.IO.Stream.Dispose()""


I expect all of the old tests to now have no changes.

fayrose · 2018-06-13T17:01:39Z

test this please

agocke · 2018-06-13T20:04:24Z

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

+        }
+
+        /// <summary>
+        /// The overload resolution portion of FindForEachPatternMethod.


I think this is called FindPatternMethod now.

agocke · 2018-06-13T20:05:18Z

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

@@ -766,141 +767,14 @@ private ForEachEnumeratorInfo.Builder GetDefaultEnumeratorInfo(ForEachEnumerator
        private bool SatisfiesGetEnumeratorPattern(ref ForEachEnumeratorInfo.Builder builder, TypeSymbol collectionExprType, DiagnosticBag diagnostics)
        {
            LookupResult lookupResult = LookupResult.GetInstance();
-            MethodSymbol getEnumeratorMethod = FindForEachPatternMethod(collectionExprType, GetEnumeratorMethodName, lookupResult, warningsOnly: true, diagnostics: diagnostics);
+            MethodSymbol getEnumeratorMethod = FindPatternMethod(collectionExprType, GetEnumeratorMethodName, lookupResult,
+                                                                _syntax.Expression, warningsOnly: true, diagnostics: diagnostics,


Looks like the indentation is one space off.

agocke · 2018-06-13T20:09:57Z

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

-                if (!iDisposableConversion.IsImplicit)
+                TypeSymbol expressionType = expressionOpt.Type;
+
+                if (!iDisposableConversion.IsImplicit && disposeMethod is null)


How could disposeMethod be non-null here?

agocke · 2018-06-13T20:18:03Z

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

@@ -110,14 +119,17 @@ internal override BoundStatement BindUsingStatementParts(DiagnosticBag diagnosti
                    iDisposableConversion = originalBinder.Conversions.ClassifyImplicitConversionFromType(declType, iDisposable, ref useSiteDiagnostics);
                    diagnostics.Add(declarationSyntax, useSiteDiagnostics);

-                    if (!iDisposableConversion.IsImplicit)
+                    if (!iDisposableConversion.IsImplicit && disposeMethod is null)


I also don't see how disposeMethod could be null here.

agocke · 2018-06-13T20:20:09Z

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

                hasErrors);
        }

+        /// <summary>
+        /// Checks for a Dispose method on exprType. Failing to satisfy the pattern is not an error -
+        /// it just means we have to check for an interface instead.


I think this comment is wrong now since we check for an interface before looking for the pattern. I don't think that this code should add any errors, though. The error reporting in the other methods makes sense.

agocke · 2018-06-13T20:21:09Z

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

+        /// </summary>
+        /// <param name="exprType">Type of the expression over which to iterate</param>
+        /// <param name="diagnostics">Populated with warnings if there are near misses</param>
+        /// <returns>True if a matching method is found (still need to verify return type).</returns>


It looks like this method verifies the return type.

agocke · 2018-06-13T20:23:05Z

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

+            LookupResult lookupResult = LookupResult.GetInstance();
+            SyntaxNode syntax = _syntax.Expression != null ? (SyntaxNode)_syntax.Expression : (SyntaxNode)_syntax.Declaration;
+            MethodSymbol disposeMethod = FindPatternMethod(exprType, WellKnownMemberNames.DisposeMethodName, lookupResult, syntax, warningsOnly: true, diagnostics, _syntax.SyntaxTree, MessageID.IDS_Disposable);
+            lookupResult.Free();


This lookupResult logic is crazy complicated (not your fault and not something I would necessarily fix in this PR).

agocke · 2018-06-13T20:33:19Z

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

-                // (8,16): error CS1674: 'Program.S1': type used in a using statement must be implicitly convertible to 'System.IDisposable'
-                //         using (new S1())
-                Diagnostic(ErrorCode.ERR_NoConvToIDisp, "new S1()").WithArguments("Program.S1").WithLocation(8, 16)
+                Diagnostic(ErrorCode.ERR_RefStructInterfaceImpl, "IDisposable").WithArguments("Program.S1", "System.IDisposable").WithLocation(14, 28)


Probably worth adding a test below this one which is the same except S1 doesn't implement IDisposable. It should work fine now. 😄

agocke

LGTM

gafter · 2018-06-18T18:48:29Z

Is there an LDM champion for this feature? Link to the championed feature issue please.
Is there a draft specification for this feature? Link to the draft spec please.
What is the tentative proposed language milestone for it?

jaredpar · 2018-06-18T20:47:38Z

@gafter

Is there an LDM champion for this feature? Link to the championed feature issue please.

I will be championing the feature. But at the same time I don't think we need to block this PR on having a champion as it exists on a feature branch, not any shipping branch.

What is the tentative proposed language milestone for it?

C# 8.0 will be my proposed release but again though not sure we need to discuss that for the purpose of this PR. Probably best to discuss that on the csharplang repository.

gafter · 2018-06-18T20:51:36Z

I will be championing the feature.

OK, then I'll drop by with language questions for this feature.

gafter · 2018-06-18T21:49:08Z

Need a link in the PR description to whatever is being used as a spec. Is it dotnet/csharplang#1623 ?

gafter · 2018-06-19T00:32:35Z

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

-                // (8,16): error CS1674: 'Program.S1': type used in a using statement must be implicitly convertible to 'System.IDisposable'
-                //         using (new S1())
-                Diagnostic(ErrorCode.ERR_NoConvToIDisp, "new S1()").WithArguments("Program.S1").WithLocation(8, 16)
+                Diagnostic(ErrorCode.ERR_RefStructInterfaceImpl, "IDisposable").WithArguments("Program.S1", "System.IDisposable").WithLocation(14, 28)


ERR_RefStructInterfaceImpl [](start = 37, length = 26)

The comment does not align with the error message here.

gafter · 2018-06-19T00:33:29Z

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

+                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.
+                //         using (C1 c = new C1())
+                Diagnostic(ErrorCode.ERR_NoConvToIDisp, "C1 c = new C1()").WithArguments("C1").WithLocation(13, 16)


ErrorCode [](start = 27, length = 9)

Can we report only one error for this instead of two?

gafter

LGTM assuming the spec is dotnet/csharplang#1623. Before integrating, please open an issue about the cascaded diagnostic in UsingPatternSameSignatureAmbiguousTest and correct the comment on a test where noted in SpanStackSafetyTests.cs test InterfaceImpl.

jcouv · 2018-06-27T20:34:37Z

@fayrose I marked the PR as "for personal review" since it is not actively being reviewed. Please remove the label once you've added the tests. Thanks

jaredpar · 2018-06-27T20:36:17Z

@jcouv spoke with @fayrose earlier today. The POR is to get this PR merged and follow up with another PR for the additional tests I requested.

jcouv · 2018-06-27T20:48:13Z

Sounds good. Ready to merge then?

Lauren Rosenberg added 7 commits June 7, 2018 10:32

Initial commit, own tests passing

7f96f70

Fix pre-existing tests

2c95c13

Abstracted MessageID and fixed tests

377cd2c

Pre-existing tests updated

e915f2f

Expanded testing to cover ambiguous, inherited and extension cases.

adf6e69

Update error message to reflect that Dispose must be a non-static ins…

5a98feb

…tance method

Removed extraneous spacing

9554f56

fayrose added the Area-Compilers label Jun 11, 2018

fayrose requested a review from a team as a code owner June 11, 2018 20:22

fayrose requested a review from agocke June 11, 2018 20:22

agocke reviewed Jun 11, 2018

View reviewed changes

Lauren Rosenberg added 6 commits June 11, 2018 14:28

Minor fixes

bcf2cb7

Moved return type checks to own classes

92401fa

Rename f(x)

936e16c

Finished rename of methodOpt to DisposeMethodOpt

609f674

Tiny fix

dfff3fe

Fixed 1 failing test

208e986

gafter self-requested a review June 12, 2018 00:17

agocke reviewed Jun 12, 2018

View reviewed changes

Lauren Rosenberg added 3 commits June 12, 2018 13:20

Method refactor

2ce5a67

Minor refactoring

02aa307

Larger refactor

ce10d18

gafter reviewed Jun 12, 2018

View reviewed changes

Prioritizes iDisposable and fixes test cases

4912594

agocke reviewed Jun 12, 2018

View reviewed changes

Lauren Rosenberg added 2 commits June 12, 2018 16:01

Fixed casting and else clause

83bdbab

Reverted old tests

20e472e

Reverted emit tests

0f0ec15

agocke reviewed Jun 13, 2018

View reviewed changes

Requested changes

433ed8c

agocke approved these changes Jun 14, 2018

View reviewed changes

gafter reviewed Jun 19, 2018

View reviewed changes

gafter approved these changes Jun 19, 2018

View reviewed changes

agocke requested review from jaredpar and a team June 20, 2018 20:18

agocke mentioned this pull request Jun 25, 2018

More tests for pattern using #28102

Closed

jcouv added this to the 16.0 milestone Jun 27, 2018

jcouv added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Jun 27, 2018

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

fayrose merged commit 336e138 into dotnet:features/enhanced-using Jun 28, 2018

fayrose deleted the pattern-using branch June 28, 2018 20:08

fayrose restored the pattern-using branch June 28, 2018 20:08

Pattern Using #27729

Pattern Using #27729

Conversation

fayrose commented Jun 11, 2018 • edited by gafter Loading

Description

Choose a reason for hiding this comment

agocke Jun 11, 2018 • 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fayrose commented Jun 12, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gafter Jun 12, 2018 • edited Loading

Choose a reason for hiding this comment

gafter Jun 12, 2018 • edited Loading

Choose a reason for hiding this comment

gafter Jun 12, 2018 • edited Loading

Choose a reason for hiding this comment

gafter Jun 12, 2018 • edited Loading

Choose a reason for hiding this comment

gafter Jun 12, 2018 • edited Loading

Choose a reason for hiding this comment

gafter Jun 12, 2018 • 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

fayrose commented Jun 13, 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

Choose a reason for hiding this comment

agocke left a comment

Choose a reason for hiding this comment

gafter commented Jun 18, 2018

jaredpar commented Jun 18, 2018

gafter commented Jun 18, 2018

gafter commented Jun 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gafter left a comment

Choose a reason for hiding this comment

jcouv commented Jun 27, 2018

jaredpar commented Jun 27, 2018

jcouv commented Jun 27, 2018

fayrose commented Jun 11, 2018 •

edited by gafter

Loading

agocke Jun 11, 2018 •

edited

Loading

gafter Jun 12, 2018 •

edited

Loading

gafter Jun 12, 2018 •

edited

Loading

gafter Jun 12, 2018 •

edited

Loading

gafter Jun 12, 2018 •

edited

Loading

gafter Jun 12, 2018 •

edited

Loading

gafter Jun 12, 2018 •

edited

Loading