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

Determine solution for nullable attributes and the new public API to access them #1991

Closed
vitek-karas opened this issue Apr 23, 2021 · 5 comments

Comments

@vitek-karas
Copy link
Member

We will need to come up with a linker solution for the new API proposed here: dotnet/runtime#29723.

@eerhardt
Copy link
Member

eerhardt commented Jul 2, 2021

I had a thought here, that I am copying below:

One thought I had here is if we could annotate these new NullabilityInfoContext APIs with something like:

[RequiresFeature("System.Reflection.NullabilityInfo")]
public NullabilityInfo Create(ParameterInfo parameterInfo) {}

And what this would mean is:

When the linker sees a method with RequiresFeature attribute being called, it would "turn on that feature switch".

Then in ILLink.LinkAttributes.xml, we would put all these RemoveAttributeInstances requests behind the System.Reflection.NullabilityInfo feature switch:

https://github.com/dotnet/runtime/blob/eeadfdb2f57e607241b9b09ada1ef08717e618f6/src/mono/System.Private.CoreLib/src/ILLink/ILLink.LinkAttributes.xml#L133-L165

This would mean, if the feature switch wasn't "on", the attributes could be trimmed safely. If the feature switch was "on", then the attributes won't be trimmed. And then the APIs would work just fine in a trimmed app.

And in the off case where someone was reading the attributes directly, not through these APIs, they could turn the feature switch on for the app in the usual way, and the attributes wouldn't be trimmed.

@vitek-karas
Copy link
Member Author

Thanks for the idea @eerhardt. I like:

  • It makes things just work
  • Could be used in other places as well

What I don't like:

  • Would introduce lot of complexity to the linker. Currently there are several places where linker assumes that feature switches are all specified up front (branch removal, constant prop, ...). To support this proposal linker would have to be able to retroactively change FS values, which would be VERY tricky to implement (or it would require two passes).
  • Requires new public API

@sbomer
Copy link
Member

sbomer commented Jul 7, 2021

Expanding on what @MichalStrehovsky mentioned in https://github.com/mono/linker/issues/2078: We should also consider the GetCustomAttributes APIs. If we put different attributes under different feature switches, then GetCustomAttributes will only be "safe" when all of those feature switches are enabled (all the attributes are kept). For example

foreach (var a in typeof(Foo).GetCustomAttributes()) // ...

This should either keep all custom attributes on Foo, or issue a warning.

@MichalStrehovsky
Copy link
Member

@vitek-karas Can this be closed since we have an extra new feature switch to make this one API work? We still have dotnet/runtime#103934 open tracking the fact that IL Linker is breaking user code without warnings when attribute stripping is enabled in general.

@vitek-karas
Copy link
Member Author

I think it can - the specific issue is resolved. And generic attribute trimming is... problematic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants