Add VSMEF002 analyzer to catch mixing MEF attributes on one type

doc/analyzers/VSMEF002.md

@@ -0,0 +1,117 @@
+# VSMEF002 Avoid mixing MEF attribute libraries
+
+Two varieties of MEF attributes exist, and are found in the following namespaces:
+
+- `System.ComponentModel.Composition` (aka MEF v1, or ".NET MEF")
+- `System.Composition` (aka MEF v2 or "NuGet MEF")
+
+An application or library must use the attributes that match the MEF engine used in the application.
+
+If the VS-MEF engine (Microsoft.VisualStudio.Composition) is used, both sets of attributes are allowed in the MEF catalog.
+During composition, the engine will create edges between MEF parts that use different attribute sets.
+
+It is important that only one variety of attributes be applied to any given MEF part (i.e. class or struct).
+MEF parts have one or more exporting attributes applied to them, and that MEF part will be defined in the catalog only with the imports that are defined using attributes from the same variety as the exporting attribute.
+As a result, exporting with one variety and importing with another will cause the MEF part to be defined in the catalog without any imports.
+
+Note that the focus on just one variety of MEF attributes is honored at the scope of a single type.
+Nested types are types in their own right and *may* use a different variety of MEF attributes than its containing type.
+
+## Example violations found by this analyzer
+
+### Simple example
+
+The following example makes the mix of attribute varieties clear:
+
+```cs
+[System.ComponentModel.Composition.Export]
+class Foo
+{
+    [System.Composition.Import]
+    public Bar Bar { get; set; }
+}
+```
+
+### Custom export attribute example
+
+Other equally problematic but more subtle violations are possible.
+Exporting attributes may be declared that derive from one variety but itself be defined in its own namespace.
+
+Consider the following custom export attribute, which happens to derive from the MEFv1 `ExportAttribute` yet lives in its own namespace:
+
+```cs
+using System.ComponentModel.Composition;
+
+namespace MyVendor;
+
+[AttributeUsage(AttributeTargets.Class, Inherited = false)]
+public class ExportFireAttribute : ExportAttribute
+{
+    public ExportFireAttribute()
+        : base(typeof(IFire))
+    { }
+}
+```
+
+Now consider a MEF part declared elsewhere that uses the above custom attribute:
+
+```cs
+using System.Composition;
+using MyVendor;
+
+namespace MyUser;
+
+[ExportFireAttribute]
+public class FastFire : IFire
+{
+    [Import]
+    public IWater Water { get; set; }
+}
+```
+
+The above MEF part is defective because it uses an export attribute that derives from the MEFv1 `ExportAttribute` but is implicitly using the mismatched MEFv2 `ImportAttribute` due to its `using` directive.
+
+## How to fix violations
+
+Consolidate on just one variety of MEF attributes for each MEF part.
+
+Note that each variety of MEF attributes have subtle but important semantic differences.
+For example, MEFv1 exported parts default to being instantiated once and shared with all importers,
+whereas MEFv2 exported parts default to being instantiated once per importer.
+
+The simple example above may be fixed by using just one namespace for all MEF attributes:
+
+```cs
+using System.ComponentModel.Composition;
+
+[Export]
+class Foo
+{
+    [Import]
+    public Bar Bar { get; set; }
+}
+```
+
+The other example that uses a custom export attribute may be fixed by verifying the variety that the custom attribute(s) belong to, and using that same variety for all other MEF attributes on that MEF part.
+
+```diff
+-using System.Composition;
++using System.ComponentModel.Composition;
+ using MyVendor;
+
+ namespace MyUser;
+
+ [ExportFireAttribute]
+ public class FastFire : IFire
+ {
+     [Import]
+     public IWater Water { get; set; }
+ }
+```
+
+## Very advanced scenarios
+
+You might be in a very advanced scenario where a single type intentionally uses both MEF attribute varieties.
+Such a case would presumably include at least `Export` attributes from both namespaces in order to define a MEF part in each world.
+In such a case, you can suppress this diagnostic on the type in question.
+You would need to manually confirm that each importing property has an `Import` attribute applied from both varieties.

