dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.3k stars 4.74k forks source link

Generated code should include [GeneratedCodeAttribute] and // <auto-generated/> #64541

Open danmoseley opened 2 years ago

danmoseley commented 2 years ago

Logging, Interop, Regex and JSON generators include // <auto-generated/> at the top of their emitted .g.cs. EventSource generator does not.

Logging, Regex and JSON generators include something like [global::System.CodeDom.Compiler.GeneratedCodeAttribute("Microsoft.Extensions.Logging.Generators", "42.42.42.42")] EventSource and Interop generators do not.

After https://github.com/dotnet/runtime/pull/64534, all 5 of them use .g.cs extensions when writing to disk.

@jaredpar is it reasonable that all three of these should be required by convention in generators? I'm not sure what effect each one has. For example, we discovered that coverlet looks for .g.cs.

ghost commented 2 years ago

Tagging subscribers to this area: @dotnet/area-meta See info in area-owners.md if you want to be subscribed.

Issue Details
Logging, Interop, Regex and JSON generators include `// ` at the top of their emitted .g.cs. EventSource generator does not. Logging, Regex and JSON generators include something like ` [global::System.CodeDom.Compiler.GeneratedCodeAttribute("Microsoft.Extensions.Logging.Generators", "42.42.42.42")]` EventSource and Interop generators do not. After https://github.com/dotnet/runtime/pull/64534, all 5 of them use `.g.cs` extensions when writing to disk. @jaredpar is it reasonable that all three of these should be required by convention in generators? I'm not sure what effect each one has. For example, we discovered that coverlet looks for `.g.cs`.
Author: danmoseley
Assignees: -
Labels: `area-Meta`, `source-generator`
Milestone: -
jaredpar commented 2 years ago

I'm not sure what effect each one has. For example, we discovered that coverlet looks for .g.cs.

That seems like a bug in coverlet, not something we should be designing our generators around.

[global::System.CodeDom.Compiler.GeneratedCodeAttribute("Microsoft.Extensions.Logging.Generators", "42.42.42.42")]

This has no impact on code that I am aware of.

stephentoub commented 2 years ago

That seems like a bug in coverlet, not something we should be designing our generators around.

I agree. In the case of https://github.com/dotnet/runtime/pull/64534 I was fine "fixing" it because we already use ".g.cs" everywhere else and this was the one outlier, with no good reason, so changing it made it more consistent with little downside. But if coverlet actually breaks if someone uses a different suffix (or no suffix) with a source generator, that certainly seems like a coverlet bug.

is it reasonable that all three of these should be required by convention in generators?

From my perspective, the <auto-generated/> is valuable in source to highlight to someone reading the source that trying to change it will be futile as it will be regenerated, the .g.cs suffix is valuable for someone looking at files on disk to know what's been created by a tool rather than by hand, and [GeneratedCode] is useful for tools that inspect metadata.

danmoseley commented 2 years ago

Does VS respect both <auto-generated/> and [GeneratedCode]?

Coverlet aside, consistency is good; we should try to be consistent across our own generators, and if these two annotations have value, we should likely recommend them as well.

@dotnet/interop-contrib do you want me to open a specific issue for your generator to include GeneratedCodeAttribute, like the others?

jaredpar commented 2 years ago

Does VS respect both and [GeneratedCode]?

Can you be more specefic in what you expect from VS here? There are many layers of VS and, unfortunately, there are differing heuristics for detecting and different responses to generated code

danmoseley commented 2 years ago

I meant "are there components of VS that look for one but not the other, such that it is necessary to apply both for all components to identify generated code"?

For example, MSBuild writes XX.AssemblyInfo.cs with

//------------------------------------------------------------------------------
// <auto-generated>
//     This code was generated by a tool.
//
//     Changes to this file may cause incorrect behavior and will be lost if
//     the code is regenerated.
// </auto-generated>
//------------------------------------------------------------------------------

but no [GeneratedCode] attribute. WPF's XAML code behind files have that plus [System.CodeDom.Compiler.GeneratedCodeAttribute("PresentationBuildTasks", "7.0.0.0")] (and end in '.g.cs' incidentally)

Yes, I wasn't intending to suggest we should cater to Coverlet's bugs. The extension would purely have the value of consistency for humans.

jaredpar commented 2 years ago

I meant "are there components of VS that look for one but not the other, such that it is necessary to apply both for all components to identify generated code"?

Yes. Across VS there are different heuristics for identifying generated code. The one we are trying to standardize on is the compiler heuristic around generated code.

https://sourceroslyn.io/#Microsoft.CodeAnalysis/InternalUtilities/GeneratedCodeUtilities.cs,1c5dbec99946c19e,references

This takes into account several popular existing hueristics, including extensions. That is genenerally not a great approach. The one we'd prefer is that people have the <auto-generated> header.

AaronRobinsonMSFT commented 2 years ago

@dotnet/interop-contrib do you want me to open a specific issue for your generator to include GeneratedCodeAttribute, like the others?

We have been discussing this in our team meeting. Adding a tracking issue would be helpful and adding it to the productization issue - https://github.com/dotnet/runtime/issues/60595.

danmoseley commented 2 years ago

This takes into account several popular existing hueristics, including extensions. That is genenerally not a great approach. The one we'd prefer is that people have the <auto-generated> header.

@stephentoub are you aware of tools in the ecosystem that only respect [GeneratedCode] such that we can't realistically just use <auto-generated> ? (I have zero opinion just trying to record consensus here)

stephentoub commented 2 years ago

@stephentoub are you aware of tools in the ecosystem that only respect [GeneratedCode]

Anything that only has access to binaries / metadata / reflection and not source, e.g. https://grep.app/search?q=%3CGeneratedCodeAttribute%3E https://grep.app/search?q=typeof%28GeneratedCodeAttribute%29

stephentoub commented 2 years ago

(In particular, I think it's important for source generated code to include [GeneratedCode(version)] so if there are any discovered bugs in the generator, binaries can be more easily audited to understand which ones are likely impacted.)

danmoseley commented 2 years ago

@jaredpar if you agree, where should guidance go - in https://docs.microsoft.com/en-us/dotnet/csharp/roslyn-sdk/source-generators-overview? and perhaps samples?

Guiorgy commented 10 months ago

I only came across the GeneratedCodeAttribute attribute while reviewing some generator sources.