dotnet / roslyn-analyzers

MIT License
1.6k stars 468 forks source link

Analyzers via attributes? #2580

Closed josh-degraw closed 5 years ago

josh-degraw commented 5 years ago

Analyzer package

Microsoft.CodeAnalysis.PublicApiAnalyzers and/or Microsoft.CodeAnalysis.BannedApiAnalyzers

I've searched around a little but haven't seen anything addressing this so far. I've used ReSharper which has gotten me used to some neat features, specifically attribute-based analysis, specifically [PublicAPIAttribute], which in that case (as far as I could tell) really only indicated that public members may be used externally and was more of a cosmetic nicety than anything.

I would love to use these analyzers, but having to keep track of symbols in a separate file is a lot of hassle, especially when working on larger projects, it's nice to just be able do specify on the object itself, and also saves time when trying to apply the analyzers to an existing project.

Are there any plans on adopting this sort of approach for these analyzers? And if not, is it something that you would be open to exploring?

mavasani commented 5 years ago

The core purpose of public API analyzers is to track the entire shipping public API surface of an assembly in a single place. This enables easy code reviews when one needs to look at a single file diffs to guage the public API changes in a PR. Additionally, one can look at the history of that file to see how the public API surface has evolved over time. Use of attribute based approach doesn't really seem to serve either of these purposes, and also serves as just an annotation of effectively accessibility of the symbol as opposed to declared accessibility. IMO that is not a big value add.

josh-degraw commented 5 years ago

I totally get that use case, and that makes a lot of sense. I think my main concern is that it's just a lot of manual work to set up for a large existing project, and the documentation is quite vague as to how exactly to do that (assuming I'm looking in the right place).

If an attribute-based approach is out of the question, I would think that some sort of build tool/script could be useful at least for applying this to new projects. I think it would be ideal to be able to specify on the types and/or member(s) that they are public using an attribute, and then a "code" generator could write to a file on build or something like that.

I guess the main hassle for me is how to declare this information. I like the idea of having seeing a single diff to see changes, but it's a lot easier to plop an attribute on something and let the build figure out the implementation details than it is to make a change in the file, then open the .txt file and paste in the thing I'm working on, and make sure I used the right syntax for it to work properly. It just feels like a lot more manual effort than it's worth at the moment to set this up with a large existing project, which bums me out because I love the idea of it and think it would help a lot with my current projects, it just seems like so much overhead to set up.

drewnoakes commented 5 years ago

@josh-degraw the analyzer keeps the txt file up to date for you so you don't have to edit it manually.

Steps to get this set up are:

  1. Add a package reference to Microsoft.CodeAnalysis.PublicApiAnalyzers
  2. Create files PublicAPI.Shipped.txt and PublicAPI.Unshipped.txt in your project
  3. Set those text file build actions to "C# analyzer additional file" (i.e. <AdditionalFiles> in csproj)

Once that's done, all public members will have warnings. You can then select "Add to public API / All in solution" to populate items in the "unshipped" file. You can manually move them to the "shipped" one if you like, though that's optional.

We might be able to have the analyzer automatically create the text files if they don't exist, so that you can skip steps 2 and 3 above.


For completeness, here's a project file that shows how this might look:

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>netcoreapp3.0</TargetFramework>
  </PropertyGroup>

  <ItemGroup>
    <None Remove="PublicAPI.Shipped.txt" />
    <None Remove="PublicAPI.Unshipped.txt" />
  </ItemGroup>

  <ItemGroup>
    <AdditionalFiles Include="PublicAPI.Shipped.txt" />
    <AdditionalFiles Include="PublicAPI.Unshipped.txt" />
  </ItemGroup>

  <ItemGroup>
    <PackageReference Include="Microsoft.CodeAnalysis.PublicApiAnalyzers" Version="2.9.3" />
  </ItemGroup>

</Project>
josh-degraw commented 5 years ago

@drewnoakes Thanks a lot for that clarification! That makes a lot more sense. Haha my main concern was having to manually update the file by hand, but it looks like this should do that just fine-- it might be good to make that aspect a little more clear in the docs, perhaps, or maybe I'm just blind and misunderstood lol. I definitely like the idea of the analyzer being able to do steps 2 and 3 above, but I this has helped out a lot and it makes way more sense now.

I'll go ahead and mark this issue resolved.

drewnoakes commented 5 years ago

There are two possible actionable items here:

  1. Make the analyzer capable of creating the missing files (#2622 created to track this)
  2. Update the documentation to make it clearer how to get started with the analyzer

@mavasani where is a good place to capture the above documentation to assist people wanting to get started with the analyzer?

mavasani commented 5 years ago

@genlu where would you prefer such a documentation?

genlu commented 5 years ago

Make the analyzer capable of creating the missing files (#2622 created to track this)

I like this :)

Update the documentation to make it clearer how to get started with the analyzer

we have some very basic doc here, which is linked from the auto generated md file does link to that doc. You need to click the diagnostic ID though, which might not be very obvious