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

[FUSE] Fix code folding #10459

Merged
merged 5 commits into from
Jun 17, 2024
Merged

Conversation

chsienki
Copy link
Member

@chsienki chsienki commented Jun 7, 2024

When parsing code blocks, don't associate the trailing whitespace and newline with the actual directive, but as a meta node that follows it. That ensures that the directive itself only covers the space up to the final }, but we still class the trailing whitespace as C# so don't emit it as HTML either.

Fixes #10358

@chsienki chsienki added area-compiler Umbrella for all compiler issues New Feature: Fuse labels Jun 7, 2024
@chsienki chsienki marked this pull request as ready for review June 10, 2024 18:08
@chsienki chsienki requested a review from a team as a code owner June 10, 2024 18:08
@chsienki
Copy link
Member Author

chsienki commented Jun 10, 2024

@dotnet/razor-compiler for reviews please :)

Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

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

Not sure I'm a huge fan of this without some driving principle behind it. In Roslyn, you can be confident that, when you look at a piece of text, you know where the whitespace will end up; I'm not certain that I can state such invariants here (before or after the change). Can we make such a statement, and then document it and adjust the code to follow it?

@chsienki
Copy link
Member Author

@333fred I'm not sure I understand the ask totally; where would you want to document it?

@333fred
Copy link
Member

333fred commented Jun 11, 2024

Probably a simple markdown file in the docs/ folder. We can add things there in a future PR as well, like how the FindToken API works.

Copy link
Member

Choose a reason for hiding this comment

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

Can we add some unit tests for the folding issue (some test that would fail without this PR)? If that would not be easy, perhaps at least some compiler unit test that verifies the whitespace is attached to the correct nodes. The baselines themselves are not enough I think - we won't notice if we regress this in the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

We do have the integration tests (which is how we found this) but I'll see if I can cook up some parsing tests.

Copy link
Member

@jjonescz jjonescz Jun 14, 2024

Choose a reason for hiding this comment

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

I see you added some parsing tests which are nice because they are narrow and test only the actual issue. But it doesn't address my concern - we usually regenerate lots of baselines and it's difficult to review them all and check they look as expected in all aspects - so it would be really easy to regress this fix in the future. Can we add some asserts that check the whitespace or spans of the nodes, i.e., something that would fail regardless of baselines?

docs/Parsing.md Outdated

```

This would be previously be conceptually parsed as something like:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
This would be previously be conceptually parsed as something like:
This would previously be conceptually parsed as something like:

Copy link
Member

@jjonescz jjonescz Jun 14, 2024

Choose a reason for hiding this comment

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

I see you added some parsing tests which are nice because they are narrow and test only the actual issue. But it doesn't address my concern - we usually regenerate lots of baselines and it's difficult to review them all and check they look as expected in all aspects - so it would be really easy to regress this fix in the future. Can we add some asserts that check the whitespace or spans of the nodes, i.e., something that would fail regardless of baselines?

@@ -244,4 +245,104 @@ public void SupportsAllKindsOfImplicitMarkupInCodeBlock()
}
""");
}

[Fact]
Copy link
Member

Choose a reason for hiding this comment

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

Consider adding WorkItemAttributes linking to the issue

Copy link
Member

Choose a reason for hiding this comment

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

Thanks. This was incredibly helpful for reviewing this PR.

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 New Feature: Fuse
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Whitespace changes behavior of folding @code ranges in fuse
3 participants