#11793 Simplify tag to self-closing code action

[Peter-Juhasz]

Summary of the changes

• Introduced a new Code Action Provider and Resolver
• Added test cases using the cohost method as advised in comment which cover the conditions

[Peter-Juhasz]

@dotnet-policy-service agree

[Peter-Juhasz]

Since there is no symbol information available here, if there is a better way to check whether an Attribute is of type of a RenderFragment, let me know.

[Peter-Juhasz]

IsChildContentProperty() and IsParameterizedChildContentProperty() on BoundAttributeDescriptor may be a better choice, although naming is not clear: they both use metadata populated by the compiler layer, calculated by the IsRenderFragment(IPropertySymbol property) method under the hood.

Shall I introduce an IsRenderFragmentProperty() alias on BoundAttributeDescriptor for IsChildContentProperty() to make naming clear at least on this layer?

[davidwengier]

It's not clear to me why we're explicitly checking for RenderFragment anyway. Why are they important? I would have thought the more important thing to check for was ChildContent.

eg, given a component with a random render fragment property "Foo", if its required and not supplied, we can check for diagnostics, if its not required and not supplied then it doesn't matter, and if its not required and supplied, then it will be non-whitespace characters in the tag, so will already be filtered out.

ChildContent though, we need to check for because something this:

<Goo>

</Goo>

That could be a component with no content, or, if there is a ChildContent property, it could be a component with ChildContent set to some whitespace, which we probably want to keep.

[Peter-Juhasz]

why we're explicitly checking for RenderFragment anyway?

Because RenderFragement typed Parameters (including named ChildContent if any) are the only ones which can be defined in the Component markup element syntax body (too). Not only named ChildContent but any other:

<LoadItemView Load="LoadAsync">
  <FoundView Context="item">...</FoundView>
  <NotFoundView>...</NotFoundView>
  <ErrorView>...</ErrorView>
</LoadItemView>

ChildContent is indeed a special case and distinguished by the language, but I believe having a generalized approach has more value.

That could be a component with no content, or, if there is a ChildContent property, it could be a component with ChildContent set to some whitespace, which we probably want to keep.

Having a Component with an optional RenderFragment? ChildContent Parameter, designed explicitly to accept some form of whitespace, which is then set by the user as a markup element body syntax (which is subject to formatting, so setting it as an attribute would make more sense) is quite an edge case I believe.

While the majority of the cases is that people have Components with optional/empty or no ChildContent Parameters, end up with their tags fully closed with an end tag, offering this refactor would bring value. (It could be a follow up task to improve Auto Insert to close applicable Component tags in a self-closing way.)

Even so, this refactoring is not offered as a Diagnostic which would be reported and then a Code Fix for it, but an optional refactor which can be executed at will, because Razor allows both ways to work (without applying this refactor).

[Peter-Juhasz]

In other words: we can only offer simplification, if all editor required RenderFragment Parameters are already set (either in body, or set as an attribute, or has a binding as an attribute).

If there are still any left (so it is not set as an attribute either), we need to keep open that it may be set in the body (which is usually the case), so we don't offer the simplification yet.

[davidwengier]

Let's agree to disagree on whether the logic should be here, because you've already written it anyway. I would ask though that this logic be re-written without the nested lambdas and multiple conditions that make it much harder to read (and probably debug). Having a separate method for this logic, that does an old fashioned foreach loop, and has a few guard conditions that just return false, would improve this a lot I think.

[Peter-Juhasz]

I fixed the whitespace issues in the resource file, but there is still an error in the build pipeline, possibly the missing translations.

Shall I check in the result of msbuild /t:UpdateXlf as well? (updated .xlf files)

[ryzngard]

I fixed the whitespace issues in the resource file, but there is still an error in the build pipeline, possibly the missing translations.

Shall I check in the result of msbuild /t:UpdateXlf as well? (updated .xlf files)

yup, that should fix. Or if you build in VS it should generate them as well

[davidwengier]

Thanks for the PR, I'll take a look later this afternoon. Please try not to force push any more now the PR is open though, it makes it much harder for us to keep track of changes as feedback is responded to.

For other reviewers: it was my suggestion to target the release branch, as that's the easiest to test with for now. Once everything is signed off, we can change the target to main, and deal with any conflicts and merge.

[davidwengier]

This is really good! Thanks for the contribution.

