Skip to content

Fix ReflectionProvider bug on inheritance #470

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Daniel-Svensson wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Fix ReflectionProvider bug on inheritance #470

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
14f7867
Fix: System.InvalidOperationException : The declared derived type 'a'…
Daniel-Svensson Jun 25, 2026
e1edf44
Simplify where
Daniel-Svensson Jul 2, 2026
53b7c7e
Apply suggestion from @Daniel-Svensson
Daniel-Svensson Jul 2, 2026
3fe9bcc
Update src/PolyType/ReflectionProvider/ReflectionTypeShapeProvider.cs
Daniel-Svensson Jul 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions src/PolyType/ReflectionProvider/ReflectionTypeShapeProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,22 +294,23 @@ private IUnionTypeShape CreateUnionTypeShape(Type unionType, FSharpUnionInfo? fS
return (IUnionTypeShape)Activator.CreateInstance(fsharpUnionTypeTy, fSharpUnionInfo, this, options)!;
}

List<DerivedTypeShapeAttribute> derivedTypeAttributes = unionType.GetCustomAttributes<DerivedTypeShapeAttribute>().ToList();
List<DerivedTypeShapeAttribute> derivedTypeAttributes = unionType.GetCustomAttributes<DerivedTypeShapeAttribute>(inherit: false).ToList();

// Honor KnownTypeAttribute annotations only when no DerivedTypeShapeAttribute is present,
// which takes precedence over KnownTypeAttribute.
if (derivedTypeAttributes.Count == 0)
{
var mappedKnownTypeAttributes = unionType.GetCustomAttributes<KnownTypeAttribute>()
var mappedKnownTypeAttributes = unionType.GetCustomAttributes<KnownTypeAttribute>(inherit: false)
.Select(attr =>
{
if (attr.Type is null)
{
throw new NotSupportedException("KnownTypeAttribute annotations using methods are not supported.");
}

return new DerivedTypeShapeAttribute(attr.Type);
});
return attr.Type;
})
.Select(t => new DerivedTypeShapeAttribute(t));

derivedTypeAttributes.AddRange(mappedKnownTypeAttributes);
}
Expand Down
24 changes: 22 additions & 2 deletions tests/PolyType.Tests/DataContractShapeTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,9 +89,19 @@ public void KnownTypeAttribute_ReportsUnionShape()
{
var shape = Assert.IsType<IUnionTypeShape>(providerUnderTest.Provider.GetTypeShape(typeof(Animal)), exactMatch: false);

Assert.Equal(2, shape.UnionCases.Count);
Assert.Equal(3, shape.UnionCases.Count);
Assert.Contains(shape.UnionCases, c => c.UnionCaseType.Type == typeof(Dog));
Assert.Contains(shape.UnionCases, c => c.UnionCaseType.Type == typeof(Cat));
Assert.Contains(shape.UnionCases, c => c.UnionCaseType.Type == typeof(PersianCat));
}

[Fact]
public void KnownTypeAttribute_DerivedTypesReportsUnionShape()

@eiriktsarpalis eiriktsarpalis Jul 2, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this test for the bug you fixed?

@Daniel-Svensson Daniel-Svensson Jul 2, 2026 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Getting the type shape for cat would throw InvalidOperationException. saying that Dog is not assignable to Cat

It would never reach the asserts.

@Daniel-Svensson Daniel-Svensson Jul 2, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not see any similar testcases for DerivedTypeShapeAttributes, if you want separate tests to verify DerivedTypeShapeAttributes please give some advice on where they make sense to add

@eiriktsarpalis eiriktsarpalis Jul 2, 2026 •
edited
Loading

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't you use Assert.Throws?

@Daniel-Svensson Daniel-Svensson Jul 6, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eiriktsarpalis can you please elaborate a bit, I am new to xunit, why would I use Assert.Throws to check that a method call does not throw ?

I verified that the test fails without the code fix, since GetTypeShape would throw an an exception.

In addition I check the union cases to match KnownTypeAttribute_ReportsUnionShape above since originally all cases from the base class was discovered and added, so It would guard against Cat beeing a union case.

@eiriktsarpalis eiriktsarpalis Jul 6, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I understood you had said the opposite. As mentioned in #470 (comment) I would still prefer it if we threw an exception in this case to make sure user misconfiguration is caught immediately.

@Daniel-Svensson Daniel-Svensson Jul 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I think there is some confusion since the PR did 2 things.

  1. The most important part is fix a problem when it threw an exception when it should not.

The test verifies that the following works with both KnownType (the same problem is fixed for DerivedType as well) attributes. The problem only affected ReflectionProvider but works with GenerateShapeAttribute.
Note that the attributes for "PersianCat" is applied to both "Animal" and "Cat ", (which is not required by DataContractSerializer) but match what is required by json attributes and the PolyType DerivedType when used with source generator. Which is how I understand you want it to work.

    [DataContract]
    [KnownType(typeof(Dog))]
    [KnownType(typeof(Cat))]
    [KnownType(typeof(PersianCat))]
    public partial class Animal
    {
        [DataMember(Order = 0)] public string? Name { get; set; }
        [DataMember(Order = 1)] public bool Barks { get; set; }
    }

    [GenerateShape]
    [DataContract]
    [KnownType(typeof(PersianCat))]
    public partial class Cat : Animal
    {
        [DataMember(Order = 1)] public int Lives { get; set; }
    }

    [DataContract]
    public class PersianCat : Cat
    {
        [DataMember(Order = 2)] public string? FurColor { get; set; }
        [DataMember(Order = 3)] public bool RequiresGrooming { get; set; }
    }
  1. It stopped throwing for KnownTypeAttributes that was used in "AssociatedType" scenarios which is fully supported by DataContractSerializer. That support was removed so it will give error just as before.

{
var shape = Assert.IsType<IUnionTypeShape>(providerUnderTest.Provider.GetTypeShape(typeof(Cat)), exactMatch: false);

Assert.Single(shape.UnionCases);
Assert.Contains(shape.UnionCases, c => c.UnionCaseType.Type == typeof(PersianCat));
}

[Fact]
Expand Down Expand Up @@ -193,6 +203,7 @@ public enum ContractEnum
[DataContract]
[KnownType(typeof(Dog))]
[KnownType(typeof(Cat))]
[KnownType(typeof(PersianCat))]
public partial class Animal
{
[DataMember(Order = 0)] public string? Name { get; set; }
Expand All @@ -204,12 +215,21 @@ public class Dog : Animal
[DataMember(Order = 1)] public bool Barks { get; set; }
}

[GenerateShape]
[DataContract]
public class Cat : Animal
[KnownType(typeof(PersianCat))]
public partial class Cat : Animal
{
[DataMember(Order = 1)] public int Lives { get; set; }
}

[DataContract]
public class PersianCat : Cat
{
[DataMember(Order = 2)] public string? FurColor { get; set; }
[DataMember(Order = 3)] public bool RequiresGrooming { get; set; }
}

[GenerateShape]
[KnownType(typeof(DogNoDataContract))]
public partial class AnimalNoDataContract
Expand Down