src/Microsoft.VisualStudio.Composition.Analyzers/AnalyzerReleases.Shipped.md

@@ -1,3 +1,13 @@
-; Shipped analyzer releases
-; https://github.com/dotnet/roslyn-analyzers/blob/master/src/Microsoft.CodeAnalysis.Analyzers/ReleaseTrackingAnalyzers.Help.md
+## Release 17.10
 
+### New Rules
+Rule ID | Category | Severity | Notes
+--------|----------|----------|-------
+VSMEF001 | Usage | Error | VSMEF001PropertyMustHaveSetter
+
+## Release 17.11
+
+### New Rules
+Rule ID | Category | Severity | Notes
+--------|----------|----------|-------
+VSMEF002 | Usage | Warning | VSMEF002AvoidMixingAttributeLibraries

src/Microsoft.VisualStudio.Composition.Analyzers/AnalyzerReleases.Unshipped.md

@@ -4,4 +4,3 @@
 ### New Rules
 Rule ID | Category | Severity | Notes
 --------|----------|----------|-------
-VSMEF001 | Usage | Error | VSMEF001PropertyMustHaveSetter

src/Microsoft.VisualStudio.Composition.Analyzers/Microsoft.VisualStudio.Composition.Analyzers.csproj

@@ -1,4 +1,4 @@
-<Project Sdk="Microsoft.NET.Sdk">
+<Project Sdk="Microsoft.NET.Sdk">
 
   <PropertyGroup>
     <TargetFramework>netstandard2.0</TargetFramework>
@@ -35,6 +35,12 @@
     <None Update="tools\*.ps1" Pack="true" PackagePath="tools\" />
   </ItemGroup>
 
+  <ItemGroup>
+    <Using Include="Microsoft.CodeAnalysis" />
+    <Using Include="Microsoft.CodeAnalysis.Diagnostics" />
+    <Using Include="System.Collections.Immutable" />
+  </ItemGroup>
+
   <Target Name="PackBuildOutputs" DependsOnTargets="SatelliteDllsProjectOutputGroup;DebugSymbolsProjectOutputGroup">
     <ItemGroup>
       <TfmSpecificPackageFile Include="$(TargetPath)" PackagePath="analyzers\dotnet\" />

src/Microsoft.VisualStudio.Composition.Analyzers/Strings.resx

@@ -123,4 +123,10 @@
   <data name="VSMEF001_Title" xml:space="preserve">
     <value>Importing property must have setter</value>
   </data>
+  <data name="VSMEF002_MessageFormat" xml:space="preserve">
+    <value>The type "{0}" contains a mix of attributes from MEFv1 and MEFv2. Consolidate to just one variety of attributes.</value>
+  </data>
+  <data name="VSMEF002_Title" xml:space="preserve">
+    <value>Avoid mixing MEF attribute varieties</value>
+  </data>
 </root>

src/Microsoft.VisualStudio.Composition.Analyzers/VSMEF001PropertyMustHaveSetter.cs

