Fix MethodImpl decl method processing#38310
Conversation
- Require a signature that matches the actual target method definition - Except for using MethodImpl records that point at base type - In that case, apply Substitution to provide a signature matching environment that matches the parent type in the MemberRef This change allows specifying an unambiguous target when there was previously potential for ambiguity in the presence of generics and MethodImpl records.
|
This change needs additional tests, as well as validation that this doesn't break any of the existing language compilers going back several versions. |
|
@janvorli This is the fix I've come up with for addressing issue #38119. Its a little more impactful than I had thought, so we'll need to write more extensive testing for it before we can consider checking it in. @lambdageek I'd like to make sure that once I get this put together I write a some tests that cover some of the corner cases of what this logic does. Could you point me at some documentation that would make it easy to run some of the new tests I'm going to write on Mono? |
Just add them to |
- It also throws a different exception for Test3 and Test5, but that's acceptable
|
@lambdageek, the new test passes on Mono for the 99% case, but for the scenario where the overridden method on the base type is moved to its parent type, when not running the interpreter, the mono runtime fails. I've added an exclusion to the issues.rsp, so to allow checkin, but I don't consider it to be a significant problem worth holding the work up. (Moving methods like that is extremely rare in practice, but it is considered a backwards compatible change, and the framework team has made such moves in the past.) |
janvorli
left a comment
janvorli
left a comment
There was a problem hiding this comment.
I have just a couple of nits.
|
|
||
| Substitution *pDeclSubst = &bmtMetaData->pMethodDeclSubsts[m]; | ||
| MethodSignature declSig(GetModule(), szName, pSig, cbSig, pDeclSubst); | ||
| MethodSignature declSig(GetModule(), szName, pSig, cbSig, NULL); |
There was a problem hiding this comment.
It looks like the pDeclSubst is not used anywhere after this change.
| // If the declaration method is a part of an interface, search through | ||
| // the interface map to find the matching interface so we can provide | ||
| // the correct substitution chain. | ||
| pDeclType = NULL; |
There was a problem hiding this comment.
A nit - this was set to NULL on the previous line, maybe just move the variable declaration here?
| declSig.GetSignature(), | ||
| static_cast<DWORD>(declSig.GetSignatureLength()), | ||
| declSig.GetModule(), | ||
| NULL, // Do not use the substitution of declSig, as we have adjusted the pDeclTypeSubstituion such that it must not be used. |
There was a problem hiding this comment.
A nit
| NULL, // Do not use the substitution of declSig, as we have adjusted the pDeclTypeSubstituion such that it must not be used. | |
| NULL, // Do not use the substitution of declSig, as we have adjusted the pDeclTypeSubstitution such that it must not be used. |
ghost
locked as resolved and limited conversation to collaborators
4 participants
environment that matches the parent type in the MemberRef
This change allows specifying an unambiguous target when there was
previously potential for ambiguity in the presence of generics and
MethodImpl records.