Just had some thoughts about perhaps simplifying the logic and maybe doing a little less work in tooling, but rather leaning on the compiler a bit more. Curious for your thoughts on it.

[davidwengier]

It's not clear to me why we're explicitly checking for RenderFragment anyway. Why are they important? I would have thought the more important thing to check for was ChildContent.

eg, given a component with a random render fragment property "Foo", if its required and not supplied, we can check for diagnostics, if its not required and not supplied then it doesn't matter, and if its not required and supplied, then it will be non-whitespace characters in the tag, so will already be filtered out.

ChildContent though, we need to check for because something this:

<Goo>

</Goo>

That could be a component with no content, or, if there is a ChildContent property, it could be a component with ChildContent set to some whitespace, which we probably want to keep.

[Peter-Juhasz]

🌴 FYI: I will be traveling in the rest of this week, and while I can read/reply comments, I won't be close to my desktop workstation to be able to make any meaningful changes. Will be back on Monday next week.

Feel free to make any changes if needed to proceed with this PR until then.

[ryzngard]

🌴 FYI: I will be traveling in the rest of this week, and while I can read/reply comments, I won't be close to my desktop workstation to be able to make any meaningful changes. Will be back on Monday next week.

Feel free to make any changes if needed to proceed with this PR until then.

Will do. Thanks @Peter-Juhasz ! Love to see both of your contributions. Have safe travels

[davidwengier]

Had another look and other than a couple of relatively minor (and most readability) tweaks, this is really good.

No rush on the follow up though :)

[davidwengier]

My previous comments about this not being necessary were wrong, I think I missed that you were iterating through BoundAttributes and not the actual attribute syntax nodes.

It might still be nice though to move this attribute name parsing to a helper method so it least it only has to be written once. Perhaps a new method in RazorSyntaxFacts?

So it would be if (!markupElementSyntax.TagHelperInfo!.BindingResult.Attributes.Any(a => attribute.Name == RazorSyntaxFacts.GetAttributeNameForAttribute(a.Key)) or similar?

[DustinCampbell]

FYI that @dotnet/razor-compiler also needs to take a look, since this PR adds a method in the Compiler layer.

[DustinCampbell]

@phil-allen-msft: Does this need any word-smithing?

[Peter-Juhasz]

@DustinCampbell I appreciate your feedback, but I think I need some guidance interpreting the severities.

While I totally agree and you are technically correct on each one and don't get me wrong, I'm not against "perfect" code, but suggestions which do not change meaning, like:

• fix whitespace formatting
• remove unnecessary parenthesis for parameterless constructor
• seal an internal class
• invert condition (is it subjective taste of reviewer or shall we optimize for least amount of operations?)

Are these suggestions or blockers? I'm happy to fix them all, I'm just curious about the severity when I see findings like these.

In any case, are there any analyzers I forgot to enable locally to shorten the feedback loop? Thanks.

[davidwengier]

Whitespace formatting would be a compile error if it violated IDE0055, and any suggested whitespace changes where there wasn't a violation are pretty rare, or would have introduced a violation. The issue with that one pattern that Dustin and I both commented on is the latter, and a Roslyn bug that I haven't got around to logging yet.

In any case, are there any analyzers I forgot to enable locally to shorten the feedback loop?

If there were any that had to be manually enabled, then that would be a bug on us not setting up our dev environment correctly.

[DustinCampbell]

@DustinCampbell I appreciate your feedback, but I think I need some guidance interpreting the severities.

While I totally agree and you are technically correct on each one and don't get me wrong, I'm not against "perfect" code, but suggestions which do not change meaning, like:

• fix whitespace formatting
• remove unnecessary parenthesis for parameterless constructor
• seal an internal class
• invert condition (is it subjective taste of reviewer or shall we optimize for least amount of operations?)

Are these suggestions or blockers? I'm happy to fix them all, I'm just curious about the severity when I see findings like these.

In any case, are there any analyzers I forgot to enable locally to shorten the feedback loop? Thanks.

@Peter-Juhasz: I've left feedback that I would leave for any Razor team member submitting a pull request. My goal is to ensure consistency and readability across the code base,, which is admittedly a work-in-progress. 😄 You are free to ignore my comments that you feel are burdensome, especially if you're not planning to push any other changes. However, if you're planning to push more changes, I can't see a reason why you wouldn't make any of my suggested changes.