Skip to content

[mono] Assert that we don't need to inflate types when applying DIM overrides #74379

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
lambdageek merged 9 commits into from
Aug 25, 2022
Merged

[mono] Assert that we don't need to inflate types when applying DIM overrides #74379

lambdageek merged 9 commits into from
Aug 25, 2022

Conversation

@lambdageek
Copy link
Member

@lambdageek lambdageek commented Aug 22, 2022 •
edited
Loading

Due to rearranging the logic in #64102 we already inflated the interfaces that are generic instances. Inflating again is wrong and will use the wrong generic context.

Fixes #70190

@ghost ghost added the area-VM-meta-mono label Aug 22, 2022
@ghost ghost assigned lambdageek Aug 22, 2022
@lambdageek lambdageek added NO-REVIEW Experimental/testing PR, do NOT review it NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) area-VM-meta-mono and removed area-VM-meta-mono labels Aug 22, 2022
@lambdageek lambdageek changed the title [NO MERGE] mono inflate overrides add assertion [mono] Assert that we don't need to inflate types when applying DIM overrides Aug 23, 2022
@lambdageek lambdageek removed NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) NO-REVIEW Experimental/testing PR, do NOT review it labels Aug 23, 2022
@lambdageek lambdageek marked this pull request as ready for review August 23, 2022 19:34
@lambdageek
Copy link
Member Author

lambdageek commented Aug 23, 2022 •
edited
Loading

Update they still fail. So I just added a link to their tracking issue.

Previously:
I tried to uncomment some unrelated DIM tests and they pass locally with the JIT, but I don't think I'm addressing them - so they'll probably fail on AOT or something. Just want to see if that's the case.

@lambdageek
Copy link
Member Author

lambdageek commented Aug 23, 2022

@thaystg @vargaz this is ready for review, I think.

@SamMonoRT I think this is probably worth backporting to net7

@SamMonoRT
Copy link
Member

SamMonoRT commented Aug 23, 2022

@thaystg @vargaz this is ready for review, I think.

@SamMonoRT I think this is probably worth backporting to net7

Yes, we should backport this change.

@runfoapp runfoapp bot mentioned this pull request Aug 24, 2022
Closed
This was referenced Aug 24, 2022
Open
Closed
@lambdageek
Copy link
Member Author

lambdageek commented Aug 24, 2022

/backport to release/7.0

@github-actions
Copy link
Contributor

github-actions bot commented Aug 24, 2022

Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/2921758914

@github-actions github-actions bot mentioned this pull request Aug 24, 2022
Merged
fanyang-mono
fanyang-mono approved these changes Aug 24, 2022
View reviewed changes
Copy link
Member

@fanyang-mono fanyang-mono left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The change looks good to me!

@lambdageek lambdageek merged commit 33fddbd into dotnet:main Aug 25, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Sep 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@thaystg thaystg thaystg approved these changes

@fanyang-mono fanyang-mono fanyang-mono approved these changes

@vargaz vargaz Awaiting requested review from vargaz

@lateralusX lateralusX Awaiting requested review from lateralusX

Assignees

@lambdageek lambdageek

Labels

area-VM-meta-mono

Projects

None yet

Milestone

No milestone

4 participants

@lambdageek @SamMonoRT @thaystg @fanyang-mono