Skip to content

Make ComponentModel intrinsic TypeConverters linker-safe #39973

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.

Sign up for GitHub

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

Jump to bottom
Merged
layomia merged 8 commits into from
Aug 7, 2020
Merged

Make ComponentModel intrinsic TypeConverters linker-safe #39973

layomia merged 8 commits into from
Aug 7, 2020

Conversation

@layomia
Copy link
Contributor

@layomia layomia commented Jul 27, 2020

Fixes #38582

@layomia layomia added this to the 5.0.0 milestone Jul 27, 2020
@layomia layomia self-assigned this Jul 27, 2020
@ghost
Copy link

ghost commented Jul 27, 2020

Tagging subscribers to this area: @safern
See info in area-owners.md if you want to be subscribed.

@eerhardt
Copy link
Member

eerhardt commented Jul 27, 2020

Can we add a couple tests for this?

@jaredpar jaredpar mentioned this pull request Jul 27, 2020
Closed
@layomia layomia requested a review from eerhardt August 6, 2020 14:39
@eerhardt
Copy link
Member

eerhardt commented Aug 6, 2020 •
edited
Loading 

            // Special case converters

Since this method is only used for editors now, can we:

  1. Rename the method to be symmetric with GetIntrinsicTypeConverter? e.g. GetIntrinsicTypeEditor
  2. Remove the Special case converters code.

Refers to: src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/ReflectTypeDescriptionProvider.cs:1336 in 072efce. [](commit_id = 072efce, deletion_comment = False)

eerhardt
eerhardt approved these changes Aug 6, 2020
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 good to me. Thanks for getting this taken care of. This should eliminate the need for the LinkerWorkaround.xml in Blazor now.

@layomia layomia merged commit 2d160e4 into dotnet:master Aug 7, 2020
@layomia layomia deleted the converters branch August 7, 2020 00:35
@layomia layomia mentioned this pull request Aug 7, 2020
Closed
Jacksondr5 pushed a commit to Jacksondr5/runtime that referenced this pull request Aug 10, 2020
@layomia @Jacksondr5
Make ComponentModel intrinsic TypeConverters linker-safe (dotnet#39973)
4096486 
* Make ComponentModel intrinsic TypeConverters linker-safe

* Avoid reflection in converter func lookup

* Re-add code path that supports editors

* Consolidate intrinsic converter caches, move converter-specific code to new method, add trimming tests

* Address feedback

* Revert change to GetIntrinsicTypeEditor method

* Handle inheritance with Uri and CultureInfo types

* Add heirarchy-related comments to cache docs
@eerhardt eerhardt mentioned this pull request Aug 10, 2020
Closed
4 tasks
@eerhardt eerhardt mentioned this pull request Aug 26, 2020
Closed
rolfbjarne added a commit to rolfbjarne/macios that referenced this pull request Aug 27, 2020
@rolfbjarne
[tests] Skip the TypeDescriptorCanary test in .NET.
24eb31b 
It tests internal implementation details, and those details have now changed
(dotnet/runtime#39973). So just skip the entire test
for now.

Fixes this test failure:

    DontLink.CommonDontLinkTest
        [FAIL] TypeDescriptorCanary : System.InvalidCastException : Specified cast is not valid. :    at DontLink.CommonDontLinkTest.TypeDescriptorCanary() in /Users/builder/jenkins/workspace/xamarin-macios-pr-builder/tests/linker/CommonDontLinkTest.cs:line 22
            at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
@rolfbjarne rolfbjarne mentioned this pull request Aug 27, 2020
Merged
rolfbjarne added a commit to dotnet/macios that referenced this pull request Aug 27, 2020
@rolfbjarne
[tests] Skip the TypeDescriptorCanary test in .NET. (#9527)
d5ddc92 
It tests internal implementation details, and those details have now changed
(dotnet/runtime#39973). So just skip the entire test
for now.

Fixes this test failure:

    DontLink.CommonDontLinkTest
        [FAIL] TypeDescriptorCanary : System.InvalidCastException : Specified cast is not valid. :    at DontLink.CommonDontLinkTest.TypeDescriptorCanary() in /Users/builder/jenkins/workspace/xamarin-macios-pr-builder/tests/linker/CommonDontLinkTest.cs:line 22
            at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
@eerhardt eerhardt mentioned this pull request Sep 9, 2020
Closed
@pranavkm pranavkm mentioned this pull request Sep 9, 2020
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@danmoseley danmoseley danmoseley left review comments

@eerhardt eerhardt eerhardt approved these changes

@MichalStrehovsky MichalStrehovsky Awaiting requested review from MichalStrehovsky

+1 more reviewer

@pranavkm pranavkm pranavkm left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@layomia layomia

Labels

area-System.ComponentModel

Projects

None yet

Milestone

5.0.0

Development

Successfully merging this pull request may close these issues.

ComponentModel IntrinsicTypeConverters are not linker safe

4 participants

@layomia @eerhardt @pranavkm @danmoseley