Fix FunctionContext Check and Polymorphic Type Conversions in Activity Analyzer #506

nytian · 2025-11-17T04:20:45Z

Copilot

Pull Request Overview

This PR fixes issue #453 by improving the Activity analyzer to correctly handle FunctionContext parameters and support polymorphic type conversions.

Adds special handling to skip validation for activities that use FunctionContext with [ActivityTrigger], preventing false positive warnings
Replaces exact type matching with type compatibility checking using Roslyn's ClassifyConversion API and custom collection compatibility logic
Adds comprehensive test coverage for FunctionContext handling, polymorphism, collection type compatibility, and incompatible type detection

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
src/Analyzers/Activities/MatchingInputOutputTypeActivityAnalyzer.cs	Adds FunctionContext detection to skip DI parameter validation; implements type compatibility checking with support for polymorphism, inheritance, and collection interface implementations (List to IReadOnlyList)
test/Analyzers.Tests/Activities/MatchingInputOutputTypeActivityAnalyzerTests.cs	Adds 6 new test cases covering FunctionContext scenarios, polymorphic input/output types, collection type compatibility, and incompatible type detection

Copilot · 2025-11-17T04:26:09Z

src/Analyzers/Activities/MatchingInputOutputTypeActivityAnalyzer.cs

+        foreach (INamedTypeSymbol @interface in sourceType.AllInterfaces)
+        {
+            if (SymbolEqualityComparer.Default.Equals(@interface.OriginalDefinition, targetInterface.OriginalDefinition))
+            {
+                return true;
+            }
+        }


This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.

bachuv

Thanks for opening this PR!

Can you add more details in the PR description?

src/Analyzers/Activities/MatchingInputOutputTypeActivityAnalyzer.cs

andystaples

One question

andystaples · 2025-11-19T18:22:46Z

src/Analyzers/Activities/MatchingInputOutputTypeActivityAnalyzer.cs

+        }
+
+        // One is null, the other isn't = not compatible
+        if (sourceType == null || targetType == null)


Just checking - does this logic support passing null as input to a nullable function parameter? That was also an issue with this analyzer previously

…rabletask-dotnet into nytian/analyzer-bug

Copilot

Pull Request Overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

Copilot · 2025-11-20T23:20:31Z

src/Analyzers/Activities/MatchingInputOutputTypeActivityAnalyzer.cs

+        // Array implements: IEnumerable<T>, ICollection<T>, IList<T>, IReadOnlyCollection<T>, IReadOnlyList<T>
+        string targetName = targetInterface.OriginalDefinition.ToDisplayString();
+        return targetName == "System.Collections.Generic.IEnumerable<T>" ||
+               targetName == "System.Collections.Generic.ICollection<T>" ||
+               targetName == "System.Collections.Generic.IList<T>" ||
+               targetName == "System.Collections.Generic.IReadOnlyCollection<T>" ||
+               targetName == "System.Collections.Generic.IReadOnlyList<T>";


[nitpick] The array interface compatibility check uses string comparison of type names, which may not be robust across different compilation contexts or type forwarding scenarios. Consider using GetTypeByMetadataName to get reference symbols for the standard collection interfaces and comparing using SymbolEqualityComparer.Default.Equals instead of string comparison. This would be more reliable and less prone to edge cases with type representation differences.

Copilot · 2025-11-20T23:20:32Z

src/Analyzers/Activities/MatchingInputOutputTypeActivityAnalyzer.cs

+            if (sourceType == null && targetType != null)
+            {
+                // Check if target is a nullable reference type (NullableAnnotation.Annotated)
+                // or a nullable value type (Nullable<T>)
+                if (targetType.NullableAnnotation == NullableAnnotation.Annotated ||
+                    targetType.OriginalDefinition.SpecialType == SpecialType.System_Nullable_T)
+                {
+                    return true;
+                }
+            }


The null handling logic may incorrectly reject null values passed to reference type parameters when nullable reference types are disabled. When sourceType is null and targetType is a reference type (like string), the code checks for NullableAnnotation.Annotated (line 369), which will be false when nullable reference types are disabled. However, in such projects, all reference types can accept null. Consider adding || targetType.IsReferenceType to line 369 to allow null for all reference types, or checking if nullable context is enabled before applying strict nullable validation.

