Merge pull request #682 from microsoft/copilot/fix-vs-sdk-vsmef008-errors

Reduce VSMEF008 false positives: lower severity, soften wording, add allow-list support

Author: Copilot

docfx/analyzers/VSMEF008.md

@@ -4,11 +4,11 @@ When using `[Import(typeof(T))]` or `[ImportMany(typeof(T))]` with an explicit c
 
 ## Cause
 
-An import specifies a `ContractType` that is incompatible with the member type receiving the import. This will cause a composition failure at runtime.
+An import specifies a `ContractType` that is incompatible with the member type receiving the import. This may cause a composition failure at runtime.
 
 ## Rule description
 
-MEF allows you to specify an explicit contract type using `[Import(typeof(T))]` or `[ImportMany(typeof(T))]`. The exports matching this contract must be assignable to the member where the import is declared. If the types are incompatible, composition will fail at runtime.
+MEF allows you to specify an explicit contract type using `[Import(typeof(T))]` or `[ImportMany(typeof(T))]`. The exports matching this contract must be assignable to the member where the import is declared. If the types are incompatible, composition may fail at runtime.
 
 This analyzer detects the following incompatibilities:
 
@@ -152,9 +152,32 @@ public IDatabase Database { get; set; }  // Change contract to match member type
 public ILogger Logger { get; set; }  // Let MEF infer the contract type
 ```
 
-## When to suppress errors
+### Option 4: Add an allow-list entry (for SDK scenarios)
 
-This error should generally not be suppressed as it indicates a bug that will cause runtime failures. If you're doing something unusual with MEF that the analyzer doesn't understand, ensure you have tests that verify the composition works correctly.
+Some SDKs use a type identity as a contract name even when that type is not assignable to the import type. For example, the Visual Studio SDK uses `SVsFullAccessServiceBroker` as a contract name for imports of type `IServiceBroker`.
+
+If your project relies on such a convention, you can suppress this warning for specific pairs by adding an `AdditionalFiles` entry named `vs-mef.ContractNamesAssignability.txt` to your project:
+
+```xml
+<ItemGroup>
+  <AdditionalFiles Include="vs-mef.ContractNamesAssignability.txt" />
+</ItemGroup>
+```
+
+The file format is one entry per line, using the pattern `MemberType <= ContractType`:
+
+```
+# Lines starting with # are comments
+Microsoft.ServiceHub.Framework.IServiceBroker <= Microsoft.VisualStudio.Shell.ServiceBroker.SVsFullAccessServiceBroker
+Microsoft.VisualStudio.Shell.IAsyncServiceProvider <= Microsoft.VisualStudio.Shell.Interop.SAsyncServiceProvider
+Microsoft.VisualStudio.Shell.IAsyncServiceProvider2 <= Microsoft.VisualStudio.Shell.Interop.SAsyncServiceProvider
+```
+
+Each line declares that `ContractType` is a known-good contract name for an import of `MemberType`, even though the types are not statically assignable.
+
+## When to suppress warnings
+
+This warning indicates a potential bug in most cases. However, if you're using a contract name that is a type identity not assignable to the import type (a common SDK pattern), consider using the allow-list approach described in Option 4 instead of suppressing the diagnostic globally.
 
 ## Notes
 

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

@@ -10,7 +10,7 @@ VSMEF004 | Usage | Error | Exported type missing importing constructor
 VSMEF005 | Usage | Error | Multiple importing constructors
 VSMEF006 | Usage | Warning | Import nullability and AllowDefault mismatch
 VSMEF007 | Usage | Warning | Duplicate import contract
-VSMEF008 | Usage | Error | Import contract type not assignable to member type
+VSMEF008 | Usage | Warning | Import contract type not assignable to member type
 VSMEF009 | Usage | Error | ImportMany on non-collection type
 VSMEF010 | Usage | Error | ImportMany with unsupported collection type in constructor
 VSMEF011 | Usage | Error | Both Import and ImportMany applied to same member

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

@@ -199,7 +199,7 @@
     <comment>{Locked="AllowDefault"} {Locked="true"} The property name and value should not be localized.</comment>
   </data>
   <data name="VSMEF008_MessageFormat" xml:space="preserve">
-    <value>The contract type "{0}" is not assignable to the member type "{1}". The import will fail at runtime.</value>
+    <value>The contract type "{0}" is not assignable to the member type "{1}". The import may fail at runtime.</value>
   </data>
   <data name="VSMEF008_Title" xml:space="preserve">
     <value>Import contract type not assignable to member type</value>

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

@@ -6,6 +6,7 @@ namespace Microsoft.VisualStudio.Composition.Analyzers;
 using System.Collections.Immutable;
 using Microsoft.CodeAnalysis;
 using Microsoft.CodeAnalysis.Diagnostics;
+using Microsoft.CodeAnalysis.Text;
 
 /// <summary>
 /// Creates a diagnostic when an Import attribute specifies a contract type that is not assignable to the member type.
@@ -28,6 +29,21 @@ public class VSMEF008ImportContractTypeMismatchAnalyzer : DiagnosticAnalyzer
     /// </summary>
     public const string Id = "VSMEF008";
 
+    /// <summary>
+    /// The filename for AdditionalFiles that declare allowed contract type assignments.
+    /// </summary>
+    /// <remarks>
+    /// Each line in this file should follow the pattern: <c>MemberType &lt;= ContractType</c>.
+    /// Lines starting with <c>#</c> and blank lines are ignored.
+    /// </remarks>
+    public const string ContractNamesAssignabilityFileName = "vs-mef.ContractNamesAssignability.txt";
+
+    /// <summary>
+    /// The display format used to produce fully-qualified type names for allow-list matching.
+    /// </summary>
+    private static readonly SymbolDisplayFormat FullyQualifiedTypeNameFormat = new(
+        typeQualificationStyle: SymbolDisplayTypeQualificationStyle.NameAndContainingTypesAndNamespaces);
+
     /// <summary>
     /// The descriptor used for diagnostics created by this rule.
     /// </summary>
@@ -37,7 +53,7 @@ public class VSMEF008ImportContractTypeMismatchAnalyzer : DiagnosticAnalyzer
         messageFormat: Strings.VSMEF008_MessageFormat,
         helpLinkUri: Utils.GetHelpLink(Id),
         category: "Usage",
-        defaultSeverity: DiagnosticSeverity.Error,
+        defaultSeverity: DiagnosticSeverity.Warning,
         isEnabledByDefault: true);
 
     /// <inheritdoc/>
@@ -78,7 +94,8 @@ public override void Initialize(AnalysisContext context)
                 lazyWithMetadataType,
                 exportFactoryType,
                 exportFactoryWithMetadataType,
-                ienumerableType);
+                ienumerableType,
+                ParseAllowedAssignments(context.Options.AdditionalFiles, context.CancellationToken));
 
             context.RegisterSymbolAction(ctx => AnalyzeProperty(ctx, state), SymbolKind.Property);
             context.RegisterSymbolAction(ctx => AnalyzeField(ctx, state), SymbolKind.Field);
@@ -156,6 +173,13 @@ private static void AnalyzeMember(
             // Check if the contract type is assignable to the expected type
             if (!IsAssignableTo(explicitContractType, expectedType, context.Compilation))
             {
+                // Check the allow-list before reporting
+                if (state.AllowedAssignments.Contains(
+                        (GetFullTypeName(expectedType), GetFullTypeName(explicitContractType))))
+                {
+                    continue;
+                }
+
                 context.ReportDiagnostic(Diagnostic.Create(
                     Descriptor,
                     location,
@@ -257,6 +281,58 @@ private static ITypeSymbol UnwrapLazyOrExportFactory(ITypeSymbol type, AnalyzerS
         return type;
     }
 
+    private static string GetFullTypeName(ITypeSymbol type) => type.ToDisplayString(FullyQualifiedTypeNameFormat);
+
+    private static ImmutableHashSet<(string MemberType, string ContractType)> ParseAllowedAssignments(
+        ImmutableArray<AdditionalText> additionalFiles,
+        System.Threading.CancellationToken cancellationToken)
+    {
+        ImmutableHashSet<(string, string)>.Builder? builder = null;
+
+        foreach (AdditionalText file in additionalFiles)
+        {
+            if (!string.Equals(
+                    System.IO.Path.GetFileName(file.Path),
+                    ContractNamesAssignabilityFileName,
+                    StringComparison.OrdinalIgnoreCase))
+            {
+                continue;
+            }
+
+            SourceText? text = file.GetText(cancellationToken);
+            if (text is null)
+            {
+                continue;
+            }
+
+            foreach (TextLine line in text.Lines)
+            {
+                string lineText = line.ToString().Trim();
+                if (lineText.Length == 0 || lineText[0] == '#')
+                {
+                    continue;
+                }
+
+                int separatorIndex = lineText.IndexOf("<=", StringComparison.Ordinal);
+                if (separatorIndex < 0)
+                {
+                    continue;
+                }
+
+                string memberType = lineText.Substring(0, separatorIndex).Trim();
+                string contractType = lineText.Substring(separatorIndex + 2).Trim();
+
+                if (memberType.Length > 0 && contractType.Length > 0)
+                {
+                    builder ??= ImmutableHashSet.CreateBuilder<(string, string)>();
+                    builder.Add((memberType, contractType));
+                }
+            }
+        }
+
+        return builder?.ToImmutable() ?? ImmutableHashSet<(string, string)>.Empty;
+    }
+
     private static bool IsAssignableTo(ITypeSymbol source, ITypeSymbol target, Compilation compilation)
     {
         // Direct equality check
@@ -308,5 +384,6 @@ private sealed record AnalyzerState(
         INamedTypeSymbol? LazyWithMetadataType,
         INamedTypeSymbol? ExportFactoryType,
         INamedTypeSymbol? ExportFactoryWithMetadataType,
-        INamedTypeSymbol? IEnumerableType);
+        INamedTypeSymbol? IEnumerableType,
+        ImmutableHashSet<(string MemberType, string ContractType)> AllowedAssignments);
 }

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

@@ -1,6 +1,7 @@
 // Copyright (c) Microsoft Corporation. All rights reserved.
 // Licensed under the MIT license. See LICENSE file in the project root for full license information.
 
+using Microsoft.CodeAnalysis.Text;
 using VerifyCS = CSharpCodeFixVerifier<Microsoft.VisualStudio.Composition.Analyzers.VSMEF008ImportContractTypeMismatchAnalyzer, Microsoft.CodeAnalysis.Testing.EmptyCodeFixProvider>;
 
 public class VSMEF008ImportContractTypeMismatchAnalyzerTests
@@ -193,4 +194,90 @@ class Foo
 
         await VerifyCS.VerifyAnalyzerAsync(test);
     }
+
+    [Fact]
+    public async Task ImportWithAllowListedAssignment_NoWarning()
+    {
+        string test = """
+            using System.ComponentModel.Composition;
+
+            namespace MyNS
+            {
+                interface IServiceBroker { }
+                class SVsFullAccessServiceBroker { }
+
+                class Foo
+                {
+                    [Import(typeof(SVsFullAccessServiceBroker))]
+                    public IServiceBroker Value { get; set; }
+                }
+            }
+            """;
+
+        string allowList = "MyNS.IServiceBroker <= MyNS.SVsFullAccessServiceBroker\n";
+
+        var verifier = new VerifyCS.Test { TestCode = test };
+        verifier.TestState.AdditionalFiles.Add(
+            ("vs-mef.ContractNamesAssignability.txt", SourceText.From(allowList)));
+        await verifier.RunAsync();
+    }
+
+    [Fact]
+    public async Task ImportWithAllowListedAssignmentWithComments_NoWarning()
+    {
+        string test = """
+            using System.ComponentModel.Composition;
+
+            namespace MyNS
+            {
+                interface IServiceBroker { }
+                class SVsFullAccessServiceBroker { }
+
+                class Foo
+                {
+                    [Import(typeof(SVsFullAccessServiceBroker))]
+                    public IServiceBroker Value { get; set; }
+                }
+            }
+            """;
+
+        string allowList = """
+            # This is a comment
+            MyNS.IServiceBroker <= MyNS.SVsFullAccessServiceBroker
+
+            """;
+
+        var verifier = new VerifyCS.Test { TestCode = test };
+        verifier.TestState.AdditionalFiles.Add(
+            ("vs-mef.ContractNamesAssignability.txt", SourceText.From(allowList)));
+        await verifier.RunAsync();
+    }
+
+    [Fact]
+    public async Task ImportWithUnrelatedAllowList_StillReportsWarning()
+    {
+        string test = """
+            using System.ComponentModel.Composition;
+
+            namespace MyNS
+            {
+                interface IServiceBroker { }
+                class SVsFullAccessServiceBroker { }
+
+                class Foo
+                {
+                    [Import(typeof(SVsFullAccessServiceBroker))]
+                    public IServiceBroker {|VSMEF008:Value|} { get; set; }
+                }
+            }
+            """;
+
+        // The allow-list entry maps a different pair so the warning should still fire
+        string allowList = "MyNS.IOtherService <= MyNS.SVsFullAccessServiceBroker\n";
+
+        var verifier = new VerifyCS.Test { TestCode = test };
+        verifier.TestState.AdditionalFiles.Add(
+            ("vs-mef.ContractNamesAssignability.txt", SourceText.From(allowList)));
+        await verifier.RunAsync();
+    }
 }