Conversation
…hase - Remove syntax tree access from DefaultTagHelperResolutionPhase.ExecuteCore; use codeDocument.ParserOptions directly instead of syntaxTree?.Options - Fix null handling: parserOptions was potentially null when no syntax tree was present; codeDocument.ParserOptions is always non-null - Remove unused ParserOptions field from ResolutionContext struct - Add clarifying comment explaining why options are read from the document Agent-Logs-Url: https://github.com/dotnet/razor/sessions/823f746a-8af7-4c38-8d38-e58389029de9 Co-authored-by: chsienki <16246502+chsienki@users.noreply.github.com>
…agHelperRewrittenSyntaxTree - Rename RazorCodeDocument fields and all related methods: - _preTagHelperSyntaxTree → _syntaxTree (canonical, pre-rewrite tree) - _syntaxTree → _tagHelperRewrittenSyntaxTree (rewritten tree) - Update all phase code: - DefaultRazorParsingPhase: WithSyntaxTree → WithTagHelperRewrittenSyntaxTree - DefaultRazorSyntaxTreePhase: same rename - DefaultRazorTagHelperContextDiscoveryPhase: update fallback + early return - DefaultRazorIntermediateNodeLoweringPhase: remove fallback (uses GetSyntaxTree()) - DefaultRazorTagHelperRewritePhase: TryGetSyntaxTree + WithTagHelperRewrittenSyntaxTree - SourceGeneratorProjectEngine: WithSyntaxTree(GetRequiredTagHelperRewrittenSyntaxTree()) - Update all workspaces production code to GetTagHelperRewrittenSyntaxTree - Update all test files to use correct methods - All 4195 tests pass Agent-Logs-Url: https://github.com/dotnet/razor/sessions/71473a82-86d0-4095-8117-46f8a6a66aea Co-authored-by: chsienki <16246502+chsienki@users.noreply.github.com>
…urceGeneratorProjectEngine Agent-Logs-Url: https://github.com/dotnet/razor/sessions/71473a82-86d0-4095-8117-46f8a6a66aea Co-authored-by: chsienki <16246502+chsienki@users.noreply.github.com>
…ack in discovery phase - DefaultRazorParsingPhase: sets _syntaxTree (canonical) directly, not _tagHelperRewrittenSyntaxTree - DefaultRazorSyntaxTreePhase: reads/writes _syntaxTree (canonical) - DefaultRazorTagHelperContextDiscoveryPhase: reads _syntaxTree directly (no fallback needed since parsing always sets it); also removes redundant WithSyntaxTree(syntaxTree) write and simplifies early return - SourceGeneratorProjectEngine: remove redundant WithSyntaxTree() pre-discovery call (parsing now sets it) - NamespaceComputer: TryGetTagHelperRewrittenSyntaxTree → TryGetSyntaxTree - Compiler-layer tests: all non-rewrite tests switch to GetSyntaxTree()/WithSyntaxTree() - Execute_CombinesErrorsOnRewritingErrors: keeps GetTagHelperRewrittenSyntaxTree() (tests rewrite output) - IntegrationTestBase/RazorBaselineIntegrationTestBase: use GetTagHelperRewrittenSyntaxTree() ?? GetRequiredSyntaxTree() fallback for AssertSourceMappingsMatchBaseline/AssertLinePragmas - RazorCodeDocumentExtensionsTest: WithSyntaxTree (matches NamespaceComputer using TryGetSyntaxTree) - All 4195 tests pass Agent-Logs-Url: https://github.com/dotnet/razor/sessions/11f652dc-32e1-43bf-987a-3994b23f355d Co-authored-by: chsienki <16246502+chsienki@users.noreply.github.com>
…written tree When context.TagHelpers is empty, the rewrite phase early-returns without setting _tagHelperRewrittenSyntaxTree. Previously this was fine because parsing set it; now that parsing sets _syntaxTree (canonical), the rewritten tree must be explicitly propagated in the no-tag-helpers path. Also revert TaskListDiagnosticProviderTest to use WithTagHelperRewrittenSyntaxTree since it tests tooling code (TaskListDiagnosticProvider) that reads the rewritten tree via GetRequiredSyntaxRoot(). Agent-Logs-Url: https://github.com/dotnet/razor/sessions/b3f84bbc-9b8b-41ba-9e14-335907d39162 Co-authored-by: chsienki <16246502+chsienki@users.noreply.github.com>
davidwengier
left a comment
There was a problem hiding this comment.
I don't love that this rename was not just internal to the compiler, because I think it's an easy trap for tooling to call GetSyntaxTree and not get the final one. BUT long term I guess we hope there is only GetSyntaxTree so maybe its okay?
Not sure if FinalSyntaxTree might be a better name, as its easier for tooling to reason about how it might need that one. Alternatively, make GetSyntaxTree be GetUnresolvedSyntaxTree, for the same reason.
I could see an LLM making the same mistake here, and calling the wrong method.
BUT maybe this was all discussed? I don't recall, so I'm approving because everything else looks fine
|
@davidwengier No it wasn't discussed it was just my naming, so it's totally fine if you want to change things. I guess my intent is that right now Given that the tooling stuff isn't going to happen immediately I'm happy to rename it to something else, but I want to keep |
In that case, feel free to ignore and merge. If it was going to hand around longer term, then I might feel more strongly. |
Background
RazorCodeDocumenthas two syntax tree fields that were previously named confusingly:GetSyntaxTree(), which is unintuitiveWhat this PR does
1. Rename the fields and methods
The methods are renamed to make their intent unambiguous:
GetPreTagHelperSyntaxTree()GetSyntaxTree()GetSyntaxTree()GetTagHelperRewrittenSyntaxTree()Same pattern for
TryGet*,GetRequired*, andWith*variants, plus the backing fields.2. Restructure the pipeline to set the canonical tree at parse time
Previously:
DefaultRazorParsingPhaseset_tagHelperRewrittenSyntaxTreeDefaultRazorTagHelperContextDiscoveryPhaseread the working tree, then saved it as the canonical treeGetSyntaxTree() ?? GetTagHelperRewrittenSyntaxTree()Now:
DefaultRazorParsingPhasesets_syntaxTree(canonical) directlyDefaultRazorSyntaxTreePhasereads and writes_syntaxTreeDefaultRazorTagHelperContextDiscoveryPhasereads_syntaxTreedirectly — no fallback, no redundant writeDefaultRazorTagHelperRewritePhasereads_syntaxTree, rewrites it, and writes_tagHelperRewrittenSyntaxTree; when no tag helpers exist it copies the canonical tree so tooling can unconditionally callGetRequiredTagHelperRewrittenSyntaxTree()3. Update tests to use the right tree
Compiler-layer tests (which don't go through tag helper rewriting) now use
GetSyntaxTree()/WithSyntaxTree(). Only the one test that explicitly validates rewrite-phase output (Execute_CombinesErrorsOnRewritingErrors) continues to useGetTagHelperRewrittenSyntaxTree(). Tooling-layer tests keep usingGetTagHelperRewrittenSyntaxTree()since they test features that depend on the fully-rewritten tree.