-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
[API Proposal]: some way (attribute?) to mark types as constructed when used in casts #112287
Comments
This repro prints Is the example missing some important detail? |
Mmh... Have you tried on NativeAOT? The issue shouldn't be specific to that, but I haven't personally tried with CoreCLR + trimming myself. @manodasanW the minimal example looks correct to you too, right? Trying to think about what difference could there be between this and what CsWinRT is doing exactly but it generally feels equivalent to me? 🤔 |
Yes, I have tried with native. This is the output with
|
My bad, yeah the repro wasn't complete. I've updated it and I can also repro locally now 😅 |
Narrowed it down to the abstract attribute. That is, this repros (lookup abstract class ObjectFactoryAttribute : Attribute
{
public abstract object CreateInstance(IntPtr obj);
}
class MyObjectFactoryAttribute : ObjectFactoryAttribute
{
public override object CreateInstance(IntPtr obj) => new MyObject();
} This doesn't (lookup class MyObjectFactoryAttribute : Attribute
{
public object CreateInstance(IntPtr obj) => new MyObject();
} |
Also worth noting, we explicitly do not want to always preserve types having an attribute that is being used as type argument for |
Castability can be observed by the application in many different ways. For example, through |
Instead of limiting this to castability, could the feature instead more generally say "mentions of this type must result in a fully-constructed type symbol"? This would effectively remove the "Necessary" type symbol optimization for these types. That would add their custom attributes to the dependency graph when the type is directly referenced elsewhere, rooting the constructor as part of rooting the custom attribute info. That way it would cover |
That seems reasonable to me. Would it make sense to make this a general feature? Something like (placeholder name): [AttributeUsage(AttributeTargets.All, Inherited = false)]
internal sealed class DynamicDependencyModifiersAttribute: Attribute
{
public DynamicDependencyModifiersAttribute(DynamicDependencyModifier modifier);
public DynamicDependencyModifier Modifier { get; }
}
[Flags]
public enum DynamicDependencyModifier
{
None = 0,
TypeReferenceImpliesConstructed = 1
} So that in the future we could easily add more modifiers to this enum to support more scenarios, if needed, without the need for new attributes. And for this scenario in CsWinRT, we'd just use
As long as this doesn't cause false positives resulting in all projection types being rooted, that would be fine for CsWinRT 🙂 |
My main question for trimming related API proposals is: can this be used without trim warning suppressions?
It's difficult to find all mentions of a type. For example if a type was used as a field type: we don't (currently) trim fields so this is a "mention" no matter if the field is ever written/read. We only look at the field as part of calculating type layout - and type layout has no connection to dependency analysis right now. So we'd need extra dependency analysis passes to find "mentions" in all kinds of places. The compiler is currently not in the business of finding all IL constructs that were looked at, but in the business of figuring out what code/data structures need to be generated. An unused field type currently only contributes to vacated space in terms of output data structures and the compiler is not well equipped to track it right now. There will be lots of other "mentions" that we'll need to collect in unrelated places (e.g. type of a local variable, type used as generic argument, type used as part of a function pointer signature in a calli, etc.) and every place we forgot to collect will be a bug. We don't have a single choke point for "mentions". |
The way to provide a scenario here without Type.GetType would be to use the Interop Type Mapping proposal, which would provide the trim-safe 1:1 type lookup. That proposal also depends on at least some level of trimming out unused types as the foreign type universe projections are expensive. Maybe we could put the two together? Basically say "if this type is mapped in from a foreign type universe and is present in the AOT image, it's fully constructed". |
Could you clarify how that would work in this case? As in, suppose we have the projected types |
Basically, if Then, when a "type symbol" is requested for |
A few of the ideas we have for the interop type mapping proposal have led us back to the idea of "call GetCustomAttribute on the type returned from the interop type mapping" as our guidance for getting auxiliary data, so we may actually need this for that anyway in some capacity. |
I have to agree with @MichalStrehovsky here - whatever the solution is, it should not require warning suppression. Currently there's no solution (without warning suppression) to the problem of some external source (WinRT, Java, objC) effectively instantiating .NET types. This proposal doesn't fix that, since the code which calls the instantiation would still get warnings. There's no tie between the annotation, the site which creates the instance and the "type map"/"cast" which proves that the type is actually used and kept. |
We were talking about this a bit with the team. One thing that came up with this heuristic is this:
Are these possible scenarios we need to worry about? |
These scenarios are solved from the CsWinRT side, as follows:
That will work (assuming When we go to create an RCW for a given native object, we do the following:
So basically, the actual managed object you get is always the most derived projection type that wasn't trimmed.
That will still work, because all projected types implement |
Closing in deference to #110691 |
Background and motivation
We have a trimming issue in CsWinRT which users (both internal and not) keep hitting. The problem is that we have projected types (concrete classes) used in a cast expression, and never directly instantiated. This type is instantiated earlier, when we create an RCW for a native object, if the runtime class name that object returns matches the type name of a projected class that CsWinRT generated. If that's the case, we then lookup
[WinRTImplementationTypeRcwFactoryAttribute]
on that type, and we use that to construct the actual RCW instance, via its factory method. If there is no referenced projected class matching the returned runtime class name, then we just return a genericIInspectable
object, which is not relevant here.The problem is that the type is only constructed via the attribute, which is on the type itself, and the type is never directly instantiated. So ILC sees the cast, assumes the type can't be constructed (as it ignores that cycle), and the cast always fails, even when it should succeed (because the wrapped native object is that desired projected type). We don't want to root all types with the factory, as that'd be terrible for trimming. But we do need a way to make it so that such types are marked as constructed, when they're used in a cast. Basically, casts should succeed if we have a native object matching a referenced projected class.
Here's some examples:
Concrete example
The scenario is basically this:
We need a way to tell the linker/ILC:
API Proposal
Note
The exact attribute name is a placeholder, it doesn't matter. Happy to update the proposal with any suggestions.
API Usage
In the example above, we'd simply add the attribute on
MyObject
, and things would work as expected.In CsWinRT, we'll update our projections to include the attribute for any projected class also using our factory attribute.
Alternative Designs
Alternatives we considered:
[DynamicDependency]
in a module initializer. This is suboptimal for trimming, as it roots unconditionally.[DynamicDependency]
on the containing method. We don't want users to have to see this implementation detail.Cast<T>
method instead, which happens to root the type. This is awful, we want people to be able to use idiomatic C#.Risks
None that I can see. It adds a bit of cognitive overhead to users, but it's a super niche feature only needed by library authors.
cc. @MichalStrehovsky
The text was updated successfully, but these errors were encountered: