-
Notifications
You must be signed in to change notification settings - Fork 4k
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.
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
Fix exponential blowup parsing pathological files #73788
Conversation
@dotnet/roslyn-compiler This is ready for review. Tnx. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test case should be a minimal repro to hit the specific parsing issue.
73df9c6
to
7923e2f
Compare
var saveTerm = _termState; | ||
_termState |= TerminatorState.IsAttributeDeclarationTerminator; | ||
|
||
// An attribute can never appear *inside* an attribute declaration in the language. However, during parsing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An attribute can never appear inside an attribute declaration in the language.
It looks like the grammar allows an attribute argument expression to contain a lambda expression with an attribute on the lambda. This is only disallowed in binding because the lambda expression does not represent a constant, correct?
Example: sharplab.io
[A([A]() => 1)] // error CS0182: attribute argument must be a constant ...
class Program
{
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is only disallowed in binding because the lambda expression does not represent a constant, correct?
Correct. This is never going to be legal code. So the decision here is to prevent it at parsing time vs binding time (and only for the attributed lambda case). That way we can avoid pathlogical parsing conditions.
Pros: Pathological parsing cases don't go off hte rails in speculative parsing.
Cons: A syntactically legal (but semantically illegal) case errors earlier and produces a worse parse tree.
This seems like an easy justification to make. The impact of speculative parsing here is enormous (it literally hangs vs/compiler). While the impact of disallowing an attributed-lambda in an attribute at parsing time is extremely minor. Who is realistically writing that? And, even if they do, getting a different syntactic error is fine.
var saveTerm = _termState; | ||
_termState |= TerminatorState.IsAttributeDeclarationTerminator; | ||
|
||
// An attribute can never appear *inside* an attribute declaration in the language. However, during parsing | ||
// we can end up in a state where we're trying to exactly that, through a path of | ||
// Attribute->Argument->Expression->Attribute (since attributes can not be on lambda expressions). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Attributes can be applied to lambda expressions.
Not legally. That's my point here. Attributes can't ever actually be used within an attribute argument itself.
src/Compilers/CSharp/Test/Syntax/Parsing/LambdaAttributeParsingTests.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Test/Syntax/Parsing/LambdaAttributeParsingTests.cs
Outdated
Show resolved
Hide resolved
@cston i think all feedback responded to. |
src/Compilers/CSharp/Test/Syntax/Parsing/LambdaAttributeParsingTests.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Test/Syntax/Parsing/LambdaAttributeParsingTests.cs
Outdated
Show resolved
Hide resolved
b375aee
to
2695c24
Compare
@dotnet/roslyn-compiler for another set of eyes. thankx :) |
src/Compilers/CSharp/Test/Syntax/Parsing/LambdaAttributeParsingTests.cs
Outdated
Show resolved
Hide resolved
builder.Append(")]"); | ||
builder.Append("class C { }"); | ||
|
||
var tree = ParseTree(builder.ToString(), CSharpParseOptions.Default); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: this test no longer validates https://github.com/dotnet/roslyn/pull/73788/files#diff-a655d393d9799eabace5e5109383232749be8052f1fd825fc52b9f170908e838R11891. We can remove that codee and this test passes (though the original user report still hangs the compiler).
@dotnet/roslyn-compiler for another set of eyes. thanks. |
@jaredpar when you have a minute. |
So never? ;) |
We'll just pencil it for Monday 6/12 at never o'clock. |
Fixes #73789