turns out this is not right

src/Analyzers/Activities/MatchingInputOutputTypeActivityAnalyzer.cs

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.

Comments suppressed due to low confidence (1)

src/Analyzers/Activities/MatchingInputOutputTypeActivityAnalyzer.cs:374

These 'if' statements can be combined.

            if (sourceType == null && targetType != null)
            {
                // Check if target is a nullable reference type (NullableAnnotation.Annotated)
                // or a nullable value type (Nullable<T>)
                if (targetType.NullableAnnotation == NullableAnnotation.Annotated ||
                    targetType.OriginalDefinition.SpecialType == SpecialType.System_Nullable_T)
                {
                    return true;
                }
            }

src/Analyzers/Activities/MatchingInputOutputTypeActivityAnalyzer.cs

test/Analyzers.Tests/Activities/MatchingInputOutputTypeActivityAnalyzerTests.cs

src/Analyzers/Activities/MatchingInputOutputTypeActivityAnalyzer.cs

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

src/Analyzers/Activities/MatchingInputOutputTypeActivityAnalyzer.cs

test/Analyzers.Tests/Activities/MatchingInputOutputTypeActivityAnalyzerTests.cs

nytian · 2025-11-26T04:08:21Z

E2E test
-- with latest v1.11.1

--with fix

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

Copilot · 2025-11-26T04:12:30Z

src/Analyzers/Activities/MatchingInputOutputTypeActivityAnalyzer.cs

