Make local function parameters bind lazily #16736
Conversation
|
@dotnet/roslyn-compiler For review |
| @@ -194,23 +193,28 @@ internal override bool IsCallerMemberName | |||
| } | |||
| } | |||
|
|
|||
| private ConstantValue DefaultSyntaxValue | |||
| private ConstantValue GetDefaultSyntaxValue(DiagnosticBag diagnosticsOpt = null) | |||
There was a problem hiding this comment.
private ConstantValue GetDefaultSyntaxValue(DiagnosticBag diagnosticsOpt = null) [](start = 8, length = 80)
Please open an issue to get rid of this pattern post RTM #Closed
There was a problem hiding this comment.
Do you think this is encompassed by #16652?
That would remove the flakiness with adding to declaration diagnostics.
There was a problem hiding this comment.
Somewhat, I think we should explicitly track API changes what we would like to revert. We can keep the list in that issue. #Closed
| { | ||
| get { return null; } | ||
| } | ||
| protected virtual Binder ParameterBinder { get; } |
There was a problem hiding this comment.
protected virtual Binder ParameterBinder { get; } [](start = 8, length = 49)
It feels like this property can be calculated from the containing LocalFunctionSymbol. #Closed
| { | ||
| get { return null; } | ||
| } | ||
| protected virtual Binder ParameterBinder { get; } |
There was a problem hiding this comment.
protected [](start = 8, length = 9)
private? #Closed
| protected ConstantValue _lazyDefaultSyntaxValue; | ||
| private ConstantValue _lazyDefaultSyntaxValue; | ||
|
|
||
| private Binder _parameterBinder |
There was a problem hiding this comment.
_parameterBinder [](start = 23, length = 16)
_parameterBinder [](start = 23, length = 16)
Does this follow naming convention for properties? #Closed
There was a problem hiding this comment.
Consider naming LocalFunctionParameterBinder so it's clear when the binder is expected.
| ((MethodSymbol)ContainingSymbol).MethodKind == MethodKind.LocalFunction; | ||
| return isLocalFunction | ||
| ? ((LocalFunctionSymbol)ContainingSymbol).ParameterBinder | ||
| : null; |
There was a problem hiding this comment.
While we typically prefer Kind checks, would it be simpler to use as instead here?
return (ContainingSymbol as LocalFunctionSymbol)?.ParameterBinder;
| // int Local2(int p = Local1()) => 0; | ||
| Diagnostic(ErrorCode.ERR_DefaultValueMustBeConstant, "Local1()").WithArguments("p").WithLocation(7, 28)); | ||
| comp.DeclarationDiagnostics.Verify(); | ||
| } |
There was a problem hiding this comment.
Is nameof interesting? string Local(string s = nameof(Local)) => s;
|
LGTM |
Local function parameters seemed to be designed to bind their parameters' default values eagerly, as opposed to methods which bind their default values lazily. This creates the possibility of infinite binder recursion if the default value mentions the local function itself (which has yet to be fully bound). Fixes dotnet#16451
agocke
force-pushed
the
localfunc-default-params
branch
from
811d4b3
to
580f286
Compare
|
@MattGertz For escrow approval |
|
Rejected for RTW per raised bar in Shiproom. Please target 2.1 for this fix. |
|
@agocke We probably shouldn't merge this into 2.1 as is without addressing the issue around GetDefaultSyntaxValue (#16736 (review)). |
|
@AlekseyTs Agreed |
Customer scenario
Mentioning a local function in the default value of one of its parameters will cause the compiler to crash. The following program is the simplest repro for this scenario:
Bugs this fixes:
Fixes #16451
Workarounds, if any
Don't mention the local function in its parameters' default values.
Risk
This change preserves the existing code paths for binding of default parameters for almost all circumstances and simply attempts to delay the code path for local functions to occur later.
Performance impact
Low. Same code path, just done lazily.
Is this a regression from a previous update?
No.
Root cause analysis:
Insufficient testing across new language features and lack of adversarial testing. Unit tests have been added for this and scenarios like this.
How was the bug found?
Testing local function default parameters with other language features.