Fix FunctionContext Check and Polymorphic Type Conversions in Activity Analyzer by nytian · Pull Request #506 · microsoft/durabletask-dotnet

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.

src/Analyzers/Activities/MatchingInputOutputTypeActivityAnalyzer.cs

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

andystaples approved these changes Nov 26, 2025

View reviewed changes

nytian merged commit 321c9e5 into main Nov 26, 2025
10 checks passed

nytian deleted the nytian/analyzer-bug branch November 26, 2025 18:17

dependabot bot mentioned this pull request Dec 8, 2025

[nuget][Find]- Bump Microsoft.DurableTask.Client from 1.17.1 to 1.18.0 DFE-Digital/single-unique-identifier#247

Closed

dependabot bot mentioned this pull request Dec 22, 2025

[nuget][Find]- Bump Microsoft.DurableTask.Client from 1.17.1 to 1.18.1 DFE-Digital/single-unique-identifier#328

Closed

dependabot bot mentioned this pull request Jan 26, 2026

[nuget][Find]- Bump Microsoft.DurableTask.Client from 1.17.1 to 1.19.1 DFE-Digital/single-unique-identifier#368

Closed

dependabot bot mentioned this pull request Feb 3, 2026

[nuget][Find]- Bump Microsoft.DurableTask.Client from 1.17.1 to 1.20.1 DFE-Digital/single-unique-identifier#391

Closed

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!

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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants