Skip to content

Resolve ILLink warnings for System.Resources.ResourceManager#47778

Merged
13 commits merged intodotnet:masterfrom
joperezr:ResourceManagerTrimSafe
Feb 8, 2021
Merged

Resolve ILLink warnings for System.Resources.ResourceManager#47778
13 commits merged intodotnet:masterfrom
joperezr:ResourceManagerTrimSafe

Conversation

@joperezr
Copy link
Member

@joperezr joperezr commented Feb 2, 2021

cc: @eerhardt @marek-safar @vitek-karas @ericstj

fixes #45272
fixes #32862

Resolving remaining ILLinker warnings on System.Resources.ResourceManager.

This will introduce a feature switch System.Resources.AllowReflectionForNonPrimitiveObjects that can be set via AppContext which will control whether or not you can use ResourceManager to read/write non primitive types. By default, we are thinking of setting this switch to false when publishing a trimmed app, as it won't be safe as the trimmer won't be able to tell which types are present on the resources stream and need to be preserved. For published apps that manually specify this switch to true They should get a warning saying that some code paths might require unreferenced code, so that they make sure to keep the types they expect to use and then disable the warning if appropriate. The sdk PR setting this switch as default will come after this PR gets merged.

@joperezr joperezr requested a review from eerhardt February 2, 2021 18:26
@ghost ghost added the area-System.Resources label Feb 2, 2021
@ghost
Copy link

ghost commented Feb 2, 2021 •
edited by joperezr
Loading

Tagging subscribers to this area: @tarekgh, @buyaa-n, @krwq
See info in area-owners.md if you want to be subscribed.

Issue Details

cc: @eerhardt @marek-safar @vitek-karas @ericstj

fixes #45272
fixes #32862

Resolving remaining ILLinker warnings on System.Resources.ResourceManager.

This will introduce a feature switch System.Resources.AllowReflectionForNonPrimitiveObjects that can be set via AppContext which will control whether or not you can use ResourceManager to read/write non primitive types. By default, we are thinking of setting this switch to false when publishing a trimmed app, as it won't be safe as the trimmer won't be able to tell which types are present on the resources stream and need to be preserved. For published apps that manually specify this switch to true They should get a warning saying that some code paths might require unreferenced code, so that they make sure to keep the types they expect to use and then disable the warning if appropriate. The sdk PR setting this switch as default will come after this PR gets merged.

Author: joperezr
Assignees: -
Labels:

area-System.Resources
Milestone: -
#Resolved

jkotas
jkotas reviewed Feb 2, 2021
View reviewed changes
@joperezr joperezr requested a review from ericstj February 2, 2021 20:52
@eerhardt eerhardt added the linkable-framework Issues associated with delivering a linker friendly framework label Feb 2, 2021
@ghost
Copy link

ghost commented Feb 2, 2021 •
edited by joperezr
Loading

Tagging subscribers to 'linkable-framework': @eerhardt, @vitek-karas, @LakshanF, @tannergooding, @sbomer
See info in area-owners.md if you want to be subscribed.

Issue Details

cc: @eerhardt @marek-safar @vitek-karas @ericstj

fixes #45272
fixes #32862

Resolving remaining ILLinker warnings on System.Resources.ResourceManager.

This will introduce a feature switch System.Resources.AllowReflectionForNonPrimitiveObjects that can be set via AppContext which will control whether or not you can use ResourceManager to read/write non primitive types. By default, we are thinking of setting this switch to false when publishing a trimmed app, as it won't be safe as the trimmer won't be able to tell which types are present on the resources stream and need to be preserved. For published apps that manually specify this switch to true They should get a warning saying that some code paths might require unreferenced code, so that they make sure to keep the types they expect to use and then disable the warning if appropriate. The sdk PR setting this switch as default will come after this PR gets merged.

Author: joperezr
Assignees: -
Labels:

area-System.Resources, linkable-framework
Milestone: -
#Resolved

eerhardt
eerhardt reviewed Feb 2, 2021
View reviewed changes
Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

Thanks for tackling this @joperezr!

marek-safar
marek-safar reviewed Feb 3, 2021
View reviewed changes
@vitek-karas vitek-karas mentioned this pull request Feb 3, 2021
Merged
vitek-karas
vitek-karas approved these changes Feb 4, 2021
View reviewed changes
jkotas
jkotas reviewed Feb 4, 2021
View reviewed changes
jkotas
jkotas reviewed Feb 4, 2021
View reviewed changes
@eerhardt
Copy link
Member

eerhardt commented Feb 4, 2021 •
edited by joperezr
Loading

CustomType.resources is a binary file. Is it possible to use a .resx file instead? Maybe with a bitmap in it or something. That way we aren't committing binary files. #Resolved

@eerhardt
Copy link
Member

eerhardt commented Feb 4, 2021 •
edited by joperezr
Loading 

        if (!_permitDeserialization)

We should be checking AllowCustomResourceTypes here as well. That way we can trim all the code below. #Resolved

Refers to: src/libraries/System.Private.CoreLib/src/System/Resources/ResourceReader.Core.cs:50 in 55fd1fc. [](commit_id = 55fd1fc, deletion_comment = False)

@dotnet dotnet deleted a comment Feb 5, 2021
@dotnet dotnet deleted a comment Feb 5, 2021
eerhardt
eerhardt approved these changes Feb 5, 2021
View reviewed changes
Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

Looks great to me. Thanks for the great work here, @joperezr.

I just had one minor nit about the ordering to allow a tiny more bit of code to be trimmed. But I believe this is ready to be merged.

@ghost
Copy link

ghost commented Feb 5, 2021

Hello @joperezr!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit 744f1ff into dotnet:master Feb 8, 2021
@joperezr joperezr deleted the ResourceManagerTrimSafe branch February 8, 2021 20:46
@marek-safar
Copy link
Contributor

marek-safar commented Feb 19, 2021

@joperezr how is the "The sdk PR setting this switch as default will come after this PR gets merged." coming along?

@joperezr
Copy link
Member Author

joperezr commented Feb 19, 2021

It’s merged: dotnet/sdk#15702

sorry that I didn’t link it here before.

@eerhardt eerhardt mentioned this pull request Mar 3, 2021
Merged
@ghost ghost locked as resolved and limited conversation to collaborators Mar 21, 2021
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jkotas jkotas jkotas left review comments

@ViktorHofer ViktorHofer ViktorHofer left review comments

@tannergooding tannergooding tannergooding left review comments

@tarekgh tarekgh tarekgh left review comments

@marek-safar marek-safar marek-safar approved these changes

@eerhardt eerhardt eerhardt approved these changes

@vitek-karas vitek-karas vitek-karas approved these changes

@ericstj ericstj Awaiting requested review from ericstj

Assignees

No one assigned

Labels

area-System.Resources linkable-framework Issues associated with delivering a linker friendly framework

Projects

No open projects
Triage POD for Reflection, META, etc.
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add feature switch to disable custom .resources reader support (UserResourceSet) ResourceManager contains code which ILLink can't analyze

9 participants

@joperezr @eerhardt @marek-safar @jkotas @ViktorHofer @tannergooding @vitek-karas @tarekgh @krwq