Skip to content
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.

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

NativeAOT does not honor new() constraint #80037

Closed
kant2002 opened this issue Dec 29, 2022 · 8 comments
Closed

NativeAOT does not honor new() constraint #80037

kant2002 opened this issue Dec 29, 2022 · 8 comments

Comments

@kant2002
Copy link
Contributor

Description

NativeAOT does not honor new() constraint applied to interface.

Reproduction Steps

using System.Diagnostics.CodeAnalysis;

var serviceCollection = new ServiceCollection();
var w = serviceCollection.GetRequiredService<IGeneric<Target>>();
Console.WriteLine("Working");


class ServiceCollection
{
    public T GetRequiredService<T>()
    {
        return (T)Activator.CreateInstance(typeof(GenericInstantiaion<>).MakeGenericType(typeof(T).GenericTypeArguments[0]))!;
    }
}

interface IGeneric<T> where T: new() {}
class GenericInstantiaion<T> : IGeneric<T> where T: new()
{
    public GenericInstantiaion()
    {
        new T();
    }
}

public class Target {

}

Expected behavior

Do not trim constructor of Target class.

Actual behavior

Exception at runtime.

Regression?

No

Known Workarounds

No response

Configuration

No response

Other information

No response

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Dec 29, 2022
@ghost
Copy link

ghost commented Dec 29, 2022

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

Issue Details

Description

NativeAOT does not honor new() constraint applied to interface.

Reproduction Steps

using System.Diagnostics.CodeAnalysis;

var serviceCollection = new ServiceCollection();
var w = serviceCollection.GetRequiredService<IGeneric<Target>>();
Console.WriteLine("Working");


class ServiceCollection
{
    public T GetRequiredService<T>()
    {
        return (T)Activator.CreateInstance(typeof(GenericInstantiaion<>).MakeGenericType(typeof(T).GenericTypeArguments[0]))!;
    }
}

interface IGeneric<T> where T: new() {}
class GenericInstantiaion<T> : IGeneric<T> where T: new()
{
    public GenericInstantiaion()
    {
        new T();
    }
}

public class Target {

}

Expected behavior

Do not trim constructor of Target class.

Actual behavior

Exception at runtime.

Regression?

No

Known Workarounds

No response

Configuration

No response

Other information

No response

Author: kant2002
Assignees: -
Labels:

area-NativeAOT-coreclr

Milestone: -

@MichalStrehovsky
Copy link
Member

Thanks! Here's a repro that doesn't generate trimming warnings (and is also broken):

IGeneric<Target>.Hey();


interface IGeneric<T> where T: new()
{
    public static void Hey()
    {
        Activator.CreateInstance(typeof(T));
    }

}

public class Target { }

@kant2002
Copy link
Contributor Author

I think my sample closer to how open generic registered in MEDI. Your example probably different issue :)

@MichalStrehovsky
Copy link
Member

The issue would be a won't fix if it required MakeGeneric, especially if it's with something that has a new constraint. If there's a warning, trimming can and will generate broken things. The only aspects that are analysis holes are if there's no warnings.

@kant2002
Copy link
Contributor Author

I do not want replicate whole DI setup. That’s bigger sample with only one warning from BuildServiceProvider which I believe is fine.

using System.Diagnostics.CodeAnalysis;
using Microsoft.Extensions.DependencyInjection;

var serviceCollection = new ServiceCollection();
serviceCollection.AddSingleton(typeof(IGeneric<>),typeof(GenericInstantiaion<>));
var serviceProvider = serviceCollection.BuildServiceProvider();
var w = serviceProvider.GetRequiredService<IGeneric<Target>>();
Console.WriteLine("Working");




interface IGeneric<T> where T: new() {}
class GenericInstantiaion<T> : IGeneric<T> where T: new()
{
    public GenericInstantiaion()
    {
        new T();
    }
}

public class Target {

}

@kant2002
Copy link
Contributor Author

We can have situations like in MEDI where library author would support similar pattern for construction as in medi for open generics and just suppress warning like here

[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2055:MakeGenericType",
Justification = "MakeGenericType here is used to create a closed generic implementation type given the closed service type. " +
"Trimming annotations on the generic types are verified when 'Microsoft.Extensions.DependencyInjection.VerifyOpenGenericServiceTrimmability' is set, which is set by default when PublishTrimmed=true. " +
"That check informs developers when these generic types don't have compatible trimming annotations.")]
private ServiceCallSite? TryCreateOpenGeneric(ServiceDescriptor descriptor, Type serviceType, CallSiteChain callSiteChain, int slot, bool throwOnConstraintViolation)

I think it's reasonable to perform analysys on parameters and return types of method when they specified. Develop specify that he need type, and that type guaranteed to return specific type by compiler, so that means that constraints on specific generic type would be satisfied. I honestly did not see how developer can break something with this extra requirement.

Yes, internally it's MakeGenericType probably, but why I cannot suppress usage if I know what I'm doing?

@MichalStrehovsky
Copy link
Member

I do not want replicate whole DI setup. That’s bigger sample with only one warning from BuildServiceProvider which I believe is fine.

Oh, I see, it's the pattern from DI. Yes, the suppression in DI relies on the fact that because the interface has a new() constraint and there's a concrete instantiation of the interface with type X, we will not trim the constructor of X. The reduced sample will cover that and it is the same issue. The compiler is not great at detecting all the places where generic instantiations happen.

@MichalStrehovsky
Copy link
Member

Vitek fixed this in #82818.

@ghost ghost locked as resolved and limited conversation to collaborators Apr 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Archived in project
Development

No branches or pull requests

2 participants