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

Attribute stripping #103934

Open
MichalStrehovsky opened this issue Jun 3, 2021 · 5 comments
Open

Attribute stripping #103934

MichalStrehovsky opened this issue Jun 3, 2021 · 5 comments
Labels
area-Tools-ILLink .NET linker development as well as trimming analyzers
Milestone

Comments

@MichalStrehovsky
Copy link
Member

With #29723 looking more and more real, I think we need to think about how attribute stripping can be done safely.

Attribute stripping is currently the only optimization in illink that I know of that will break user code without any warnings.

I think it would be interesting if we could scope attribute stripping down to places where we can provably do it safely. Illink has a good idea of what members are going to be reflected on. We could limit attribute stripping to places that we know are not going to be reflected on. I think we could still get pretty decent savings and APIs like the proposed nullability info decoder would not get broken by illink.

Cc @vitek-karas

@marek-safar
Copy link
Contributor

Duplicate of dotnet/linker#1991

@marek-safar marek-safar marked this as a duplicate of dotnet/linker#1991 Jun 3, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jun 24, 2024
@sbomer sbomer transferred this issue from dotnet/linker Jun 24, 2024
@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Jun 24, 2024
@sbomer sbomer added area-Tools-ILLink .NET linker development as well as trimming analyzers and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Jun 24, 2024
Copy link
Contributor

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

@agocke agocke modified the milestones: Future, 10.0.0 Jun 28, 2024
@agocke agocke removed the untriaged New issue has not been triaged by the area owner label Jun 28, 2024
@rolfbjarne
Copy link
Member

What would be the potential problem if attributes were trimmed away if the corresponding type is never referenced anywhere other than in attributes?

So for instance if no IL references System.ObsoleteAttribute, then all System.ObsoleteAttribute would be trimmed away.

Of course it's possible to construct corner cases using reflection, but I believe those should already show warnings:

public bool IsObsolete (Type type)
{
	return type.GetCustomAttributes ().Any (v => v.GetType ().Name == "ObsoleteAttribute");
}

@MichalStrehovsky
Copy link
Member Author

Of course it's possible to construct corner cases using reflection, but I believe those should already show warnings:

These don't show warnings. The GetCustomAttributes API is not annotated as trim unsafe and there's no analysis for it. So the snippet above should work, but won't.

We ran into this with nullable attributes because those are exactly in the category of attribute types that the Roslyn compiler emits into all assemblies (when targeting profiles that don't have the attribute in CoreLib that Roslyn could reference instead) and therefore cannot be reliably be checked for using typeof and everyone is looking for it by name. Other attributes could be checked with a typeof and we could detect that, but because of the above this is simply a breaking optimization.

The nullable attribute stripping actually caused tons of stress for the team around .NET 8 Preview 6 snap because Json started calling into nullable APIs (that are not declared trim unsafe so we didn't statically detect it) and then Blazor WASM SDK enabled attribute stripping and it broke everything several repos away. It took a while for the .NET team to root cause and fix, delayed build availability because of course this was checked in right before preview snap, and this was the .NET team with all the expertise we have. Imagine random Joe Developer hitting this. We want trimming to be a reliable optimization.

@rolfbjarne
Copy link
Member

We ran into this with nullable attributes because [...] everyone is looking for it by name.

Ugh, I see.

I guess a workaround would be to also preserve an attribute if its type name is found within any string in the app...

Going forward I wonder if it would be better to add reflection API for attributes that's easier to trim, say something like:

MemberInfo.GetCustomAttributes<T> () where T: System.Attribute;
MemberInfo.GetCustomAttributesByName (string name); // name can be either just the type's name or the full name, and the trimmer knows how to translate name to the actual type

but I can see this taking a long time to be used everywhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-Tools-ILLink .NET linker development as well as trimming analyzers
Projects
Status: No status
Development

No branches or pull requests

5 participants