Repository navigation
Fix RestrictChildren incorrectly flagging valid child tags - #84870
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: cfc3366c-dc5a-4eed-8937-2c351f70533d
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: cfc3366c-dc5a-4eed-8937-2c351f70533d
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: cfc3366c-dc5a-4eed-8937-2c351f70533d
|
Azure Pipelines: Successfully started running 2 pipeline(s). There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
This pull request addresses dotnet/razor#13219 by adjusting how RestrictChildren/AllowedChildren constraints are validated during tag helper resolution so valid child tags aren’t incorrectly flagged after legacy resolution flattens unmatched elements into HTML content.
Changes:
- Moves AllowedChildren validation earlier in
DefaultTagHelperResolutionPhase(before unresolved children are resolved/flattened), and extends validation to understandUnresolvedElementIntermediateNodechildren. - Uses the
TagHelperBinderduring validation to recognize when a prefixed unresolved element would bind as a tag helper (so the prefix can be stripped for AllowedChildren comparisons). - Adds/extends integration tests to cover non-tag content and the reported regression scenario.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/Razor/src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Language/DefaultTagHelperResolutionPhase.cs | Validates AllowedChildren before child resolution and adds unresolved-element handling using the binder to preserve tag names for correct RestrictChildren behavior. |
| src/Razor/src/Compiler/Microsoft.AspNetCore.Razor.Language/test/DefaultRazorIntermediateNodeLoweringPhaseIntegrationTest.cs | Adds integration coverage for RZ2009/RZ2010 and a WorkItem regression test ensuring allowed markup children don’t produce diagnostics. |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: cfc3366c-dc5a-4eed-8937-2c351f70533d
|
|
||
| // Assert | ||
| var diagnostic = Assert.Single(documentNode.GetAllDiagnostics()); | ||
| Assert.Equal("RZ2010", diagnostic.Id); |
There was a problem hiding this comment.
This used to report RZ2009 (invalid non-tag content), but now reports RZ2010 (invalid child tag) which seems more appropriate, since its the div that is wrong.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/Razor/src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Language/DefaultTagHelperResolutionPhase.cs:546
prefix + parentTagNameis concatenated inside the per-child loop, creating a new string allocation each time we need to check whether a prefixed unresolved child would bind as a tag helper. SinceparentTagNameis constant for this validation pass, precompute the prefixed parent tag name once outside the loop and reuse it in the binder lookup to avoid repeated allocations in a hot compilation path.
binder.GetBinding(
childTagName,
unresolvedElement.AttributeData,
prefix + parentTagName,
parentIsTagHelper: true) != null)
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: cfc3366c-dc5a-4eed-8937-2c351f70533d
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/Razor/src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Language/DefaultTagHelperResolutionPhase.cs:345
- Allowed-children validation now runs (1) once before the reverse walk, and (2) per original
UnresolvedElementIntermediateNodeafterResolveElement. IfResolveElementresolves a StartTagOnly tag helper, it can promote parsed body content intoparent.Childrenas new siblings (see the StartTagOnly promotion logic inResolveElement). Those newly inserted siblings are added after the current index while this loop is iterating backwards, so they won’t be visited/validated here, and they also weren’t present during the initialValidateAllowedChildrenpre-pass. This can allow invalid direct children to slip through (or miss RZ2009/RZ2010) when StartTagOnly promotion occurs under an AllowedChildTags constraint.
Consider adding a validation step for any siblings inserted by StartTagOnly promotion (e.g., detect resolvedTagHelper.TagMode == TagMode.StartTagOnly and validate the promoted range), or restructure the logic so all final direct children are validated without reintroducing the original false-positive issue for flattened plain-HTML elements.
var resolvedTagHelper = ResolveElement(
parent, i, elementNode, binder, prefix, usedHelpers, in context, tagHelperParent);
if (allowedChildrenString != null)
|
/backport to release/10.0.4xx |
|
Started backporting to |
…ild tags (#85060) Backport of #84870 to release/10.0.4xx /cc @davidwengier ## Customer Impact Razor compiler errors in files that previously worked with earlier SDK versions ## Regression - [x] Yes - [ ] No Yes, this was a regression from the rewrite of tag helper resolution into a two-stage approach, to separate resolution from lowering, in dotnet/razor#12957 ## Testing Verified with specific regression tests. It was missed due to historically low test coverage for the variety of Razor language/runtime features. ## Risk Low. The fix builds on previous work for another similar regression caused by the same work, and is ultimately just a re-ordering of diagnostic reporting, not a fundamental compiler change. ###### Microsoft Reviewers: [Open in CodeFlow](https://microsoft.github.io/open-pr/?codeflow=https://github.com/dotnet/roslyn/pull/85060)
Fixes dotnet/razor#13219
Microsoft Reviewers: Open in CodeFlow