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

Razor fallback changes #73001

Merged
merged 7 commits into from
May 6, 2024
Merged

Razor fallback changes #73001

merged 7 commits into from
May 6, 2024

Conversation

333fred
Copy link
Member

@333fred 333fred commented Apr 12, 2024

This adds a couple of new APIs to deal with edge cases that Razor needs to care about:

  • We add ParseLeadingTrivia and ParseTrailingTrivia methods to the SyntaxTokenParser, which will allow Razor to handle trivia sections before a @: correctly.
  • We add a dedicated bad token type for BadRazorContentToken. We don't want to use a generic BadToken for this because BadToken can cause parsing loops to fail upward, under the assumption that an outer scope may better be able to handle it. We don't to negatively affect mainline C# parsing here, so we add a generic token for this scenario that the parser will always convert into skipped tokens later.

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Apr 12, 2024
@333fred
Copy link
Member Author

333fred commented May 1, 2024

/azp run

@333fred
Copy link
Member Author

333fred commented May 1, 2024

I'm going to proceed with these changes for now, and I've marked #73002 as blocking so that we cover it in our next API review session. @dotnet/roslyn-compiler @CyrusNajmabadi for review please.

@333fred 333fred marked this pull request as ready for review May 1, 2024 19:47
@333fred 333fred requested review from a team as code owners May 1, 2024 19:47
@333fred
Copy link
Member Author

333fred commented May 2, 2024

@dotnet/roslyn-compiler @CyrusNajmabadi for reviews please

@@ -526,6 +526,7 @@ public enum SyntaxKind : ushort
Utf8StringLiteralToken = 8520,
Utf8SingleLineRawStringLiteralToken = 8521,
Utf8MultiLineRawStringLiteralToken = 8522,
BadRazorContentToken = 8523,
Copy link
Member

Choose a reason for hiding this comment

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

do we want some sort of razor section?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so? I think the single token should be fine.

Copy link
Member

@CyrusNajmabadi CyrusNajmabadi May 2, 2024

Choose a reason for hiding this comment

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

wfm. what i was imaginging though was that there might be a few types of razor kinds we detected. like razor comments, bad razor tokens. perhaps a couple different razor tokens.

--

Ah, i now see why i don't like this name. i want somethnig to indicate it's a @:. So perhaps BadRazorAtSignColonToken. Or perhaps BadRazorLineToken. Something that really conveys this is the token we produce when we see @: and go to the end of the line.

I feel moderately strongly :)

Copy link
Member Author

Choose a reason for hiding this comment

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

See, I want exactly the opposite: I don't want to break out the various things we do for error recovery for razor. What if next year razor adds a new thing that we want to recover for, are we going to need to break that out separately? Or what if Razor removes the need for us to handle this? I'd rather it just be "this is something C# doesn't expect", with no further specificity.

Copy link
Member Author

Choose a reason for hiding this comment

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

Or what if C# decides that @: is a cool syntax to use for something next year, so we add an AtColonToken, and now we have this vestigal BadRazorAtSignColonToken.

Copy link
Member

Choose a reason for hiding this comment

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

i'll let you make the call here. but my gut tells me breaking things out would be valuable. especially as i can imagine you wanting ot have a particular error recovery strategy for a particular razor token in a particular position.

That said, this is nto blocking feedback. You make the call. Imagine if you do want specific error recovery, you can still get it, even with a single kind. Because you can always still examine the token to see the starting chars and whatnot. It just seems oogier. But if that's your preference, totally ok with me. My sign off still remains :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Razor is already in the business of inspecting characters, so that doesn't feel oogie to me 🙂. I'd prefer to keep the C# api flexible here.

/// Parse the leading trivia of the next token from the input at the current position. This will advance the internal position of the
/// token parser to the end of the leading trivia of the next token. The returned result will have a token with <see cref="CSharpExtensions.Kind(SyntaxToken)"/>
/// of <see cref="SyntaxKind.None"/>, <see cref="SyntaxToken.IsMissing"/> set to <see langword="true"/>, and a parent of <see langword="null"/>. The
/// parsed trivia will be set as the <see cref="SyntaxToken.LeadingTrivia"/> of the token.
Copy link
Member

Choose a reason for hiding this comment

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

nice.

Copy link
Member

@CyrusNajmabadi CyrusNajmabadi left a comment

Choose a reason for hiding this comment

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

parser changes lgtm and work in a consistent way that i think fits roslyn's syntactic model.

still not 100% sure about how razor will be consuming this and if it will be sufficient for your needs.

Open question to consider: should we make this API "experiemntal" so that if we discover we need to make tweaks in teh short term, we can do so more easily without leaving cruft behind.

* upstream/main: (1047 commits)
  Ensure source paths are comparable with editorconfig directory paths (dotnet#73100)
  Lower drop retention from 10 years to 3 months (dotnet#73190)
  Move
  fix
  restore code
  remove
  simplify
  simplify
  Move off of map
  Simplify tests
  Simplify tests
  Simplify tests
  Simplify tests
  Simplify tests
  simplify
  finish
  Simplify
  Fix
  Finish
  Remove dispose support
  ...
@jaredpar
Copy link
Member

jaredpar commented May 3, 2024

@cston, @jjonescz PTAL

@jaredpar jaredpar added this to the 17.11 milestone May 3, 2024
Co-authored-by: Jan Jones <[email protected]>
@333fred 333fred enabled auto-merge (squash) May 6, 2024 16:33
@333fred 333fred merged commit 8d6f547 into dotnet:main May 6, 2024
24 checks passed
@dotnet-policy-service dotnet-policy-service bot modified the milestones: 17.11, Next May 6, 2024
@333fred 333fred deleted the at-colon-v2 branch May 6, 2024 19:20
@Cosifne Cosifne modified the milestones: Next, 17.11 P2 May 28, 2024
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.

6 participants