+    /// <summary>
+    /// Checks if the source type is compatible with (can be assigned to) the target type.
+    /// This handles polymorphism, interface implementation, inheritance, and collection type compatibility.
+    /// </summary>
+    static bool AreTypesCompatible(Compilation compilation, ITypeSymbol? sourceType, ITypeSymbol? targetType)
+    {
+        // Both null = compatible (no input/output on both sides)
+        if (sourceType == null && targetType == null)
+        {
+            return true;
+        }
+
+        // Special case: null (no input/output provided) can be passed to explicitly nullable parameters
+        // This handles nullable value types (int?) and nullable reference types (string?)
+        if (sourceType == null && targetType != null)
+        {
+            // Check if target is a nullable value type (Nullable<T>)
+            if (targetType.OriginalDefinition.SpecialType == SpecialType.System_Nullable_T)
+            {
+                return true;
+            }
+
+            // Check if target is a nullable reference type (string?)
+            if (targetType.NullableAnnotation == NullableAnnotation.Annotated)
+            {
+                return true;
+            }
+
+            // Not nullable, so null input is incompatible
+            return false;
+        }
+
+        // If targetType is null but sourceType is not, they're incompatible
+        if (targetType == null && sourceType != null)
+        {
+            return false;
+        }
+
+        // Check if types are exactly equal
+        if (SymbolEqualityComparer.Default.Equals(sourceType, targetType))
+        {
+            return true;
+        }
+
+        // Check if source type can be converted to target type (handles inheritance, interface implementation, etc.)
+        // At this point, both sourceType and targetType are guaranteed to be non-null
+        Conversion conversion = compilation.ClassifyConversion(sourceType!, targetType!);
+        if (conversion.IsImplicit || conversion.IsIdentity)
+        {
+            return true;
+        }
+
+        // Special handling for collection types since ClassifyConversion doesn't always recognize
+        // generic interface implementations (e.g., List<T> to IReadOnlyList<T>)
+        // At this point, both sourceType and targetType are guaranteed to be non-null
+        if (IsCollectionTypeCompatible(sourceType!, targetType!))
+        {
+            return true;
+        }
+
+        return false;
+    }
+
+    /// <summary>
+    /// Checks if the source collection type is compatible with the target collection type.
+    /// Handles common scenarios like List to IReadOnlyList, arrays to IEnumerable, etc.
+    /// </summary>
+    static bool IsCollectionTypeCompatible(ITypeSymbol sourceType, ITypeSymbol targetType)
+    {
+        // Check if source is an array and target is a collection interface
+        if (sourceType is IArrayTypeSymbol sourceArray && targetType is INamedTypeSymbol targetNamed)
+        {
+            return IsArrayCompatibleWithCollectionInterface(sourceArray, targetNamed);
+        }
+
+        // Both must be generic named types
+        if (sourceType is not INamedTypeSymbol sourceNamed || targetType is not INamedTypeSymbol targetNamedType)
+        {
+            return false;
+        }
+
+        // Both must be generic types with the same type arguments
+        if (!sourceNamed.IsGenericType || !targetNamedType.IsGenericType)
+        {
+            return false;
+        }
+
+        if (sourceNamed.TypeArguments.Length != targetNamedType.TypeArguments.Length)
+        {
+            return false;
+        }
+
+        // Check if type arguments are compatible (could be different but compatible types)
+        for (int i = 0; i < sourceNamed.TypeArguments.Length; i++)
+        {
+            if (!SymbolEqualityComparer.Default.Equals(sourceNamed.TypeArguments[i], targetNamedType.TypeArguments[i]))
+            {
+                // Type arguments must match exactly for collections (we don't support covariance/contravariance here)
+                return false;
+            }
+        }
+
+        // Check if source type implements or derives from target type
+        // This handles: List<T> → IReadOnlyList<T>, List<T> → IEnumerable<T>, etc.
+        return ImplementsInterface(sourceNamed, targetNamedType);
+    }
+
+    /// <summary>
+    /// Checks if an array type is compatible with a collection interface.
+    /// </summary>
+    static bool IsArrayCompatibleWithCollectionInterface(IArrayTypeSymbol arrayType, INamedTypeSymbol targetInterface)
+    {
+        if (!targetInterface.IsGenericType || targetInterface.TypeArguments.Length != 1)
+        {
+            return false;
+        }
+
+        // Check if array element type matches the generic type argument
+        if (!SymbolEqualityComparer.Default.Equals(arrayType.ElementType, targetInterface.TypeArguments[0]))
+        {
+            return false;
+        }
+
+        // Array implements: IEnumerable<T>, ICollection<T>, IList<T>, IReadOnlyCollection<T>, IReadOnlyList<T>
+        string targetName = targetInterface.OriginalDefinition.ToDisplayString();
+        return targetName == "System.Collections.Generic.IEnumerable<T>" ||
+               targetName == "System.Collections.Generic.ICollection<T>" ||
+               targetName == "System.Collections.Generic.IList<T>" ||
+               targetName == "System.Collections.Generic.IReadOnlyCollection<T>" ||
+               targetName == "System.Collections.Generic.IReadOnlyList<T>";
+    }
+
+    /// <summary>
+    /// Checks if the source type implements the target interface.
+    /// </summary>
+    static bool ImplementsInterface(INamedTypeSymbol sourceType, INamedTypeSymbol targetInterface)
+    {
+        // Check all interfaces implemented by the source type
+        return sourceType.AllInterfaces.Any(@interface =>
+            SymbolEqualityComparer.Default.Equals(@interface.OriginalDefinition, targetInterface.OriginalDefinition));
+    }


The new AreTypesCompatible, IsCollectionTypeCompatible, IsArrayCompatibleWithCollectionInterface, and ImplementsInterface methods lack test coverage. Consider adding unit tests to verify:

Polymorphic type compatibility (e.g., passing a List<string> where IReadOnlyList<string> is expected)

Array to collection interface compatibility (e.g., string[] to IEnumerable<string>)

Interface implementation checks

Null handling for nullable reference types and nullable value types

Edge cases like generic variance scenarios

Copilot · 2025-11-26T04:12:30Z

src/Analyzers/Activities/MatchingInputOutputTypeActivityAnalyzer.cs

+                    // If the parameter is FunctionContext, skip validation for this activity (it's a DI parameter, not real input)
+                    if (functionContextSymbol != null && SymbolEqualityComparer.Default.Equals(inputParam.Type, functionContextSymbol))
+                    {
+                        return;
+                    }


The FunctionContext check (lines 167-171) lacks test coverage. Consider adding a test case that verifies an activity method with a FunctionContext parameter marked with [ActivityTrigger] doesn't generate diagnostics, confirming that DI parameters are properly excluded from input type validation.

Copilot · 2025-11-26T04:12:31Z

