Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Async-streams: allow pattern-based disposal in await using and foreach #32731

Merged
merged 6 commits into from
Jan 25, 2019
Merged

Async-streams: allow pattern-based disposal in await using and foreach #32731

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d772212
Async-streams: allow pattern-based disposal in await using and foreach
jcouv Jan 23, 2019
e3622a4
Discard found extension methods
jcouv Jan 25, 2019
5945e59
Address PR feedback
jcouv Jan 25, 2019
1e16d7e
Address feedback from Chuck
jcouv Jan 25, 2019
898aa41
More tests
jcouv Jan 25, 2019
e2d996c
Tweak tests
jcouv Jan 25, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions docs/features/async-streams.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,15 +64,19 @@ Just like in iterator methods, there are several restrictions on where a yield s

An asynchronous `using` is lowered just like a regular `using`, except that `Dispose()` is replaced with `await DisposeAsync()`.

Note that pattern-based lookup for `DisposeAsync` binds to instance methods that can be invoked without arguments.
Extension methods do not contribute. The result of `DisposeAsync` must be awaitable.

### Detailed design for `await foreach` statement

An `await foreach` is lowered just like a regular `foreach`, except that:
- `GetEnumerator()` is replaced with `await GetAsyncEnumerator()`
- `MoveNext()` is replaced with `await MoveNextAsync()`
- `Dispose()` is replaced with `await DisposeAsync()`

Note that pattern-based lookup for `GetAsyncEnumerator` and `MoveNextAsync` do not place particular requirements on those methods,
as long as they could be invoked without arguments.
Note that pattern-based lookup for `GetAsyncEnumerator`, `MoveNextAsync` and `DisposeAsync` binds to instance methods that can be invoked without arguments.
Extension methods do not contribute. The result of `MoveNextAsync` and `DisposeAsync` must be awaitable.
Disposal for `await foreach` does not include a fallback to a runtime check for an interface implementation.

