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

Remove support for inferring the return type of a local function. #7916

Merged
merged 1 commit into from
Jan 14, 2016
Merged

Remove support for inferring the return type of a local function. #7916

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 1 addition & 9 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 @@ -463,7 +463,7 @@ private BoundStatement BindLocalFunctionStatement(LocalFunctionStatementSyntax n

if (block != null)
{
localSymbol.ComputeReturnType(block, returnNullIfUnknown: false, isIterator: false);
localSymbol.ComputeReturnType();

// Have to do ControlFlowPass here because in MethodCompiler, we don't call this for synthed methods
// rather we go directly to LowerBodyOrInitializer, which skips over flow analysis (which is in CompileMethod)
Expand Down Expand Up @@ -2882,14 +2882,6 @@ protected bool IsGenericTaskReturningAsyncMethod()

protected virtual TypeSymbol GetCurrentReturnType()
{
// Local functions usually report an error when their return type is used recursively. (symbol.ReturnType creates an error type named "var")
// We actually want to return null here instead of an error type + diagnostic (for return type inference)
var containingLocalFunction = this.ContainingMemberOrLambda as LocalFunctionSymbol;
if (containingLocalFunction != null)
{
return containingLocalFunction.ReturnTypeNoForce;
}

return (this.ContainingMemberOrLambda as MethodSymbol)?.ReturnType;
}

Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Binder/InMethodBinder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ internal override GeneratedLabelSymbol ContinueLabel

internal override TypeSymbol GetIteratorElementType(YieldStatementSyntax node, DiagnosticBag diagnostics)
{
TypeSymbol returnType = (_methodSymbol as LocalFunctionSymbol)?.ReturnTypeIterator ?? _methodSymbol.ReturnType;
TypeSymbol returnType = _methodSymbol.ReturnType;

if (!this.IsDirectlyInIterator)
{
Expand Down
144 changes: 11 additions & 133 deletions src/Compilers/CSharp/Portable/Symbols/Source/LocalFunctionSymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,9 @@ internal class LocalFunctionSymbol : MethodSymbol
private ImmutableArray<ParameterSymbol> _parameters;
private ImmutableArray<TypeParameterConstraintClause> _lazyTypeParameterConstraints;
private TypeSymbol _returnType;
private bool _isVar;
private bool _isVararg;
private TypeSymbol _iteratorElementType;

// TODO: Find a better way to report diagnostics.
// We can't put binding in the constructor, as it creates infinite recursion.
// We can't report to Compilation.DeclarationDiagnostics as it's already too late and they will be dropped.
Expand Down Expand Up @@ -61,8 +61,6 @@ public LocalFunctionSymbol(
diagnostics.Add(ErrorCode.ERR_BadExtensionAgg, Locations[0]);
}

_isVar = false;

_binder = binder;
_diagnostics = diagnostics.ToReadOnlyAndFree();
}
Expand All @@ -71,7 +69,7 @@ internal void GrabDiagnostics(DiagnosticBag addTo)
{
// force lazy init
ComputeParameters();
ComputeReturnType(body: null, returnNullIfUnknown: false, isIterator: false);
ComputeReturnType();

var diags = ImmutableInterlocked.InterlockedExchange(ref _diagnostics, default(ImmutableArray<Diagnostic>));
if (!diags.IsDefault)
Expand Down Expand Up @@ -135,80 +133,19 @@ public override TypeSymbol ReturnType
{
get
{
return ComputeReturnType(body: null, returnNullIfUnknown: false, isIterator: false);
}
}

// Reason for ReturnTypeNoForce and ReturnTypeIterator:
// When computing the return type, sometimes we want to return null (ReturnTypeNoForce) instead of reporting a diagnostic
// or sometimes we want to disallow var and report an error about iterators (ReturnTypeIterator)
public TypeSymbol ReturnTypeNoForce
{
get
{
return ComputeReturnType(body: null, returnNullIfUnknown: true, isIterator: false);
return ComputeReturnType();
}
}

public TypeSymbol ReturnTypeIterator
{
get
{
return ComputeReturnType(body: null, returnNullIfUnknown: false, isIterator: true);
}
}

/*
Note: `var` return types are currently very broken in subtle ways, in particular in the IDE scenario when random things are being bound.
The basic problem is that a LocalFunctionSymbol needs to compute its return type, and to do that it needs access to its BoundBlock.
However, the BoundBlock needs access to the local function's return type. Recursion detection is tricky, because this property (.ReturnType)
doesn't have access to the binder where it is being accessed from (i.e. either from inside the local function, where it should report an error,
or from outside, where it should attempt to infer the return type from the block).

The current (broken) system assumes that Binder_Statements.cs BindLocalFunctionStatement will always be called (and so a block will be provided)
before any (valid) uses of the local function are bound that require knowing the return type. This assumption breaks in the IDE, where
a use of a local function may be bound before BindLocalFunctionStatement is called on the corresponding local function.
*/
internal TypeSymbol ComputeReturnType(BoundBlock body, bool returnNullIfUnknown, bool isIterator)
internal TypeSymbol ComputeReturnType()
{
if (_returnType != null)
{
return _returnType;
}

var diagnostics = DiagnosticBag.GetInstance();
// we might call this multiple times if it's var. Only bind the first time, and cache if it's var.
TypeSymbol returnType = null; // guaranteed to be assigned, but compiler doesn't know that.
if (!_isVar)
{
bool isVar;
returnType = _binder.BindType(_syntax.ReturnType, diagnostics, out isVar);
_isVar = isVar;
}
if (_isVar)
{
if (isIterator) // cannot use IsIterator (the property) because that gets computed after the body is bound, which hasn't happened yet.
{
// Completely disallow use of var inferred in an iterator context.
// This is because we may have IAsyncEnumerable and similar types, which determine the type of state machine to emit.
// If we infer the return type, we won't know which state machine to generate.
returnType = _binder.CreateErrorType("var");
// InMethodBinder reports ERR_BadIteratorReturn, so no need to report a diagnostic here.
}
else if (body == null)
{
if (returnNullIfUnknown)
{
diagnostics.Free();
return null;
}
returnType = _binder.CreateErrorType("var");
diagnostics.Add(ErrorCode.ERR_RecursivelyTypedVariable, _syntax.ReturnType.Location, this);
}
else
{
returnType = InferReturnType(body, diagnostics);
}
}
TypeSymbol returnType = _binder.BindType(_syntax.ReturnType, diagnostics);
var raceReturnType = Interlocked.CompareExchange(ref _returnType, returnType, null);
if (raceReturnType != null)
{
Expand All @@ -220,74 +157,15 @@ internal TypeSymbol ComputeReturnType(BoundBlock body, bool returnNullIfUnknown,
// The return type of an async method must be void, Task or Task<T>
diagnostics.Add(ErrorCode.ERR_BadAsyncReturn, this.Locations[0]);
}
// TODO: note there is a race condition here that will ultimately need to be fixed.
// Specifically, the Interlocked.CompareExchange above succeeds, and will be seen by
// other threads, before the diagnostics have been recorded in this symbol, below.
// We can resolve this with a lock around this method and any other methods that
// manipulate _diagnostics, directly or indirectly.
AddDiagnostics(diagnostics.ToReadOnlyAndFree());
Copy link
Member

Choose a reason for hiding this comment

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

Consider logging a bug for TODO.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done (#7938)

return returnType;
}

private TypeSymbol InferReturnType(BoundBlock body, DiagnosticBag diagnostics)
{
int numberOfDistinctReturns;
var resultTypes = BoundLambda.BlockReturns.GetReturnTypes(body, out numberOfDistinctReturns);
if (numberOfDistinctReturns != resultTypes.Length) // included a "return;", no expression
{
resultTypes = resultTypes.Concat(ImmutableArray.Create((TypeSymbol)_binder.Compilation.GetSpecialType(SpecialType.System_Void)));
}

TypeSymbol returnType;
if (resultTypes.IsDefaultOrEmpty)
{
returnType = _binder.Compilation.GetSpecialType(SpecialType.System_Void);
}
else if (resultTypes.Length == 1)
{
returnType = resultTypes[0];
}
else
{
// Make sure every return type is exactly the same (not even a subclass of each other)
returnType = null;
foreach (var resultType in resultTypes)
{
if ((object)returnType == null)
{
returnType = resultType;
}
else if ((object)returnType != (object)resultType)
{
returnType = null;
break;
}
}
if (returnType == null)
{
returnType = _binder.CreateErrorType("var");
diagnostics.Add(ErrorCode.ERR_ReturnTypesDontMatch, Locations[0], this);
return returnType;
}
}

// do this before async lifting, as inferring Task is also not allowed.
if (returnType.SpecialType == SpecialType.System_Void)
{
diagnostics.Add(ErrorCode.ERR_CantInferVoid, Locations[0], this);
}

if (IsAsync)
{
if (returnType.SpecialType == SpecialType.System_Void)
{
returnType = _binder.Compilation.GetWellKnownType(WellKnownType.System_Threading_Tasks_Task);
}
else
{
returnType = _binder.Compilation.GetWellKnownType(WellKnownType.System_Threading_Tasks_Task_T).Construct(returnType);
}
}
// cannot be iterator

return returnType;
}

public override bool ReturnsVoid => ReturnType?.SpecialType == SpecialType.System_Void;

public override int Arity => TypeParameters.Length;
Expand Down
Loading