dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
18.71k stars 3.98k forks source link

Compile can succeed with generator being absent #52749

Open maryamariyan opened 3 years ago

maryamariyan commented 3 years ago

Given a certain attribute that is already present in the framework (e.g. LoggerMessageAttribute in Microsoft.Extensions.Logging) but in case the source generator does not run, then the user would have no ability to see a compile-time error indicating that (in the logging case) no method body was generated due to the generator not being present.

Example:

    partial class C
    {
        [LoggerMessage(...)]
        public static partial void M1(ILogger logger, LogLevel level);
    }

Link to https://github.com/dotnet/runtime/pull/51064#discussion_r612672390

CyrusNajmabadi commented 3 years ago

This is solvable by having that type come with an analyzer that checks this for you. Analyzers run after source generators, so it can validate that the SG ran.

jinujoseph commented 3 years ago

cc @chsienki @jasonmalinowski what would be best practice here ?

CyrusNajmabadi commented 3 years ago

I would think the above would have to be the solution. The above is legal code. It has to continue to compile as per hte rules of the language. If teh owner of that attribute wants it to not compile if the requisite SG code is not there, then an analyzer should be provided with that API that ensures that that's the case. :)

jasonmalinowski commented 3 years ago

Also @maryamariyan why did the generator not run? Is this a situation where the user can configure their projects in a way where that would be a problem, or just that some bug happened during development?

ericstj commented 3 years ago

I think we're more concerned about a case where the user was intentionally trying to use the code-generator but ran it in an environment that couldn't ensure the generator was passed to the compiler. For example: runtime compilation, build system that doesn't run all SDK targets, etc.

In such a case the user couldn't ensure an analyzer was passed to the compiler any more than they could the generator, so adding redundancy in an analyzer doesn't really help anything.

This is the first time we're adding a new asset type to the .NETCore framework that is required for completeness. Imagine systems out there that were replicating framework design time and didn't need to know about generators. I'd really like those systems to fail if someone gave them code that depended on code-generators without those generators being present.

sharwell commented 3 years ago

In such a case the user couldn't ensure an analyzer was passed to the compiler any more than they could the generator, so adding redundancy in an analyzer doesn't really help anything.

This is exactly the issue I was concerned about when I read this.

terrajobst commented 3 years ago

@ericstj

This is the first time we're adding a new asset type to the .NETCore framework that is required for completeness. Imagine systems out there that were replicating framework design time and didn't need to know about generators. I'd really like those systems to fail if someone gave them code that depended on code-generators without those generators being present.

My suggestion is: because this is the first time we're doing this, let's wait and see whether that's a concern that actually appears in the wild and root cause what happened.

My gut is that the problem won't be the that the generator wasn't passed to the compilation, but more that someone else, for example, defined the attribute local that the generator didn't recognized or made other mistakes in the authoring that prevented the generator from running or producing code.

ericstj commented 3 years ago

My suggestion is: because this is the first time we're doing this, let's wait and see whether that's a concern that actually appears in the wild and root cause what happened.

We can't go backwards from what we're doing now. Once the attribute is in the framework it's there forever. I was hoping we'd have a mechanism that was encapsulated within the generator for interacting with the generator. That way it either works, or it doesn't. There's no half-way.

defined the attribute local that the generator didn't recognized or made other mistakes in the authoring that prevented the generator from running or producing code.

Those would just be either bugs in the generator, or errors raised by the generator that tell the developer they've done something wrong. We need to hold generators to the same bar as the compiler or libraries. There should never be silent failure. There shouldn't be syntax that "sometimes works"

CyrusNajmabadi commented 3 years ago

There shouldn't be syntax that "sometimes works"

This is how partial had always worked though. If you fall to run whatever tool you have that generates the other part of the partial... Then you will just see that partial call get elided. It's not clear what is different now.

Can an analyzer just ship alongside this api?

ericstj commented 3 years ago

The difference here is the user is expressing that a generator fill in the partial method by applying that attribute. The problem is that the partial method will not be filled if the generator isn’t provided. An analyzer isn’t a solution because the analyzer has the same problem as the generator (they are specified in the same way). The analyzer cannot be provided by the same component which provides the API.

A possible solution would be for the generator to define the attribute. This doesn’t work well today because the generator persists the attribute definition in the user’s assembly. If a generator could define a compile time only attribute, suggested by @stephentoub, that would work.

Another possible solution, suggested by @layomia, is the attribute usage somehow “demands” the generator. Forcing a failure/warning if the generator was absent.

Another possible solution is for the generator to somehow add a unique reference that it carries within itself (eg: embedded resource + intellisense) which defines the attribute and ensures its reference is not persisted (Conditional).

CyrusNajmabadi commented 3 years ago

The analyzer cannot be provided by the same component which provides the API.

Can you expand on this piece? Why would that be? It would make a ton of sense to me that a component provide its own analyzers here. Thanks!

The difference here is the user is expressing that a generator fill in the partial method by applying that attribute. The problem is that the partial method will not be filled if the generator isn’t provided.

Sure... but that's true of any solution that uses partial methods. The idea was always that one part is provided optionally. It's always been the case that if you didn't provide the piece that provides that optional part then you just get the code elided.

ericstj commented 3 years ago

Can you expand on this piece? Why would that be? It would make a ton of sense to me that a component provide its own analyzers here.

Analyzers and references are separate parameters. You cannot guarantee both are present, even if they are the same assembly. A reference cannot say it “requires” an analyzer. If it could that would solve this (we’d require the generator though rather than an analyzer).

but that's true of any solution that uses partial methods

We aren’t claiming there is a problem with partial methods, they just happen to illustrate this limitation with source generators.

stephentoub commented 2 years ago

For proponents of an analyzer solution, what does that look like for languages that don't support Roslyn analyzers? We ship an attribute in the core libraries consumable by any language that targets those libraries, but the attribute can only meaningfully do its job when consumed by C#. We can also ship an analyzer that would flag incorrect use by C# and VB. What's the answer for F# or C++/CLI or any other .NET language?