@@ -3,13 +3,6 @@
 
 namespace Microsoft.VisualStudio.Composition.Analyzers
 {
-    using System;
-    using System.Collections.Immutable;
-    using System.Linq;
-    using Microsoft.CodeAnalysis;
-    using Microsoft.CodeAnalysis.CSharp;
-    using Microsoft.CodeAnalysis.Diagnostics;
-
     /// <summary>
     /// Creates a diagnostic when `[Import]` is applied to a property with no setter.
     /// </summary>

src/Microsoft.VisualStudio.Composition.Analyzers/VSMEF002AvoidMixingAttributeVarietiesAnalyzer.cs

@@ -0,0 +1,122 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT license. See LICENSE file in the project root for full license information.
+
+namespace Microsoft.VisualStudio.Composition.Analyzers;
+
+[DiagnosticAnalyzer(LanguageNames.CSharp, LanguageNames.VisualBasic)]
+public class VSMEF002AvoidMixingAttributeVarietiesAnalyzer : DiagnosticAnalyzer
+{
+    public const string Id = "VSMEF002";
+
+    internal static readonly DiagnosticDescriptor Descriptor = new DiagnosticDescriptor(
+        id: Id,
+        title: Strings.VSMEF002_Title,
+        messageFormat: Strings.VSMEF002_MessageFormat,
+        helpLinkUri: Utils.GetHelpLink(Id),
+        category: "Usage",
+        defaultSeverity: DiagnosticSeverity.Warning,
+        isEnabledByDefault: true);
+
+    private static readonly ImmutableArray<string> Mefv1AttributeNamespace = ImmutableArray.Create("System", "ComponentModel", "Composition");
+    private static readonly ImmutableArray<string> Mefv2AttributeNamespace = ImmutableArray.Create("System", "Composition");
+
+    public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics { get; } = ImmutableArray.Create(Descriptor);
+
+    public override void Initialize(AnalysisContext context)
+    {
+        context.EnableConcurrentExecution();
+        context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.Analyze | GeneratedCodeAnalysisFlags.ReportDiagnostics);
+
+        context.RegisterCompilationStartAction(context =>
+        {
+            // No need to scan attributes if the compilation doesn't reference both assemblies.
+            // Even in the case of custom MEF attributes being defined in another assembly,
+            // the C# compiler requires references to the assemblies that declare the base type of an attribute when the attribute is used.
+            bool mefV1AttributesPresent = context.Compilation.ReferencedAssemblyNames.Any(i => string.Equals(i.Name, "System.ComponentModel.Composition", StringComparison.OrdinalIgnoreCase));
+            bool mefV2AttributesPresent = context.Compilation.ReferencedAssemblyNames.Any(i => string.Equals(i.Name, "System.Composition.AttributedModel", StringComparison.OrdinalIgnoreCase));
+            if (mefV1AttributesPresent && mefV2AttributesPresent)
+            {
+                context.RegisterSymbolAction(context => this.AnalyzeSymbol(context), SymbolKind.NamedType);
+            }
+        });
+    }
+
+    private void AnalyzeSymbol(SymbolAnalysisContext context)
+    {
+        INamedTypeSymbol symbol = (INamedTypeSymbol)context.Symbol;
+        List<ISymbol>? mefv1AttributedMembers = null;
+        List<ISymbol>? mefv2AttributedMembers = null;
+
+        SearchSymbolForAttributes(symbol);
+        foreach (ISymbol member in symbol.GetMembers())
+        {
+            // We're not interested in nested types when scanning for members of the type.
+            if (member is ITypeSymbol)
+            {
+                continue;
+            }
+
+            SearchSymbolForAttributes(member);
+        }
+
+        if (mefv1AttributedMembers is { Count: > 0 } && mefv2AttributedMembers is { Count: > 0 })
+        {
+            // For additional locations, we'll use the shorter of the two lists, optimizing the assumption that the mistake is smaller than the intent.
+            List<ISymbol> smallerList = mefv1AttributedMembers.Count < mefv2AttributedMembers.Count ? mefv1AttributedMembers : mefv2AttributedMembers;
+            context.ReportDiagnostic(Diagnostic.Create(Descriptor, symbol.Locations[0], smallerList.Select(s => s.Locations[0]), symbol.Name));
+        }
+
+        void SearchSymbolForAttributes(ISymbol symbol)
+        {
+            foreach (AttributeData attribute in symbol.GetAttributes())
+            {
+                CheckAttribute(symbol, attribute, ref mefv1AttributedMembers, Mefv1AttributeNamespace.AsSpan());
+                CheckAttribute(symbol, attribute, ref mefv2AttributedMembers, Mefv2AttributeNamespace.AsSpan());
+            }
+        }
+    }
+
+    private static void CheckAttribute(ISymbol symbol, AttributeData attribute, ref List<ISymbol>? list, ReadOnlySpan<string> ns)
+    {
+        if (IsSymbolInOrDerivedFromNamespace(attribute.AttributeClass, ns))
+        {
+            list ??= new();
+            list.Add(symbol);
+        }
+    }
+
+    private static bool IsSymbolInOrDerivedFromNamespace(INamedTypeSymbol? attribute, ReadOnlySpan<string> namespaceName)
+    {
+        if (attribute is null)
+        {
+            return false;
+        }
+
+        if (IsNamespaceMatch(attribute.ContainingNamespace, namespaceName))
+        {
+            return true;
+        }
+
+        return IsSymbolInOrDerivedFromNamespace(attribute.BaseType, namespaceName);
+    }
+
+    private static bool IsNamespaceMatch(INamespaceSymbol? actual, ReadOnlySpan<string> expectedNames)
+    {
+        if (actual is null or { IsGlobalNamespace: true })
+        {
+            return expectedNames.Length == 0;
+        }
+
+        if (expectedNames.Length == 0)
+        {
+            return false;
+        }
+
+        if (actual.Name != expectedNames[expectedNames.Length - 1])
+        {
+            return false;
+        }
+
+        return IsNamespaceMatch(actual.ContainingNamespace, expectedNames.Slice(0, expectedNames.Length - 1));
+    }
+}

