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

Unify handling of the @ character in razor code blocks #10232

Merged
merged 7 commits into from
Apr 11, 2024
Merged

Conversation

333fred
Copy link
Member

@333fred 333fred commented Apr 6, 2024

We were extremely inconsistent with how @ was handled in razor code blocks. When @ was not followed immediately by a : or <, we would enter a recovery loop that would eat all characters until the next {, }, or <; this is not correct, and resulted in a number of inconsistent behaviors as well as bad code. In order to make the existing behavior and new behavior difference understandable, I've done a few different test refactorings, so I would highly recommend reviewing this commit-by-commit. Commit summaries:

  1. Have everything base whether to generate baselines on file. The source generator tests still use something different (Conditional methods), but we can address that later.
  2. Have all tests verify their C# diagnostics. There were several tests that are affected by this @ change where it wasn't clear that they weren't actually intended to compile. Now, all integration tests will verify that their C# diagnostics. We should at some point do the same for Razor diagnostics, but we may need to make a similar verification framework for razor diagnostics like we have for C# diagnostics.
  3. Add new tests for @ scenarios to show how it currently behaves.
  4. Finally, the real change: follow the expected rules for @, and update the test baselines to show the new behavior.

Here are the expected rules for @:

  1. When at the start of a statement, @ indicates a transition into an embedded render expression.
  2. When inside a statement:
    1. @: or @< indicate a transition into a render template
    2. Otherwise, we look for a single identifier after @, as C# would

Fixes #10186.

…tics reported by the C# compiler. At some point, we should consider doing the same for Razor diagnostics, but that can be done as a future step.
We were not consistently handling recovery after seeing `@` signs in code blocks, which meant that there were all sorts of interesting scenarios where an escaped C# identifier would cause odd error recovery. There are a few edge cases that are now compile errors, but this is a more consistent overall experience. Fixes dotnet#10186 as well.
@333fred 333fred requested review from a team as code owners April 6, 2024 01:11
@333fred
Copy link
Member Author

333fred commented Apr 6, 2024

@dotnet/razor-compiler modulo some possible test failures that I haven't seen outside the compiler layer yet, I believe this should be ready for review. Please give it a look.

Copy link
Contributor

@chsienki chsienki left a comment

Choose a reason for hiding this comment

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

Love it. LGTM.

@jjonescz jjonescz added the area-compiler Umbrella for all compiler issues label Apr 8, 2024
@333fred
Copy link
Member Author

333fred commented Apr 10, 2024

@chsienki @jjonescz I believe I've addressed feedback, please take another look.

@333fred 333fred mentioned this pull request Aug 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-compiler Umbrella for all compiler issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[dotnet-sdk-9.0.100-preview.3.24175.24] CS1646 error when building Hawaso
4 participants