Add several new analyzers #603
Conversation
# Conflicts: # doc/analyzers/index.md # src/Microsoft.VisualStudio.Composition.Analyzers/AnalyzerReleases.Unshipped.md # src/Microsoft.VisualStudio.Composition.Analyzers/README.md # src/Microsoft.VisualStudio.Composition.Analyzers/Strings.Designer.cs # src/Microsoft.VisualStudio.Composition.Analyzers/Strings.resx
| using System.ComponentModel.Composition; | ||
| [Export] | ||
| [method: ImportingConstructor] |
There was a problem hiding this comment.
TIL: I had no idea how to apply an attribute to a primary constructor, and copilot had told me there wasn't a way.
| [Export] | ||
| class Foo | ||
| { | ||
| public {|#0:Foo|}(string parameter) |
There was a problem hiding this comment.
💡 Make it easier on yourself by using this syntax:
| public {|#0:Foo|}(string parameter) | |
| public {|VSMEF004:Foo|}(string parameter) |
Then you don't have to do the "expected" stuff in your assertion area. This would be enough:
await VerifyCS.VerifyAnalyzerAsync(test);Although then I guess you lose the assertion about the formatting argument.
| [Export] | ||
| public static string StaticField = "test"; | ||
| [Export] |
There was a problem hiding this comment.
📖 Information you wish you didn't know: the VS editor defines exports like these for their metadata. They might be literally non-activatable, yet useful because the editor only wants the metadata.
But that's a very unusual case and I fully support a (suppressible) warning diagnostic as you have here.
| [Export] | ||
| class Service | ||
| { | ||
| [System.Composition.ImportingConstructor] |
There was a problem hiding this comment.
The code fix should apply the Simplifier and/or ImportUsing (or whatever it's called) annotation so that Roslyn removes this redundant qualifier.
This example shows both how to annotate all the qualified names you emit and how to apply the modification based on those annotations.
https://github.com/AArnott/Nerdbank.MessagePack/blob/bfc5c068337baf693dc9aea6a4f50a94fa6b43fe/src/Nerdbank.MessagePack.Analyzers.CodeFixes/MigrationCodeFix.cs#L144-L153
| await VerifyCS.VerifyCodeFixAsync(testCode, expected, fixedCode); | ||
| } | ||
|
|
||
| // TODO: Add test for second code action (parameterless constructor) when framework supports testing multiple code actions |
There was a problem hiding this comment.
Can we get a sample test added (with Skip=true) to demonstrate what this would look like?
By the way, I think the framework already supports multiple code actions. Here's a sample:
https://github.com/AArnott/Nerdbank.MessagePack/blob/bfc5c068337baf693dc9aea6a4f50a94fa6b43fe/test/Nerdbank.MessagePack.Analyzers.Tests/MigrationAnalyzerTests.cs#L130
and how it works:
https://github.com/AArnott/Nerdbank.MessagePack/blob/bfc5c068337baf693dc9aea6a4f50a94fa6b43fe/test/Nerdbank.MessagePack.Analyzers.Tests/MigrationAnalyzerTests.cs#L436-L439
| { | ||
| var symbol = (INamedTypeSymbol)context.Symbol; | ||
|
|
||
| // Only analyze classes |
There was a problem hiding this comment.
why not structs? They can have too many importing constructors, can't they?
| foreach (var parameter in method.Parameters) | ||
| { | ||
| var importAttribute = Utils.GetImportAttribute(parameter.GetAttributes()); | ||
| if (importAttribute is not null) |
There was a problem hiding this comment.
An importing constructor parameter is still an import even if it lacks this attribute. Is this a blindspot in the analyzer?
| } | ||
| } | ||
|
|
||
| private static string? GetImportContract(AttributeData importAttribute, ITypeSymbol importType) |
There was a problem hiding this comment.
It's actually far more complicated than this (see ContractNameServices), but since analyzers can't reference the library and I wouldn't want to reproduce all that logic here, a rough approximation is fine since it's not about what the contract name is exactly as much as just identifying duplicates. But for clarity, that's probably worth commenting here.
| /// <summary> | ||
| /// Provides code fixes for VSMEF004: Exported type missing importing constructor. | ||
| /// </summary> | ||
| [ExportCodeFixProvider(LanguageNames.CSharp, Name = nameof(VSMEF004ExportWithoutImportingConstructorCodeFixProvider))] |
There was a problem hiding this comment.
What does the Name property on this attribute do? I've never bothered setting it.
| return document.WithSyntaxRoot(newRoot); | ||
| } | ||
|
|
||
| private static MefVersion DetermineMefVersion(ClassDeclarationSyntax classDeclaration, SemanticModel semanticModel) |
There was a problem hiding this comment.
Remember that a MEF part may have no attributes from either of these namespaces at all, if the only MEF attributes it has are custom ones derived from an ExportAttribute.
Fixes #257
Fixes #287
Fixes #317
New analyzers for:
[ImportingConstructor][ImportingConstructor][Import]AllowDefaultMostly coded using GitHub Copilot.