test/Microsoft.VisualStudio.Composition.Analyzers.Tests/Helpers/CSharpCodeFixVerifier`2+Test.cs

@@ -1,23 +1,14 @@
 // Copyright (c) Microsoft Corporation. All rights reserved.
 // Licensed under the MIT license. See LICENSE file in the project root for full license information.
 
-using System;
-using System.Collections.Immutable;
-using System.IO;
-using System.Linq;
 using System.Reflection;
-using System.Runtime.CompilerServices;
-using System.Threading.Tasks;
-using Microsoft.CodeAnalysis;
 using Microsoft.CodeAnalysis.CSharp;
 using Microsoft.CodeAnalysis.CSharp.Testing;
-using Microsoft.CodeAnalysis.Diagnostics;
-using Microsoft.CodeAnalysis.Testing.Verifiers;
 using Microsoft.CodeAnalysis.Text;
 
 public static partial class CSharpCodeFixVerifier<TAnalyzer, TCodeFix>
 {
-    public class Test : CSharpCodeFixTest<TAnalyzer, TCodeFix, XUnitVerifier>
+    public class Test : CSharpCodeFixTest<TAnalyzer, TCodeFix, DefaultVerifier>
     {
         public Test()
         {

test/Microsoft.VisualStudio.Composition.Analyzers.Tests/Helpers/CSharpCodeFixVerifier`2.cs

@@ -1,23 +1,17 @@
 // Copyright (c) Microsoft Corporation. All rights reserved.
 // Licensed under the MIT license. See LICENSE file in the project root for full license information.
 
-using System.Threading.Tasks;
-using Microsoft.CodeAnalysis;
-using Microsoft.CodeAnalysis.CodeFixes;
 using Microsoft.CodeAnalysis.CSharp.Testing;
-using Microsoft.CodeAnalysis.Diagnostics;
-using Microsoft.CodeAnalysis.Testing;
-using Microsoft.CodeAnalysis.Testing.Verifiers;
 
 public static partial class CSharpCodeFixVerifier<TAnalyzer, TCodeFix>
     where TAnalyzer : DiagnosticAnalyzer, new()
     where TCodeFix : CodeFixProvider, new()
 {
     public static DiagnosticResult Diagnostic()
-        => CSharpCodeFixVerifier<TAnalyzer, TCodeFix, XUnitVerifier>.Diagnostic();
+        => CSharpCodeFixVerifier<TAnalyzer, TCodeFix, DefaultVerifier>.Diagnostic();
 
     public static DiagnosticResult Diagnostic(string diagnosticId)
-        => CSharpCodeFixVerifier<TAnalyzer, TCodeFix, XUnitVerifier>.Diagnostic(diagnosticId);
+        => CSharpCodeFixVerifier<TAnalyzer, TCodeFix, DefaultVerifier>.Diagnostic(diagnosticId);
 
     public static DiagnosticResult Diagnostic(DiagnosticDescriptor descriptor)
         => new DiagnosticResult(descriptor);