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.

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

Merged
merged 3 commits into from
Jun 6, 2024

Conversation

CyrusNajmabadi
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi commented May 30, 2024

Fixes #73789

@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner May 30, 2024 21:04
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels May 30, 2024
@CyrusNajmabadi
Copy link
Member Author

@dotnet/roslyn-compiler This is ready for review. Tnx.

Copy link
Member

@cston cston left a 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.

@cston cston self-requested a review May 31, 2024 17:21
var saveTerm = _termState;
_termState |= TerminatorState.IsAttributeDeclarationTerminator;

// An attribute can never appear *inside* an attribute declaration in the language. However, during parsing
Copy link
Member

@cston cston May 31, 2024

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
{
}

Copy link
Member Author

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).
Copy link
Member

Choose a reason for hiding this comment

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

(since attributes can not be on lambda expressions)

Attributes can be applied to lambda expressions.

Copy link
Member Author

@CyrusNajmabadi CyrusNajmabadi May 31, 2024

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.

@CyrusNajmabadi
Copy link
Member Author

@cston i think all feedback responded to.

@CyrusNajmabadi
Copy link
Member Author

@dotnet/roslyn-compiler for another set of eyes. thankx :)

@cston cston requested a review from jaredpar June 4, 2024 11:41
@cston cston self-requested a review June 4, 2024 14:31
builder.Append(")]");
builder.Append("class C { }");

var tree = ParseTree(builder.ToString(), CSharpParseOptions.Default);
Copy link
Member Author

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).

@CyrusNajmabadi
Copy link
Member Author

@dotnet/roslyn-compiler for another set of eyes. thanks.

@CyrusNajmabadi
Copy link
Member Author

@jaredpar when you have a minute.

@jaredpar
Copy link
Member

jaredpar commented Jun 6, 2024

@jaredpar when you have a minute.

So never? ;)

@CyrusNajmabadi
Copy link
Member Author

So never? ;)

We'll just pencil it for Monday 6/12 at never o'clock.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

csc.exe hangs on specific file
6 participants