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

Add coverage for analyzer violation BL0007 #35161

Merged
merged 4 commits into from
Apr 7, 2025
Merged

Conversation

guardrex
Copy link
Collaborator

@guardrex guardrex commented Apr 7, 2025

Fixes #35159

Thanks @jfoshee! 🚀 ... I had time this morning to resolve this. I'm going to slant this in the direction that it isn't safe to suppress the warning. We guide devs in the Blazor docs not to write to component parameters directly with a setter. I'm including some improvements to that coverage as part of this PR, and you can see that part of the Component parameters section (and will say with the changes) on the diff. I'll leave this up for a few hours before merging to give you a chance to take a look at the new article.


Internal previews

📄 File 🔗 Preview link
aspnetcore/blazor/components/index.md aspnetcore/blazor/components/index
aspnetcore/diagnostics/bl0001.md aspnetcore/diagnostics/bl0001
aspnetcore/diagnostics/bl0002.md aspnetcore/diagnostics/bl0002
aspnetcore/diagnostics/bl0003.md aspnetcore/diagnostics/bl0003
aspnetcore/diagnostics/bl0004.md aspnetcore/diagnostics/bl0004
aspnetcore/diagnostics/bl0005.md aspnetcore/diagnostics/bl0005
aspnetcore/diagnostics/bl0006.md aspnetcore/diagnostics/bl0006
aspnetcore/diagnostics/bl0007.md aspnetcore/diagnostics/bl0007
aspnetcore/diagnostics/code-analysis.md aspnetcore/diagnostics/code-analysis
aspnetcore/toc.yml aspnetcore/toc

@guardrex guardrex self-assigned this Apr 7, 2025
@jfoshee
Copy link
Contributor

jfoshee commented Apr 7, 2025

Thanks @guardrex for the quick attention!

The "How to Fix" section looks good (as far as my understanding goes).

The "Rule Description" still leaves me with questions. It seems to be saying that the problem is not in defining a custom setter, but the problem is in invoking the custom setter?

I have a sneaking suspicion that the analyzer itself is too blunt. If I implement a custom setter that:

  • I don't manually invoke from anywhere
  • Does not call any lifecycle methods

Then I don't see the problem (in theory or practice).

For example, is this problematic?

    [Parameter, EditorRequired]
     public Visit Visit
     {
         get => _visit ?? new Visit();
         set
         {
             _visit = value;
             procedures = _visit?.Procedures?.ToList() ?? new List<Procedure>();
         }
     }

I suppose it is not our job to fix the analyzer in the context of docs. But, I wanted to raise my confusion.

Thanks for your attention. :-)

@guardrex
Copy link
Collaborator Author

guardrex commented Apr 7, 2025

If you define it, it will be invoked if set, so there's no difference really. The problem mostly seems to be that the framework interacts with the parameter when set (and possibly get, too) internally in ways that result in the undesirable behaviors. The team is broadly telling devs that they don't recommend anything but auto properties for component parameters and to use the lifecycle method if they want to interact with the value at all, including probably even reading the value.

I do want to change this explanation. I'm going to generalize it further to cover get and set just in case. I'll issue a commit shortly.

@guardrex
Copy link
Collaborator Author

guardrex commented Apr 7, 2025

Ok, that generalizes it to match what they had me publish in the article.

If you want to discuss this further with them, open an issue on their repo at ...

https://github.com/dotnet/aspnetcore/issues

Please add ...

cc: @guardrex https://github.com/dotnet/AspNetCore.Docs/pull/35161

... to the bottom of your opening comment so that I can follow along. I might open a new issue and make further modifications based on what they say.

@guardrex
Copy link
Collaborator Author

guardrex commented Apr 7, 2025

Ok ... this looks good (for now). If you open an issue for them, I'll keep an 👁️ on the discussion for possibly further work on this.

Thanks again for the issue!

@guardrex guardrex merged commit d5fb626 into main Apr 7, 2025
3 checks passed
@guardrex guardrex deleted the guardrex/document-BL0007 branch April 7, 2025 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Need documentation on BL0007
2 participants