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

Support closed void tags with content in code blocks #8553

Closed
wants to merge 16 commits into from

Conversation

jjonescz
Copy link
Member

@jjonescz jjonescz commented Mar 31, 2023

Fixes #8460.
Fixes #7258.

Void tags (e.g., <col>, <link>) in code blocks could not have a content and a closing tag, so everything after them was considered to be code. But if the tag is actually a component, I think that behavior doesn't make sense (see the associated issues).

@jjonescz jjonescz added the area-compiler Umbrella for all compiler issues label Mar 31, 2023
@jjonescz jjonescz marked this pull request as ready for review March 31, 2023 15:34
@jjonescz jjonescz requested a review from a team as a code owner March 31, 2023 15:34
chsienki
chsienki previously approved these changes Mar 31, 2023
@jjonescz jjonescz marked this pull request as draft April 3, 2023 12:34
@jjonescz jjonescz dismissed chsienki’s stale review April 4, 2023 13:00

Reworked the fix

@jjonescz jjonescz marked this pull request as ready for review April 4, 2023 13:20
@333fred
Copy link
Member

333fred commented Apr 4, 2023

I don't think I have time to walk through these changes before I'm out for the rest of the week. @jjonescz, when I get back I could use a walkthrough of how we support this outside of code blocks in the parser today.

@jjonescz
Copy link
Member Author

jjonescz commented Apr 5, 2023

how we support this outside of code blocks in the parser today

This problem really only appears inside code blocks. The parser consumes code, then encounters <, starts consuming markup, and after <input> it doesn't know whether it should continue parsing code (if <input> is void self-closed tag) or markup (if it's a component that will be closed later). Outside code blocks, it always parses markup by default except when explicitly told not to (by @ transition)

@jaredpar jaredpar added this to the 17.8 Planning milestone Jul 12, 2023
@chsienki chsienki modified the milestones: 17.8 Planning, 17.9 Planning Oct 11, 2023
@@ -2296,5 +2344,7 @@ private class TagTracker
public SyntaxList<RazorSyntaxNode> PreviousNodes { get; }

public bool IsWellFormed { get; }

public ParserContextState? StateBeforeVoidTag { get; init; }
Copy link
Member

@333fred 333fred Mar 29, 2024

Choose a reason for hiding this comment

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

This is likely not tracking enough info; what if there are multiple? Please add some tests with multiple tags, some of which should be treated as void tags, some of which should not, all within a markup block inside a code block.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, thanks. I'm not yet sure if this will be difficult to fix, need to investigate. But alternatively we could give up this PR but at least emit some better error/warning when trying to use a component named like void tag.

Copy link
Member

Choose a reason for hiding this comment

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

Better backtracking support in general will be useful trying to fix some of the nested } error recovery scenarios, so I think it would be useful to invest in fixing, even if more complex.

Now, whether it's good language design to allow components to shadow void tags... that's an entirely different question 😆. I would have forbidden it and required an escape syntax of some kind, like C# does for identifiers named after keywords.

@jjonescz jjonescz marked this pull request as draft April 3, 2024 09:16
@chsienki
Copy link
Contributor

@jjonescz Are we likely to keep going with this?

@jjonescz
Copy link
Member Author

jjonescz commented Jul 11, 2024

Are we likely to keep going with this?

@chsienki This probably needs a razor language design discussion if we want to support it. In the meantime we could add a warning (perhaps as part of warning wave support which I have another PR open for). We would need to warn at the component declaration itself to tell people they should rename it because it's problematic.

Hm, maybe we could also implement some "force this to be parsed like a component", e.g., adding a bang in front of its name (something like that is already supported for tag helpers IIRC).

I think fully-qualyifing the component with its namespace might be also a good workaround for users. Maybe tooling could suggest that. Or the compiler could suggest it via some warning/info diagnostic at the problematic use site of the component.

@jjonescz jjonescz closed this Oct 17, 2024
@jjonescz jjonescz deleted the 8460-col branch October 17, 2024 14:09
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.

[Blazor WASM] Component named "Col" breaks razor syntax Error when using components in if blazor server side
4 participants