Asynchronous foreach loops are disallowed on collections of type dynamic,
as there is no asynchronous equivalent of the non-generic `IEnumerable` interface.
Expand Down
2 changes: 2 additions & 0 deletions src/Compilers/CSharp/Portable/Binder/AwaitableInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ namespace Microsoft.CodeAnalysis.CSharp
/// </summary>
internal sealed class AwaitableInfo
{
public static readonly AwaitableInfo Empty = new AwaitableInfo(getAwaiterMethod: null, isCompletedProperty: null, getResultMethod: null);

public readonly MethodSymbol GetAwaiter;
public readonly PropertySymbol IsCompleted;
public readonly MethodSymbol GetResult;
Expand Down
13 changes: 10 additions & 3 deletions src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -677,7 +677,7 @@ internal MethodSymbol TryFindDisposePatternMethod(BoundExpression expr, SyntaxNo
{
Debug.Assert(!(expr is null));
Debug.Assert(!(expr.Type is null));
Debug.Assert(expr.Type.IsValueType && expr.Type.IsRefLikeType); // pattern dispose lookup is only valid on ref structs
Debug.Assert((expr.Type.IsValueType && expr.Type.IsRefLikeType) || hasAwait); // pattern dispose lookup is only valid on ref structs or asynchronous usings
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we say IsValueType && IsRefLikeType? When can IsRefLikeType be true but IsValueType is false?


// Don't try and lookup if we're not enabled
if (MessageID.IDS_FeatureUsingDeclarations.RequiredVersion() > Compilation.LanguageVersion)
Expand All @@ -691,8 +691,15 @@ internal MethodSymbol TryFindDisposePatternMethod(BoundExpression expr, SyntaxNo
diagnostics,
out var disposeMethod);

if ((!hasAwait && disposeMethod?.ReturnsVoid == false)
|| (hasAwait && disposeMethod?.ReturnType.TypeSymbol.IsNonGenericTaskType(Compilation) == false)
if (disposeMethod?.IsExtensionMethod == true)
{
// Extension methods should just be ignored, rather than rejected after-the-fact
// Tracked by https://github.com/dotnet/roslyn/issues/32767

// extension methods do not contribute to pattern-based disposal
disposeMethod = null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The disposeMethod is discarded here but what about any diagnostics that resulted from calling PerformPatternMethodLookup? Won't those still be in diagnostics and hence passed back up to the caller?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the using scenarios, we keep those diagnostics and add another error (didn't find a proper way of disposing). See UsingPatternScopedExtensionMethodTest, which produces some unnecessary diagnostics. I'm thinking to leave that as-is for now, with tracking issue.

In the foreach scenarios, the caller already uses a separate diagnostic bag. I'll add tests to demonstrate.

}
else if ((!hasAwait && disposeMethod?.ReturnsVoid == false)
|| result == PatternLookupResult.NotAMethod)
{
HashSet<DiagnosticInfo> useSiteDiagnostics = null;
Expand Down
4 changes: 2 additions & 2 deletions src/Compilers/CSharp/Portable/Binder/ForEachEnumeratorInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ private ForEachEnumeratorInfo(
MethodSymbol getEnumeratorMethod,
MethodSymbol currentPropertyGetter,
MethodSymbol moveNextMethod,
bool needsDisposeMethod,
bool needsDisposal,
AwaitableInfo disposeAwaitableInfo,
MethodSymbol disposeMethod,
Conversion collectionConversion,
Expand All @@ -68,7 +68,7 @@ private ForEachEnumeratorInfo(
this.GetEnumeratorMethod = getEnumeratorMethod;
this.CurrentPropertyGetter = currentPropertyGetter;
this.MoveNextMethod = moveNextMethod;
this.NeedsDisposal = needsDisposeMethod;
this.NeedsDisposal = needsDisposal;
this.DisposeAwaitableInfo = disposeAwaitableInfo;
this.DisposeMethod = disposeMethod;
this.CollectionConversion = collectionConversion;
Expand Down
14 changes: 9 additions & 5 deletions src/Compilers/CSharp/Portable/Binder/ForEachLoopBinder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -503,8 +503,11 @@ private BoundForEachStatement BindForEachPartsWorker(DiagnosticBag diagnostics,

private bool GetAwaitDisposeAsyncInfo(ref ForEachEnumeratorInfo.Builder builder, DiagnosticBag diagnostics)
{
BoundExpression placeholder = new BoundAwaitableValuePlaceholder(_syntax.Expression,
this.GetWellKnownType(WellKnownType.System_Threading_Tasks_ValueTask, diagnostics, this._syntax));
var awaitableType = builder.DisposeMethod is null
? this.GetWellKnownType(WellKnownType.System_Threading_Tasks_ValueTask, diagnostics, this._syntax)
: builder.DisposeMethod.ReturnType.TypeSymbol;

BoundExpression placeholder = new BoundAwaitableValuePlaceholder(_syntax.Expression, awaitableType);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Why does the local change the type here of the new expression? It's not passed by ref hence couldn't see an obvious reason for this


bool hasErrors = false;
builder.DisposeAwaitableInfo = BindAwaitInfo(placeholder, _syntax.Expression, _syntax.AwaitKeyword.GetLocation(), diagnostics, ref hasErrors);
Expand Down Expand Up @@ -836,16 +839,17 @@ private void GetDisposalInfoForEnumerator(ref ForEachEnumeratorInfo.Builder buil
TypeSymbol enumeratorType = builder.GetEnumeratorMethod.ReturnType.TypeSymbol;
HashSet<DiagnosticInfo> useSiteDiagnostics = null;

if (!enumeratorType.IsSealed ||
// For async foreach, we don't do the runtime check
if ((!enumeratorType.IsSealed && !isAsync) ||
this.Conversions.ClassifyImplicitConversionFromType(enumeratorType,
isAsync ? this.Compilation.GetWellKnownType(WellKnownType.System_IAsyncDisposable) : this.Compilation.GetSpecialType(SpecialType.System_IDisposable),
ref useSiteDiagnostics).IsImplicit)
{
builder.NeedsDisposal = true;
}
else if (enumeratorType.IsRefLikeType)
else if (enumeratorType.IsRefLikeType || isAsync)
{
// if it wasn't directly convertable to IDisposable, see if it is pattern disposable
// if it wasn't directly convertable to IDisposable, see if it is pattern-disposable
// again, we throw away any binding diagnostics, and assume it's not disposable if we encounter errors
var patternDisposeDiags = new DiagnosticBag();
var receiver = new BoundDisposableValuePlaceholder(_syntax, enumeratorType);
Expand Down
30 changes: 23 additions & 7 deletions src/Compilers/CSharp/Portable/Binder/UsingStatementBinder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ internal static BoundStatement BindUsingStatementOrDeclarationFromParts(SyntaxNo
AwaitableInfo awaitOpt = null;
TypeSymbol declarationTypeOpt = null;
MethodSymbol disposeMethodOpt = null;
TypeSymbol awaitableTypeOpt = null;

if (isExpression)
{
Expand Down Expand Up @@ -117,11 +118,16 @@ internal static BoundStatement BindUsingStatementOrDeclarationFromParts(SyntaxNo

if (hasAwait)
{
TypeSymbol taskType = originalBinder.Compilation.GetWellKnownType(WellKnownType.System_Threading_Tasks_ValueTask);
hasErrors |= ReportUseSiteDiagnostics(taskType, diagnostics, awaitKeyword);

BoundExpression placeholder = new BoundAwaitableValuePlaceholder(syntax, taskType).MakeCompilerGenerated();
awaitOpt = originalBinder.BindAwaitInfo(placeholder, syntax, awaitKeyword.GetLocation(), diagnostics, ref hasErrors);
if (awaitableTypeOpt is null)
{
awaitOpt = AwaitableInfo.Empty;
}
else
{
hasErrors |= ReportUseSiteDiagnostics(awaitableTypeOpt, diagnostics, awaitKeyword);
BoundExpression placeholder = new BoundAwaitableValuePlaceholder(syntax, awaitableTypeOpt).MakeCompilerGenerated();
awaitOpt = originalBinder.BindAwaitInfo(placeholder, syntax, awaitKeyword.GetLocation(), diagnostics, ref hasErrors);
}
}

// This is not awesome, but its factored.
Expand All @@ -146,6 +152,7 @@ internal static BoundStatement BindUsingStatementOrDeclarationFromParts(SyntaxNo
hasErrors);
}

// initializes iDisposableConversion, awaitableTypeOpt and disposeMethodOpt
bool populateDisposableConversionOrDisposeMethod(bool fromExpression)
{
HashSet<DiagnosticInfo> useSiteDiagnostics = null;
Expand All @@ -155,14 +162,19 @@ bool populateDisposableConversionOrDisposeMethod(bool fromExpression)

if (iDisposableConversion.IsImplicit)
{
if (hasAwait)
{
awaitableTypeOpt = originalBinder.Compilation.GetWellKnownType(WellKnownType.System_Threading_Tasks_ValueTask);
}
return true;
}

TypeSymbol type = fromExpression ? expressionOpt.Type : declarationTypeOpt;

// If this is a ref struct, try binding via pattern.
// If this is a ref struct, or we're in a valid asynchronous using, try binding via pattern.
// We won't need to try and bind a second time if it fails, as async dispose can't be pattern based (ref structs are not allowed in async methods)
if (!(type is null) && type.IsValueType && type.IsRefLikeType)
bool isRefStruct = !(type is null) && type.IsValueType && type.IsRefLikeType;
if (!(type is null) && (isRefStruct || hasAwait))
Copy link
Member

@cston cston Jan 25, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

!(type is null) [](start = 20, length = 15)

For readability, consider avoiding the duplicate !(type is null) check here and above. Perhaps inline isRefStruct in the expression here. Or perhaps use nested if blocks:

if (!(type is null))
{
    bool isRefStruct = ...;
    if (isRefStruct || hasAwait)) { ... }
}
``` #Resolved

{
BoundExpression receiver = fromExpression
? expressionOpt
Expand All @@ -171,6 +183,10 @@ bool populateDisposableConversionOrDisposeMethod(bool fromExpression)
disposeMethodOpt = originalBinder.TryFindDisposePatternMethod(receiver, syntax, hasAwait, diagnostics);
if (!(disposeMethodOpt is null))
{
if (hasAwait)
{
awaitableTypeOpt = disposeMethodOpt.ReturnType.TypeSymbol;
}
return true;
}
}
Expand Down
8 changes: 4 additions & 4 deletions src/Compilers/CSharp/Portable/CSharpResources.Designer.cs
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 4 additions & 4 deletions src/Compilers/CSharp/Portable/CSharpResources.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2981,16 +2981,16 @@ 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 be implicitly convertible to 'System.IDisposable' or implement a suitable 'Dispose' method.</value>
</data>
<data name="ERR_NoConvToIDispWrongAsync" xml:space="preserve">
<value>'{0}': type used in a using statement must be implicitly convertible to 'System.IDisposable'. Did you mean 'await using' rather than 'using'?</value>
<value>'{0}': type used in a using statement must be implicitly convertible to 'System.IDisposable' or implement a suitable 'Dispose' method. Did you mean 'await using' rather than 'using'?</value>
</data>
<data name="ERR_NoConvToIAsyncDisp" xml:space="preserve">
<value>'{0}': type used in an async using statement must be implicitly convertible to 'System.IAsyncDisposable'</value>
<value>'{0}': type used in an async using statement must be implicitly convertible to 'System.IAsyncDisposable' or implement a suitable 'DisposeAsync' method.</value>
</data>
<data name="ERR_NoConvToIAsyncDispWrongAsync" xml:space="preserve">
<value>'{0}': type used in an async using statement must be implicitly convertible to 'System.IAsyncDisposable'. Did you mean 'using' rather than 'await using'?</value>
<value>'{0}': type used in an async using statement must be implicitly convertible to 'System.IAsyncDisposable' or implement a suitable 'DisposeAsync' method. Did you mean 'using' rather than 'await using'?</value>
</data>
<data name="ERR_BadParamRef" xml:space="preserve">
<value>Parameter {0} must be declared with the '{1}' keyword</value>
Expand Down
Loading