dotnet / roslyn-analyzers

MIT License
1.58k stars 464 forks source link

Make PublicApiAnalyzers support C# files for API representation #3901

Open ericstj opened 4 years ago

ericstj commented 4 years ago

This is a feature request.

In thinking about how we can better represent API in dotnet/runtime while removing our custom tools and leveraging roslyn features we have found that it would be convenient to have this analyzer support C# files.

  1. We already represent API in C# files and it's most convenient for reviewers to view it this way.
  2. Some of our assemblies have differing assembly locations for ref vs def. We'd like to represent the API in a single C# file and implement it with mix of typeforwards and defs. By having this analyzer accept C# we could have it serve both purposes of building our reference assembly (which cannot use /refout, due to differing type locations) and performing our compatibility check.
  3. Presumably this feature would enable the analyzer or separate tool to emit C# which could eliminate the need to maintain our GenAPI tool.

/cc @Anipik @ViktorHofer @safern

jmarolf commented 4 years ago

@ericstj we support a SyntaxTree which are a representation of a C# file but with no knowledge of the semantics. If that is all you need we offer that today. If you need to resolve symbols we at least need a set of references. How do you envision this working in a file-only analyzer?

At a higher level, can you give me some background on what GenAPI does so I can understand how best to design the feature.

Nevermind, I believe you are asking for a PublicAPI.Shipped.cs be accepted by the PublicApiAnalyzer instead of PublicAPI.Shipped.txt

jmarolf commented 4 years ago

regardless this should filed on https://github.com/dotnet/roslyn-analyzers as that is where the PublicApiAnalyzer lives

ericstj commented 4 years ago

Sorry about that, could you transfer? I'll reply with more deatails.

sharwell commented 4 years ago

I investigated this issue and found a few complexities that make it non-trivial. What it really needs is a complete design, i.e. what exact syntax would the Shipped and Unshipped files have? It would be good to review alternate approaches (I believe I've found 4 different teams using completely different solutions in this space), and ensure the proposal allows for the eventual unification of the approaches.

ericstj commented 4 years ago

Nevermind, I believe you are asking for a PublicAPI.Shipped.cs be accepted by the PublicApiAnalyzer instead of PublicAPI.Shipped.txt

Exactly.

It would be nice if instead of text, it could permit one or more C# files that represent the API. For example: https://github.com/dotnet/runtime/blob/0bed2de8ed48e7d8ccb0bb7e6c878e22d0b2fa14/src/libraries/System.Text.Json/ref/System.Text.Json.cs

References used to resolve symbols would be the same as the actual compilation.

When an API was missing from the C# file or otherwise inconsistent it would be nice to be able to invoke a fix-it to update the C# file.

mavasani commented 4 years ago

When an API was missing from the C# file or otherwise inconsistent it would be nice to be able to invoke a fix-it to update the C# file.

NOTE: Just the fact that it is a C# file will not enable code fixes inside it. Only source files which are part of compilation as syntax tree that are part of output assembly support code fixes right now.

dougbu commented 4 years ago

This would be a great direction, especially because the current format does not seem to include important (compiler-consumed) information about the public API such as [AttributeUsage], [Obsolete], and [StructLayout] attributes. The analyzer can't catch attribute changes.

@jaredpar whatever the format, what are the most important attributes for the Public API analyzer to consider❔ Is there for example a canonical list of attributes Roslyn consumes from the reference assemblies it's passed❔

For dotnet/aspnetcore, we don't need C# that can be compiled but we do need a more-complete description of the API to (a) detect all real changes and (b) support API review discussions. Roslyn generates our ref/ assemblies and we would exclude PublicAPI.*.cs files from @(Compile) items everywhere. If dotnet/runtime could throw away GenAPI, all the better❕

/cc @pranavkm @davidfowl

drewnoakes commented 4 years ago

Agree that C# representation could be clearer and more expressive. It would better allow capturing the kinds of things mentioned in #3047.

Evangelink commented 3 years ago

Agree that C# representation could be clearer and more expressive. It would better allow capturing the kinds of things mentioned in #3047.

I wasn't particularly in favor of this but you are right that this might be the easiest way to capture all the required info. The only concern would be that if you have a vbnet codebase you will "have to" support a c# based syntax for your public API (that kinda sucks).

ericstj commented 3 years ago

Hopefully multiple languages could be supported, and there's always text as available today. I would say we should try to extend to C# while making it easy to also support VB, F#. Not sure how hard it would be to support all. I would hope Roslyn has the right level of abstraction that we could just transform them all to some common model for compatibility checks.

Evangelink commented 3 years ago

I have just noticed that https://github.com/reactiveui/ReactiveUI (see https://github.com/reactiveui/ReactiveUI/blob/main/src/ReactiveUI.Tests/Utilities/ApiApprovalBase.cs) is doing something similar using https://github.com/PublicApiGenerator/PublicApiGenerator. Although this is using a different technique I think it'd be nice to do something along these lines.

adamralph commented 3 years ago

I currently use https://github.com/PublicApiGenerator/PublicApiGenerator but I would like to switch to PublicApiAnalyzers, but right now the text representation (vs the C# representation using PublicApiGenerator) is the main drawback.

dougbu commented 3 years ago

It would better allow capturing the kinds of things mentioned in #3047

Would a C# representation also handle forwarded types better (#1192 and #1193)❔

safern commented 3 years ago

@dougbu that is a good point. I think it could handle forwarded types better indeed.

FYI, I'm driving this effort and just put out a design doc for this: https://github.com/dotnet/designs/pull/177 if you want to take a look