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

Handle open types to appear in interface maps #97733

Merged
merged 2 commits into from
Feb 8, 2024

Conversation

davidwrighton
Copy link
Member

Fix the following scenarios

  • Reflection IsAssignableFrom api
  • As well as constraint checking

Fixes #97667

- Reflection IsAssignableFrom api
- As well as constraint checking
@lambdageek
Copy link
Member

COM interop also has a call to IsSpecialMarkerTypeForGenericCasting

if (pItf->HasInstantiation() || pItf->IsSpecialMarkerTypeForGenericCasting())
continue;

COM can't have the curiously recurring generic scenario, though, right?
Should we assert anyway?

@davidwrighton
Copy link
Member Author

COM interop also has a call to IsSpecialMarkerTypeForGenericCasting

if (pItf->HasInstantiation() || pItf->IsSpecialMarkerTypeForGenericCasting())
continue;

COM can't have the curiously recurring generic scenario, though, right? Should we assert anyway?

Unfortunately, we can't effectively assert there. We might or might not have the flag set, and in either case it doesn't really make any sense in the presence of COM interop.

@lambdageek
Copy link
Member

@davidwrighton Can we do a call Thursday or Friday to review. I'm having some trouble following what's going on (new codebase + curiously recursive generics == 🤯 )

@davidwrighton
Copy link
Member Author

@lambdageek, absolutely, just schedule drop something onto my schedule

Copy link
Member

@lambdageek lambdageek left a comment

Choose a reason for hiding this comment

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

I also can't make the example convoluted enough to hit the variant case without making the bug go away (because I end up passing at least one generic type argument that is not just a generic parameter - so we don't get the typical instantiation).

The code as written matches what we thought it ought to do during the code review 😄

@davidwrighton davidwrighton merged commit aa4df1f into dotnet:main Feb 8, 2024
121 checks passed
@davidwrighton
Copy link
Member Author

/backport to release/8.0-staging

Copy link
Contributor

github-actions bot commented Feb 8, 2024

Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/7834567349

Copy link
Contributor

github-actions bot commented Feb 8, 2024

@davidwrighton backporting to release/8.0-staging failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Handle open types to appear in interface maps - Reflection IsAssignableFrom api - As well as constraint checking
Using index info to reconstruct a base tree...
M	src/coreclr/vm/methodtable.cpp
M	src/coreclr/vm/methodtable.h
M	src/coreclr/vm/methodtable.inl
M	src/coreclr/vm/methodtablebuilder.cpp
M	src/tests/Loader/classloader/generics/Instantiation/Positive/CuriouslyRecurringThroughInterface.cs
Falling back to patching base and 3-way merge...
Auto-merging src/tests/Loader/classloader/generics/Instantiation/Positive/CuriouslyRecurringThroughInterface.cs
CONFLICT (content): Merge conflict in src/tests/Loader/classloader/generics/Instantiation/Positive/CuriouslyRecurringThroughInterface.cs
Auto-merging src/coreclr/vm/methodtablebuilder.cpp
Auto-merging src/coreclr/vm/methodtable.inl
Auto-merging src/coreclr/vm/methodtable.h
CONFLICT (content): Merge conflict in src/coreclr/vm/methodtable.h
Auto-merging src/coreclr/vm/methodtable.cpp
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Handle open types to appear in interface maps - Reflection IsAssignableFrom api - As well as constraint checking
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

Copy link
Contributor

github-actions bot commented Feb 8, 2024

@davidwrighton an error occurred while backporting to release/8.0-staging, please check the run log for details!

Error: git am failed, most likely due to a merge conflict.

davidwrighton added a commit to davidwrighton/runtime that referenced this pull request Feb 8, 2024
…) to release/8.0-staging

- Reflection IsAssignableFrom api
- As well as constraint checking
jeffschwMSFT pushed a commit that referenced this pull request Feb 9, 2024
… release/8.0-staging (#98182)

- Reflection IsAssignableFrom api
- As well as constraint checking
@github-actions github-actions bot locked and limited conversation to collaborators Mar 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TypeLoadException on minimal, correct and compiling program
2 participants