Merge pull request #648 from drewnoakes/fix-VSMEF007

Fix VSMEF007

Author: AArnott

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

@@ -270,6 +270,22 @@ internal static bool GetAllowDefaultValue(AttributeData importAttribute)
         return false; // Default value is false
     }
 
+    /// <summary>
+    /// Determines whether an Import attribute requires a NonShared instance.
+    /// </summary>
+    /// <param name="importAttribute">The Import attribute to check.</param>
+    /// <returns>True if RequiredCreationPolicy = NonShared; otherwise false.</returns>
+    internal static bool IsNonSharedImport(AttributeData importAttribute)
+    {
+        TypedConstant creationPolicyArg = importAttribute.NamedArguments.FirstOrDefault(arg => arg.Key == "RequiredCreationPolicy").Value;
+
+        // CreationPolicy values:
+        //     Any = 0
+        //     Shared = 1
+        //     NonShared = 2
+        return creationPolicyArg.Value is 2;
+    }
+
     /// <summary>
     /// Determines which MEF version an attribute type belongs to by walking up its inheritance hierarchy.
     /// </summary>
@@ -311,6 +327,12 @@ internal static bool GetAllowDefaultValue(AttributeData importAttribute)
 
         return null;
     }
+
+    internal static void Deconstruct<TKey, TValue>(this IGrouping<TKey, TValue> grouping, out TKey key, out IEnumerable<TValue> values)
+    {
+        key = grouping.Key;
+        values = grouping;
+    }
 }
 
 /// <summary>

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

@@ -13,9 +13,19 @@ namespace Microsoft.VisualStudio.Composition.Analyzers;
 /// Creates a diagnostic when a type imports the same contract more than once.
 /// </summary>
 /// <remarks>
+/// <para>
 /// This analyzer detects when a type has multiple imports (properties, constructor parameters, or a mix)
 /// that import the same contract. For imports with specific contract names, it only flags duplicates
 /// when the contract names match exactly.
+/// </para>
+/// <para>
+/// Note: This analyzer can produce false positives. If a MEF part is exported as non-shared, then it
+/// may make sense to have multiple seemingly-identical imports and use them independently. This analyzer
+/// considers the import-site RequiredCreationPolicy attribute, but cannot detect cases where the exported
+/// type itself is marked as NonShared, since that information is not statically available at the import site.
+/// In such cases, runtime composition may succeed even though this analyzer reports a warning, and the developer
+/// should suppress the instance of the diagnostic. This should be very rare however.
+/// </para>
 /// </remarks>
 [DiagnosticAnalyzer(LanguageNames.CSharp, LanguageNames.VisualBasic)]
 public class VSMEF007DuplicateImportAnalyzer : DiagnosticAnalyzer
@@ -66,20 +76,27 @@ private static void AnalyzeType(SymbolAnalysisContext context)
             return;
         }
 
-        var importContracts = new List<ImportContract>();
+        var imports = new List<Import>();
 
         // Collect all imports from properties
+        // Note: We skip NonShared imports because each gets a unique instance, so they're not problematic duplicates
         foreach (ISymbol member in namedType.GetMembers())
         {
             if (member is IPropertySymbol property)
             {
                 AttributeData? importAttribute = Utils.GetImportAttribute(property.GetAttributes());
 
-                string? contract = GetImportContract(importAttribute, property.Type);
-
-                if (contract is not null)
+                if (importAttribute is not null)
                 {
-                    importContracts.Add(new ImportContract(contract, property.Name, property.Locations[0]));
+                    if (Utils.IsNonSharedImport(importAttribute))
+                    {
+                        // Skip NonShared imports, each gets a unique instance.
+                        continue;
+                    }
+
+                    Contract contract = GetImportContract(importAttribute, property.Type);
+
+                    imports.Add(new Import(contract, property.Name, property.Locations[0]));
                 }
             }
         }
@@ -97,71 +114,91 @@ private static void AnalyzeType(SymbolAnalysisContext context)
                     {
                         AttributeData? importAttribute = Utils.GetImportAttribute(parameter.GetAttributes());
 
-                        string? contract = GetImportContract(importAttribute, parameter.Type);
-
-                        if (contract is not null)
+                        if (importAttribute is not null && Utils.IsNonSharedImport(importAttribute))
                         {
-                            importContracts.Add(new ImportContract(contract, parameter.Name, parameter.Locations[0]));
+                            // Skip NonShared imports, each gets a unique instance.
+                            continue;
                         }
+
+                        Contract contract = GetImportContract(importAttribute, parameter.Type);
+
+                        imports.Add(new Import(contract, parameter.Name, parameter.Locations[0]));
                     }
                 }
             }
         }
 
         // Find duplicates
-        IEnumerable<IGrouping<string, ImportContract>> contractGroups = importContracts.GroupBy(ic => ic.Contract).Where(g => g.Count() > 1);
+        IEnumerable<IGrouping<Contract, Import>> duplicateImportsByContract = imports.GroupBy(ic => ic.Contract).Where(g => g.Count() > 1);
 
-        foreach (IGrouping<string, ImportContract>? group in contractGroups)
+        foreach ((Contract contract, IEnumerable<Import> duplicateImports) in duplicateImportsByContract)
         {
-            foreach (ImportContract? import in group)
+            foreach (Import import in duplicateImports)
             {
                 context.ReportDiagnostic(Diagnostic.Create(
                     Descriptor,
                     import.Location,
                     namedType.Name,
-                    import.Contract));
+                    contract));
             }
         }
     }
 
