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

Jump to bottom

Nullable analysis of yield return #31072

Merged
merged 9 commits into from
Nov 13, 2018
Merged

Nullable analysis of yield return #31072

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
53d4e8a
Nullable analysis of yield return
jcouv Nov 8, 2018
5019025
Address feedback from Andy
jcouv Nov 9, 2018
ea0ac2a
Bulk undo all modified binding files
jcouv Nov 9, 2018
875554c
Take a NullableWalker-only approach
jcouv Nov 9, 2018
c06f8af
Address feedback from Chuck
jcouv Nov 10, 2018
d138c17
Update tests and address feedback
jcouv Nov 12, 2018
cc016e4
Fix regression caught by CI
jcouv Nov 12, 2018
410272e
Factor code more
jcouv Nov 12, 2018
3965ca0
Inlined binder.GetSpecialType to remove binder parameter
jcouv Nov 12, 2018
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
16 changes: 11 additions & 5 deletions src/Compilers/CSharp/Portable/Binder/InMethodBinder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,27 +176,33 @@ internal override TypeSymbol GetIteratorElementType(YieldStatementSyntax node, D
}

private TypeSymbol GetIteratorElementTypeFromReturnType(RefKind refKind, TypeSymbol returnType, CSharpSyntaxNode errorLocationNode, DiagnosticBag diagnostics)
{
return GetIteratorElementTypeFromReturnType(Compilation, binder: this, refKind, returnType, errorLocationNode, diagnostics).TypeSymbol;
}

internal static TypeSymbolWithAnnotations GetIteratorElementTypeFromReturnType(CSharpCompilation compilation, Binder binder, RefKind refKind, TypeSymbol returnType, CSharpSyntaxNode errorLocationNode, DiagnosticBag diagnostics)
{
if (refKind == RefKind.None && returnType.Kind == SymbolKind.NamedType)
{
switch (returnType.OriginalDefinition.SpecialType)
{
case SpecialType.System_Collections_IEnumerable:
case SpecialType.System_Collections_IEnumerator:
return GetSpecialType(SpecialType.System_Object, diagnostics, errorLocationNode);
var objectType = diagnostics is null ? compilation.GetSpecialType(SpecialType.System_Object) : binder.GetSpecialType(SpecialType.System_Object, diagnostics, errorLocationNode);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe split the ternary over three lines?

return TypeSymbolWithAnnotations.Create(objectType);

case SpecialType.System_Collections_Generic_IEnumerable_T:
case SpecialType.System_Collections_Generic_IEnumerator_T:
return ((NamedTypeSymbol)returnType).TypeArgumentsNoUseSiteDiagnostics[0].TypeSymbol;
return ((NamedTypeSymbol)returnType).TypeArgumentsNoUseSiteDiagnostics[0];
}

if (returnType.OriginalDefinition == Compilation.GetWellKnownType(WellKnownType.System_Collections_Generic_IAsyncEnumerable_T))
if (returnType.OriginalDefinition == compilation.GetWellKnownType(WellKnownType.System_Collections_Generic_IAsyncEnumerable_T))
{
return ((NamedTypeSymbol)returnType).TypeArgumentsNoUseSiteDiagnostics[0].TypeSymbol;
return ((NamedTypeSymbol)returnType).TypeArgumentsNoUseSiteDiagnostics[0];
}
}

return null;
return default;
}

internal override void LookupSymbolsInSingleBinder(
Expand Down
14 changes: 14 additions & 0 deletions src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4592,6 +4592,20 @@ public override BoundNode VisitThrowExpression(BoundThrowExpression node)
return result;
}

public override BoundNode VisitYieldReturnStatement(BoundYieldReturnStatement node)
{
BoundExpression expr = node.Expression;
if (expr == null)
{
return null;
}
Copy link
Member

@cston cston Nov 12, 2018 •
edited by jcouv
Loading

Choose a reason for hiding this comment

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

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

Please add a test where expr.HasErrors and also results in nullable warnings. #Resolved

Copy link
Member Author

@jcouv jcouv Nov 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry, it looks like I had forgotten to push the commit updating tests. There are already tests for this.
Please take another look.

I much prefer the behavior that avoids cascading diagnostics. We should discuss more. I filed a follow-up issue: #31121 #Resolved

var method = (MethodSymbol)_member;
Debug.Assert(method.IsIterator);
TypeSymbolWithAnnotations elementType = InMethodBinder.GetIteratorElementTypeFromReturnType(compilation, binder: null, RefKind.None, method.ReturnType.TypeSymbol, errorLocationNode: null, diagnostics: null);
VisitOptionalImplicitConversion(expr, elementType, useLegacyWarnings: false, AssignmentKind.Return);
return null;
}

#endregion Visitors

protected override string Dump(LocalState state)
Expand Down
Loading