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

RefEmit SymbolMethod should probably have a Metadata token override #53300

Closed
KevinRansom opened this issue May 26, 2021 · 3 comments · Fixed by #54656
Closed

RefEmit SymbolMethod should probably have a Metadata token override #53300

KevinRansom opened this issue May 26, 2021 · 3 comments · Fixed by #54656

Comments

@KevinRansom
Copy link
Member

KevinRansom commented May 26, 2021

The recent PR to add MedataToken getter overrides to builder classes has broken FSI code generation on dotnet 6.0: Add MetadataToken getter override to builder classes (#43330) · b72b13e (github.com)

The method:

private int GetMethodTokenNoLock(MethodInfo method, bool getGenericTypeDefinition)

was modified here: runtime/src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/ModuleBuilder.cs at 01b7e73 · dotnet/runtime (github.com)

This working code:

                if (symMethod.GetModule() == this)
1231
                    return symMethod.GetToken();
1232

Became this failing code:

                if (symMethod.GetModule() == this)
                    return symMethod.MetadataToken;

symMethod is an instance of System.Reflection.Emit.SymbolMethod: here runtime/SymbolMethod.cs at main · dotnet/runtime (github.com)

Which does not provide an override for the MetadataToken property, thus it relies on MemberInfos base implementation which throws a System.InvalidOperationException: 'Operation is not valid due to the current state of the object.'

I assume, someone should either revert the change back to return symMethod.GetToken(); … or provide an overload for MetadataToken on System.Reflection.Emit.SymbolMethod.

Repro:
On a machine with a Net6.0 dotnet sdk installed:

In a command window type:

dotnet fsi

let arr = Array2D.create 3 4 0
let _ = Array2D.get arr 0 0
;;

observe the output:

error FS0193: internal error: Operation is not valid due to the current state of the object.

You may see a bunch of warnings on fsi startup like this, this is an unrelated bug, a fix is in the pipeline:

unknown(1,1): warning FS3384: The .NET SDK for this script could not be determined. If the script is in a directory using a 'global.json' then ensure the relevant .NET SDK is installed. Unexpected error 'Could not find a part of the path 'C:\Program Files\dotnet\packs\Microsoft.NETCore.App.Ref\0.0.0.0\ref'.'.
@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Reflection untriaged New issue has not been triaged by the area owner labels May 26, 2021
@jkotas jkotas added this to the 6.0.0 milestone May 27, 2021
@buyaa-n buyaa-n removed the untriaged New issue has not been triaged by the area owner label May 27, 2021
@KevinRansom
Copy link
Member Author

@jkotas , has any progress been made on this?

/cc @vzarytovskii

@jkotas
Copy link
Member

jkotas commented Jun 22, 2021

I assume @dotnet/area-system-reflection crew will take care of this before .NET 6 ships. If you would like to accelerate it, you can submit a PR with the fix.

@KevinRansom
Copy link
Member Author

@jkotas , sounds like a plan, thanks mate.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jun 24, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jun 26, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jul 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

3 participants