Disallow __arglist in local functions#16660
Conversation
|
@dotnet/roslyn-compiler For review |
| { | ||
| if (p.IsArgList) | ||
| { | ||
| diagnostics.Add(ErrorCode.ERR_IllegalVarArgs, p.Location); |
There was a problem hiding this comment.
diagnostics.Add(ErrorCode.ERR_IllegalVarArgs, p.Location); [](start = 20, length = 58)
It feels like this check belongs in GrabDiagnostics/GetDeclarationDiagnostics on the local function symbol.
There was a problem hiding this comment.
I was going to eagerly report diagnostics that don't have any risk of cycles and only put lazy things into GetDeclarationDiagnostics. Would you prefer that everything be reported in GetDeclarationDiagnostics if feasible?
There was a problem hiding this comment.
I think we should follow approach when diagnostics about declaration is produced by symbols.
| Diagnostic(ErrorCode.ERR_IllegalVarArgs, "__arglist").WithLocation(24, 20), | ||
| // (26,31): error CS0190: The __arglist construct is valid only within a variable argument method | ||
| // Console.WriteLine(__arglist); | ||
| Diagnostic(ErrorCode.ERR_ArgsInvalid, "__arglist").WithLocation(26, 31), |
There was a problem hiding this comment.
This error feels unexpected, it looks like Binder.BindArgList skips through local functions, but it probably shouldn't.
There was a problem hiding this comment.
@AlekseyTs This seems weird for lambdas too... I don't think we should care if the containing member is/is not __arglist if we're inside a lambda/local function while using __arglist. It's invalid no matter what.
I've filed #16720 to rationalize this.
There was a problem hiding this comment.
I don't think this is what the logic is about in the place where ERR_ArgsInvalid is reported. Try to add __arglist to the parameter list of the containing function and see if that makes any difference.
|
Is there a reason to disallow __arglist in local functions from the language design point of view? It looks like a simple change in Binder.BindArgList might enable the feature. |
|
@AlekseyTs the design called for it to be disallowed, mostly because we didn't have a good use case for it. Should keep it disabled at this point as enabling it won't meet the bar. |
Makes sense. |
|
test windows_release_unit64_prtest please |
|
@MattGertz For escrow approval |
|
Approved to merge via 369813 |
Customer scenario
Currently declaring a local function with an __arglist parameter is not illegal, but it should be.
Disallowing this later is a breaking change.
Bugs this fixes:
Fixes #10765
Workarounds, if any
None.
Risk
This is a very small change that's specific to local functions. The change is well understood -- it is the same as the check done in lambdas. This change also only prohibits functionality.
Performance impact
None.
Is this a regression from a previous update?
No.
Root cause analysis:
New feature
How was the bug found?
Customer reported.