src/Analyzers/Activities/MatchingInputOutputTypeActivityAnalyzer.cs

+        // Check if type arguments are compatible (could be different but compatible types)
+        for (int i = 0; i < sourceNamed.TypeArguments.Length; i++)
+        {
+            if (!SymbolEqualityComparer.Default.Equals(sourceNamed.TypeArguments[i], targetNamedType.TypeArguments[i]))
+            {
+                // Type arguments must match exactly for collections (we don't support covariance/contravariance here)
+                return false;
+            }
+        }


The comment on line 436 "Check if type arguments are compatible (could be different but compatible types)" is misleading, as the implementation (lines 437-444) requires exact type argument equality, not just compatibility. Consider updating the comment to: "Check if type arguments match exactly (we don't support covariance/contravariance for collections)" to better reflect the actual behavior.

initial commit

18f4674

Copilot AI review requested due to automatic review settings November 17, 2025 04:20

Copilot started reviewing on behalf of nytian November 17, 2025 04:21 View session

Copilot finished reviewing on behalf of nytian November 17, 2025 04:25

Copilot AI reviewed Nov 17, 2025

View reviewed changes

bachuv reviewed Nov 17, 2025

View reviewed changes

src/Analyzers/Activities/MatchingInputOutputTypeActivityAnalyzer.cs Show resolved Hide resolved

andystaples reviewed Nov 19, 2025

View reviewed changes

nytian and others added 4 commits November 20, 2025 10:00

Merge branch 'main' into nytian/analyzer-bug

9012dd4

add comment and nullableannotation check

365cec0

update coilot cmment

30c59fb

Merge branch 'nytian/analyzer-bug' of https://github.com/microsoft/du…

ec21a2a

…rabletask-dotnet into nytian/analyzer-bug

Copilot AI review requested due to automatic review settings November 20, 2025 23:14

Copilot started reviewing on behalf of nytian November 20, 2025 23:15 View session

Copilot finished reviewing on behalf of nytian November 20, 2025 23:19

Copilot AI reviewed Nov 20, 2025

View reviewed changes

Copilot AI review requested due to automatic review settings November 21, 2025 19:58

nytian force-pushed the nytian/analyzer-bug branch from c47ebea to ec21a2a Compare November 21, 2025 19:58

Copilot started reviewing on behalf of nytian November 21, 2025 19:59 View session

Copilot finished reviewing on behalf of nytian November 21, 2025 20:03

Copilot AI reviewed Nov 21, 2025

View reviewed changes

nytian added 2 commits November 21, 2025 14:36

update

a7c3224

fix

5bad75a

Copilot AI review requested due to automatic review settings November 21, 2025 23:02

Copilot started reviewing on behalf of nytian November 21, 2025 23:02 View session

Copilot finished reviewing on behalf of nytian November 21, 2025 23:06

Copilot AI reviewed Nov 21, 2025

View reviewed changes

src/Analyzers/Activities/MatchingInputOutputTypeActivityAnalyzer.cs Show resolved Hide resolved

test/Analyzers.Tests/Activities/MatchingInputOutputTypeActivityAnalyzerTests.cs Outdated Show resolved Hide resolved

nytian requested review from andystaples and bachuv November 24, 2025 23:39

andystaples approved these changes Nov 25, 2025

View reviewed changes

remove tests as not necessary

6f57905

Merge branch 'main' into nytian/analyzer-bug

050f8b9

Copilot AI review requested due to automatic review settings November 26, 2025 04:07

Copilot started reviewing on behalf of nytian November 26, 2025 04:08 View session

Copilot finished reviewing on behalf of nytian November 26, 2025 04:12

Copilot AI reviewed Nov 26, 2025

View reviewed changes

Fix FunctionContext Check and Polymorphic Type Conversions in Activity Analyzer #506

Are you sure you want to change the base?

Fix FunctionContext Check and Polymorphic Type Conversions in Activity Analyzer #506

Conversation

nytian commented Nov 17, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Nov 17, 2025

Choose a reason for hiding this comment

Uh oh!

bachuv left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

andystaples left a comment

Choose a reason for hiding this comment

Uh oh!

andystaples Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

nytian Nov 21, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

nytian commented Nov 26, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants