fix #17447 -MethodAccessException on equality comparison of a record with private #17449
Conversation
❗ Release notes required
|
| @@ -8360,7 +8360,9 @@ and ComputeMethodAccessRestrictedBySig eenv vspec = | |||
| else | |||
| vspec | |||
|
|
|||
| let isHiddenBySignatureVal = IsHiddenVal eenv.sigToImplRemapInfo vspec | |||
| let isHiddenBySignatureVal = | |||
There was a problem hiding this comment.
This might intervene with the generated .Is* members properties which are automatically added for DUs.
I am not sure the problem is real, but the combination of your change and the way I remember the .Is* feature rings a bell for me.
There was a problem hiding this comment.
@T-Gro , yes that popped into my mind while working on this too ... however, the fix for that was to make the compilergenerated Is* methods visible to the TypeChecker. Here the fix is to ensure that CompilerGenerated methods are not forced to be emitted internal but instead are emitted with the visibility specified when generated for generated methods without a sigfile definiton.
Equals for these specified type generated equals is different from other generated Equals methods, because they are overrides, where this is just a new method on the type. There is a code path that forces the overriden Equals to public, and then the compiler changes it to the visibility of the overriden method definition.
There was a problem hiding this comment.
@T-Gro , over the weekend I came up with a much better more targeted fix.
| @@ -77,7 +77,7 @@ extends [runtime]System.Object | |||
|
|
|||
| } | |||
|
|
|||
| .method assembly specialname static class [runtime]System.Tuple`2<bool,int32> get_patternInput@9() cil managed | |||
| .method public specialname static class [runtime]System.Tuple`2<bool,int32> get_patternInput@9() cil managed | |||
There was a problem hiding this comment.
@KevinRansom so these guys became public now... I was pondering on this - I don't really have enough picture to evaluate the real world implications since this is quite a special case, but what I feel is that this is either okay or uncovering a different issue which should be addressed separately.
There was a problem hiding this comment.
@psfinaki public is the expected scope, it is not hard to argue that it is incorrect, but the bug is certainly elsewhere.
module OutOptionalTests
// unnecessary code elided here
let (_:bool), (_:int) = Thing.Do(i = 42)
let (_:bool), (_:int) = Thing.Do()
So:
1: bindings on modules default to being public ...
2: The binding is unnamed discards, so the side effect is still necessary.
if you make the bindings look like:
let (_a:bool), (_b:int) = Thing.Do(i = 42)
let (_c:bool), (_d:int) = Thing.Do()
Then compile with --allsigs you get this output:
module OutOptionalTests
// unnecessary code elided here
val _b: int
val _a: bool
val _d: int
val _c: bool
So give them a name and the value is emitted. So ... if it is expected to not be public ... I think it was relying on the side-effect of a bug. Raise an issue suggesting that unnamed bindings should not be public when emitted to il.
And require that it's fixed before we merge this one. I will look at it. It is better as a separate issue, because they are entirely unrelated.
There was a problem hiding this comment.
@psfinaki - ignore the above over the weekend I revisited this and decided that my earlier approach was a bit too much of a hack. I replaced it with a better more targetted fix.
KevinRansom
dismissed
psfinaki’s stale review
I changed the fix. The new one is much more targeted
|
This was entirely the wrong approach |
Fix #17447 -MethodAccessException on equality comparison of a record with private
There is an issue with code generation when using realsig- (The legacy visibility mechanism) on compiler generated methods that are not overrides. The compiler generates them internal, even though they are created with a visibility of public.
This is a better fix:
Change the codegenerator to generate augments for equals as Final, Overrides, this causes them to go through the same code path as the other code generated Equals operations, this has way fewer side effects and generates the right members.