-    private static string? GetImportContract(AttributeData? importAttribute, ITypeSymbol importType)
+    private static Contract GetImportContract(AttributeData? importAttribute, ITypeSymbol importType)
     {
-        // Note that the actual contract name used by MEF is more complex than this. See ContractNameServices
-        // for the full logic. This approximation suffices for catching duplicates, for the purposes of this analyzer.
+        string? explicitContractName = null;
+        ITypeSymbol? explicitContractType = null;
+
         if (importAttribute is not null)
         {
-            // Check for explicit contract name or type in constructor arguments
-            if (importAttribute.ConstructorArguments is [TypedConstant firstArg, ..])
+            // Check for explicit contract name or type in constructor arguments.
+            // Uses patterns from both System.Composition.ImportAttribute and System.ComponentModel.Composition.ImportAttribute.
+            // An empty or null contract name means "no explicit name".
+            (explicitContractName, explicitContractType) = importAttribute.ConstructorArguments switch
             {
-                if (firstArg.Value is string { Length: not 0 } contractName)
-                {
-                    return contractName;
-                }
-
-                if (firstArg.Value is INamedTypeSymbol contractType)
-                {
-                    return contractType.ToDisplayString();
-                }
-            }
+                [TypedConstant { Value: string { Length: not 0 } contractName }] => (contractName, null),
+                [TypedConstant { Value: INamedTypeSymbol contractType }] => (null, contractType),
+                [TypedConstant { Value: string { Length: not 0 } contractName }, TypedConstant { Value: INamedTypeSymbol contractType }] => (contractName, contractType),
+                [TypedConstant { Value: null or string { Length: 0 } }, TypedConstant { Value: INamedTypeSymbol contractType }] => (null, contractType),
+                _ => (null, null),
+            };
 
             // Check for contract name in named arguments
-            KeyValuePair<string, TypedConstant> contractNameArg = importAttribute.NamedArguments.FirstOrDefault(arg => arg.Key == "ContractName");
-            if (contractNameArg.Key is not null && contractNameArg.Value.Value is string namedContractName && !string.IsNullOrEmpty(namedContractName))
+            TypedConstant? nameArg = importAttribute.NamedArguments.FirstOrDefault(arg => arg.Key == "ContractName").Value;
+            if (nameArg?.Value is string { Length: not 0 } namedContractName)
             {
-                return namedContractName;
+                explicitContractName = namedContractName;
             }
 
             // Check for contract type in named arguments
-            KeyValuePair<string, TypedConstant> contractTypeArg = importAttribute.NamedArguments.FirstOrDefault(arg => arg.Key == "ContractType");
-            if (contractTypeArg.Key is not null && contractTypeArg.Value.Value is INamedTypeSymbol namedContractType)
+            TypedConstant? typeArg = importAttribute.NamedArguments.FirstOrDefault(arg => arg.Key == "ContractType").Value;
+            if (typeArg?.Value is INamedTypeSymbol namedContractType)
             {
-                return namedContractType.ToDisplayString();
+                explicitContractType = namedContractType;
             }
         }
 
-        // Default contract is the type name
-        return importType.ToDisplayString();
+        // Determine the base type for defaulting contract name and type.
+        // Note that the actual contract name used by MEF is more complex than this. See ContractNameServices
+        // for the full logic. This approximation suffices for catching duplicates, for the purposes of this analyzer.
+
+        // If contract type is explicitly specified, use it; otherwise use the import parameter type.
+        ITypeSymbol type = explicitContractType ?? importType;
+        string typeName = type.ToDisplayString();
+
+        // Contract name: use explicit name if provided, otherwise default to type name.
+        string name = explicitContractName ?? typeName;
+
+        return new Contract(typeName, name);
+    }
+
+    // A MEF contract consists of both a contract name and a contract type.
+    // Both must match for two imports to be considered duplicates.
+    // See: https://learn.microsoft.com/en-us/dotnet/framework/mef/attributed-programming-model-overview-mef#import-and-export-basics
+    private readonly record struct Contract(string Type, string? Name)
+    {
+        public override string ToString() => this.Name is null || string.Equals(this.Name, this.Type) ? this.Type : $"{this.Type} (\"{this.Name}\")";
     }
 
-    private record ImportContract(string Contract, string MemberName, Location Location);
+    private record Import(Contract Contract, string MemberName, Location Location);
 }

test/Microsoft.VisualStudio.Composition.Analyzers.Tests/VSMEF001PropertyMustHaveSetterTests.cs

@@ -1,9 +1,6 @@
 // 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.Threading.Tasks;
-using Xunit;
 using VerifyCS = CSharpCodeFixVerifier<Microsoft.VisualStudio.Composition.Analyzers.VSMEF001PropertyMustHaveSetter, Microsoft.CodeAnalysis.Testing.EmptyCodeFixProvider>;
 using VerifyVB = VisualBasicCodeFixVerifier<Microsoft.VisualStudio.Composition.Analyzers.VSMEF001PropertyMustHaveSetter, Microsoft.CodeAnalysis.Testing.EmptyCodeFixProvider>;