Add missing [Generator] attribute analyzer and code fix

[ryzngard]

Fixes #5050

Adds an analyzer and code fix to ensure that types that implement ISourceGenerator also have the [Generator] attribute. Works in both C# and Visual Basic.

[Youssef1313]

Note: The parameterless attribute constructor makes the generator C#-only. I'd instead make the codefix add both languages, and let the user delete an extra language if needed.

[ryzngard]

I'm not actually sure if that's enforced. I didn't find anywhere it was referenced in roslyn. @chsienki might know more.

I don't think we want to add both, I doubt that's correct in most cases. If anything, maybe use the document language

[ryzngard]

Ah nm, it's used indirectly https://sourceroslyn.io/#Microsoft.CodeAnalysis/DiagnosticAnalyzer/AnalyzerFileReference.cs,60

[Youssef1313]

@ryzngard Using the document language isn't ideal IMO. The source generator may be meant for usage for both languages, while being written in C#. Since the attribute application doesn't explicitly specify a language version, this could be hard to catch for developers.

There are few options I can come up with:

• Offer 3 codefixes (C# - VB - Both)
• Heuristic 1:
  • Look at the namespaces referenced in using statements if they are contain C#-specific, VB-specific, or both. In case there are none, I'd fall back to "both" and let the developer adjust the attribute.
• Heuristic 2:
  • Look at the type name if it contains "CSharp" or "VB" (or "VisualBasic"), and fall back to both languages
• Heuristic 3:
  • Combines both above.

Simplest is probably the first option to offer 3 codefixes.

[chsienki]

I think for now it will be fine to leave it with the default value. Ultimately a C# generator targeting a C# only project is the most likely scenario. We could put the lang parameter in and default it to C# as a hint that you can change it if people feel strongly (or as @ryzngard said, use the document language), but this feels like we're probably just over engineering something that we can address down the line if we get customer feedback about it.

[ryzngard]

While true, I think that's overengineering for the problem space right now. We also don't catch cases where the user puts the wrong language with the analyzer... we just don't know. During compilation I'm sure the generator will fail if the attribute is configured incorrectly. Hopefully whatever error happens is enough to help the author figure out next steps. Having the code fix as is does exactly what the analyzer catches, no more and no less. Having some hard to follow heuristics is unwarranted until we have a stronger user opinion on the matter.

[sharwell]

We should use the approach from ApplyDiagnosticAnalyzerAttributeFix for consistency.

[Evangelink]

I don't recall the name but I think there is an helper function for this that reads better.

[Youssef1313]

Sorry I think I was incorrect here. The constant value is "C#", it doesn't match the field constant name ("CSharp").

[ryzngard]

You know, I trusted you Youssef. 😑

I'll get this tomorrow as well when it's not bedtime 😉

[Youssef1313]

Sorry again.

Actually, even the UICulture suggestion is failing CI (I don't quite understand why). But the failure is for the good this time. If it's wrong to use UICulture for string.Format, then this should get updated and the analyzer should be enabled in roslyn:

https://github.com/dotnet/roslyn/blob/b88948f10a1085980df7080f0b765b1a8c614436/src/Compilers/CSharp/Portable/Compiler/DocumentationCommentCompiler.cs#L289

[ryzngard]

I was just jesting, it's easy to make that mistake. I often forget that "C#" is what the string is.

As for UICulture, I'll look into that. I would expect it to be correct, but it's possible not formatting for culture at all is more correct in these cases.

[ryzngard]

src\Microsoft.CodeAnalysis.Analyzers\Core\MetaAnalyzers\Fixers\SourceGeneratorAttributeAnalyzerFix.cs(38,21): error CA1305: (NETCORE_ENGINEERING_TELEMETRY=Build) The behavior of 'string.Format(string, object)' could vary based on the current user's locale settings. Replace this call in 'SourceGeneratorAttributeAnalyzerFix.RegisterCodeFixesAsync(CodeFixContext)' with a call to 'string.Format(IFormatProvider, string, params object[])'

Here it is... This seems to be disabled during build in VS though. @sharwell is the correct thing to do is pass CurrentCulture here?

[ryzngard]

Going through my old PRs. Apologies for dropping this on the floor :( PTAL if you get a moment.

[Evangelink]

We are not really consistent for naming of the code fix titles but I would avoid _X.

[ryzngard]

What is the way to distinguish between this and AddGeneratorAttribute_2?

[sharwell]

Maybe _OneLanguage and _TwoLanguages? Otherwise you could use the naming style from Roslyn here.

[codecov]

Codecov Report

Merging #5235 (469c749) into main (f471d33) will increase coverage by 0.00%.
The diff coverage is 97.29%.

@@           Coverage Diff            @@
##             main    #5235    +/-   ##
========================================
  Coverage   95.58%   95.58%            
========================================
  Files        1284     1287     +3     
  Lines      296834   297314   +480     
  Branches    18101    18122    +21     
========================================
+ Hits       283725   284191   +466     
- Misses      10670    10678     +8     
- Partials     2439     2445     +6     

[Youssef1313]

Looks like imports annotation isn't working as I was expecting (TestSimpleClass_FullyQualified_CSharp should have failed).

I think Roslyn can improve this by assuming the annotation is meant to any descendant nodes, but for now we can generate the type syntax ourselves and pass it to generator.Attribute similar to

roslyn-analyzers/src/Roslyn.Diagnostics.Analyzers/Core/ImportingConstructorShouldBeObsoleteCodeFixProvider.cs

Lines 86 to 91 in c314521

	var semanticModel = await document.GetSemanticModelAsync(cancellationToken).ConfigureAwait(false);
	var obsoleteAttributeSymbol = semanticModel.Compilation.GetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemObsoleteAttribute);
	if (obsoleteAttributeSymbol is null)
	{
	return document;
	}

roslyn-analyzers/src/Roslyn.Diagnostics.Analyzers/Core/ImportingConstructorShouldBeObsoleteCodeFixProvider.cs

Lines 103 to 104 in c314521

	var obsoleteAttribute = generator.Attribute(
	generator.TypeExpression(obsoleteAttributeSymbol).WithAddImportsAnnotation(),

Feel free if you don't want to make it in this PR.

[Youssef1313]

I think we need to extend this to IIncrementalGenerator

[ryzngard]

Closing old PR. This could be used for anyone moving forward to implement this