Add a Roslyn analyzer to detect parameter/attribute issues#9031
Add a Roslyn analyzer to detect parameter/attribute issues#9031henon merged 28 commits intoMudBlazor:devfrom
Conversation
github-actions
bot
added
enhancement
|
Hi. Excellent work! The Item after the Items or the Inline in MudText.
I have a couple of questions:
|
|
The illegal parameter detection for first-party components is a neat evolution of the current one that throws at runtime. I think that detecting possibly bad attributes on MudBlazor components is shaky, but detecting them on other controls as well way oversteps the bounds of the library. Maybe if it was opt-in but then I imagine very few people would and there's a large maintenance cost. Same issue with a separate package. I might have misunderstood the proposal though. Unit tests are crucial when it's so potentially disruptive to a user's build. Other risk I can think of is whether or not anyone who is active on MudBlazor is proficient in analyzers to be able to fix things that come up. |
Maybe @peterthorpe81 wants to join the team? Then this risk would be smaller, but we have this with everything. Team members have been known to abandon us. Others will come and replace them eventually. Of course this speaks for the need of tests. |
|
Actually, the tests in meziantou's repo don't look complicated at all. Basically he builds a c# source snippet, or (in our case) a razor component and executes the analyzer on it. [Theory]
[InlineData("Param1")]
[InlineData("Param2")]
public async Task ValidParameterName(string parameterName)
{
var sourceCode = $$"""
class TypeName : ComponentBase
{
protected override void BuildRenderTree(Microsoft.AspNetCore.Components.Rendering.RenderTreeBuilder __builder)
{
__builder.OpenComponent<SampleComponent>(0);
__builder.AddAttribute(1, "{{parameterName}}", "test");
__builder.CloseComponent();
}
}
""";
await CreateProjectBuilder()
.WithSourceCode(Usings + sourceCode + ComponentWithParameters)
.ValidateAsync();
}But maybe I don't understand yet where the difficulties really lie. |
Its been quite an interesting little project to get an analyzer working. Razor source gen adds some complexity.
|
I probably didn't explain bad attributes well. Its only applied to inheritors of MudComponentBase so should be the core library and then I guess if anyone inherits from a component which I guess is questionable. The main point for me was to detect silly mistakes like typos in parameters which then just go in as html attributes with splatting enabled. You can configure it so the current default is to allow through any lower case (first letter) attributes. That would seem to make sense as html attributes should be lower case and parameters normally begin with upper case. You can currently configure it to allow Any, DataAndAria, None extra attributes. I will take another look at tests |
I will take another look at tests i'm not particularly experienced with BUnit so perhaps was a bit quick to dismiss testability. |
bUnit is not used there. And I don't think it's necessary. bUnit is for parsing components and rendering them. Here, you just need to create the final generated Razor code and run it through the analyzer, just like in @henon's example.
I heard |
|
Hi, sorry for pushing this, but do you have any ETA for when you can look at the testing? We plan to make RC1 soon, and it would be nice if this could be shipped with RC. |
|
Hi! |
Yeah sorry I have been a bit quite about it. I had a good look into testing. The thing about Meziantou's I wasn't keen on for Blazor was it creates a project in code adds references to nugets... adds the source generated razor component from a string version and then compiles it and runs the analyzers. It's quite code heavy and would need quite a bit of maintenance as things change. Blazor wise that also means it bypasses the actual source generator. This also makes complex tests cumbersome as things like inheritance mean adding lots of stringified classes. Sonarsource actually creates a physical project with the test components in and compiles it and analyzes it meaning you can actually use .razor files. Something I am doing (or attempting to) that Meziantou's doesn't is map the source error back to the .razor file so this seems easier to maintain and test. The test files could technically sit in MudBlazor.UnitTests but will be faster in their own small project. When I got to testing I found my line mapping (source -> .razor) didn't work in all cases as ##line indicators aren't what I expected for certain circumstances (so its good to test :-)). I am looking at a different way to do this at the moment but if I don't crack it by the weekend I will release a version with tests minus the line mapping. The generator could either report to the source file or the top of the .razor file. |
|
That sounds like an interesting solution. I have made some tests using the Meziantou's version in my repo. I can agree that is is somewhat cumbersome to have the razor files as strings, but they aren't really dependent on MudBlazor code in the current form, so changes should be somewhat decoupled from the base repo (Good/Bad?). |
|
Yes it will fail on debug build but not release as I configured it for MudBlazor and MudBlazor.Docs to only run on debug build. I left the warnings in originally as it was a good demonstration of what its doing. Do you want me to add the fixes to the PR for these warnings? They appear as errors because of TreatWarningsAsErrors setting. The bulk of the fixes are changing the casing so attributes are lower case e.g. Title (formerly a parameter) to title. There are a couple of old/renamed parameters in the mix too like Sortable on MudDataGrid. I'm not getting an error on Inline locally is this an old result? The line in question does look odd though so the Inline parameter is there but value less: |
Yes, this is HTML logic, parameters without value are set to
I don't think so. We use it in many locations in the source base and the examples. Does it pose a problem to the analyzer?
When this is merged everyone working on a PR in debug mode will get the errors. That would cause issues and questions to pop up how to deal with them, so in my opinion we'll have to fix them as part of this PR. What do you think @ScarletKuro ? |
Yes, as I said I posted a month old log. Inline was re-added in #9065
Yes, I think it's better if we fix it in this PR. Maybe we could even enable it for all modes, but up to you @henon |
|
I resently saw an example of how to use conditional compilation in razor using @peterthorpe81 Is there an easy way to disable it for people who don't want it or have issues with it? |
We discussed it and I think @peterthorpe81 added this feature and there even a doc page that explains the settings? |
| </MudFileUpload> | ||
|
|
||
| <MudFileUpload T="IReadOnlyList<IBrowserFile>" Multiple> | ||
| <MudFileUpload T="IReadOnlyList<IBrowserFile>" multiple> |
There was a problem hiding this comment.
@Mr-Technician @igotinfected is this even required thing "multiple"? I think it's just an old code when there was the InputFile, but I feel like MudFileUpload doesn't require it?
|
Added the fixes to the PR. The analyzer will now run for release and debug builds of MudBlazor. This is the page in the docs about how to configure/disable the analyzer. The diagnostics include this as the link url like when you get erros that point to Microsoft pages.
|
henon
left a comment
henon
left a comment
There was a problem hiding this comment.
Awesome. This is a legendary PR and a candidate for the MudBlazor Contributions Hall of Fame which I just made up ;)
henon
added
legendary
|
I noticed that after this PR, the <ItemGroup>
<None Include="$(OutputPath)\net7.0\$(AssemblyName).Analyzers.dll" Pack="true" PackagePath="analyzers/dotnet/cs/" Visible="true" />
<Content Include="..\Directory.Build.targets" Pack="true" PackagePath="build\$(PackageId).targets" Visible="true" />
</ItemGroup>I guess this is a similar problem: Stack Overflow link. @peterthorpe81, it would be great if you could fix this. |
|
Never mind, this |
I hadn't noticed that, I have changed the way its made transitive with this PR #9229 Its similar to the stackoverflow fix using TargetsForTfmSpecificContentInPackage. Because MudBlazor is multi targeted it would try to copy the files in for each framework version which errors because the analyzer needs to sit as a .net standard 2.0 dll in analyzers/dotnet/cs/. I have limited the item group to .net 8.0 so it only does one copy. |
|
this is awesome/ this should be posted imho somewhere in installation guide and included in default mudblazor templates imho. it helped alot in migration to v7 thanks. |
Hi. |
|
ok i used older maybe in 2.0 |
This analyzer is part of the MudBlazor assembly, and no additional settings or libraries are needed for it to start working. Below is an example from a freshly created MudBlazorWebApp using the new templates.
I added |
|
@peterthorpe81 do we need to update the v8 the https://github.com/MudBlazor/MudBlazor/blob/dev/src/MudBlazor.Analyzers/IllegalParameterSet.cs
|
I have done a PR for this #10644 |
7 participants
This PR adds an analyzer to MudBlazor to detect issues with v7 parameters at compile time (unless its dynamic code). It also adds a similar analyzer to specifically not allow unknown attributes. Its originally based on https://github.com/meziantou/Meziantou.Analyzer.
The analyzer has been added to the projects in a specific way so that it applies to MudBlazor.Docs and transitively to any project that uses MudBlazor nuget package There is also the potential to have it as a separate nuget project if deemed more appropriate.
Description
The analyzers will emit two warnings:
MUD0001 - Detects parameters in the specific V7 list of removed/renamed parameters
MUD0002 - Identifies parameters/attributes that don't appear to be appropriate for the component e.g. a typo
The Analyzers can be configured within a .csproj by adding this section:
MudAllowedAttributePattern supports the options:
MudIllegalParameters support the options:
Notes
How Has This Been Tested?
Visually tested with MudBlazor docs and personal projects. Unit tests may be possible as https://github.com/meziantou/Meziantou.Analyzer has some using XUnit but it does appear to be quite complicated.
Type of Changes
Checklist
dev).