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

Reflection regression - Type.GetType does not skip leading . #84644

Closed
AndriySvyryd opened this issue Apr 11, 2023 · 17 comments · Fixed by #84957
Closed

Reflection regression - Type.GetType does not skip leading . #84644

AndriySvyryd opened this issue Apr 11, 2023 · 17 comments · Fixed by #84957
Assignees
Labels
area-System.Reflection os-mac-os-x macOS aka OSX untriaged New issue has not been triaged by the area owner

Comments

@AndriySvyryd
Copy link
Member

AndriySvyryd commented Apr 11, 2023

Description

Type.GetType does not find the type when type name starts with . (dot)..

Reproduction Steps

class MyType
{
    static void Main() => Console.WriteLine(Type.GetType(".MyType"));
}

Expected behavior

Type is found

Actual behavior

Type is not found

Regression?

Yes, this used to work on SDK 8.0.100-preview.4.23171.22

Known Workarounds

No response

Configuration

SDK 8.0.100-preview.4.23210.1
macOS-11.6.7
x64

Other information

No response

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Apr 11, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Apr 11, 2023
@vcsjones vcsjones added area-System.Reflection and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Apr 11, 2023
@ghost
Copy link

ghost commented Apr 11, 2023

Tagging subscribers to this area: @dotnet/area-system-reflection
See info in area-owners.md if you want to be subscribed.

Issue Details

Description

GetProperty returns null for a static property on a class in the global namespace, only on OSX.

Reproduction Steps

Run the attached repro

Expected behavior

No exceptions thrown

Actual behavior

System.NullReferenceException : Object reference not set to an instance of an object.

Regression?

Yes, this used to work on SDK 8.0.100-preview.4.23171.22

Known Workarounds

No response

Configuration

SDK 8.0.100-preview.4.23210.1
macOS-11.6.7
x64

Other information

No response

Author: AndriySvyryd
Assignees: -
Labels:

area-System.Reflection, untriaged

Milestone: -

@AaronRobinsonMSFT
Copy link
Member

/cc @buyaa-n

@jkotas
Copy link
Member

jkotas commented Apr 12, 2023

@EgorBo This is very recent regression. My guess is that it was introduced by #83911.

public partial class GlobalNamespaceContextModel : RuntimeModel
{
    static GlobalNamespaceContextModel()
    {
        var model = new GlobalNamespaceContextModel();
        ...
        _instance = model;
    }

    private static GlobalNamespaceContextModel _instance;
    public static IModel Instance => _instance; <- This returns null when invoked by reflection.
}

@EgorBo EgorBo self-assigned this Apr 12, 2023
@EgorBo
Copy link
Member

EgorBo commented Apr 12, 2023

hm.. I can't repro no matter what I do - I tried the exact SDK version + played with R2R/TC modes - it still prints "Success!" on run for me

macOS-x64 13.2.1

@EgorBo
Copy link
Member

EgorBo commented Apr 12, 2023

@AndriySvyryd any extra steps I need to do, does it repro via dotnet run or in VSfM?

@EgorBo
Copy link
Member

EgorBo commented Apr 12, 2023

Also tried to run it in a loop (via bash) and with various stress modes - still no luck reproing

@buyaa-n buyaa-n added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI and removed area-System.Reflection labels Apr 12, 2023
@ghost
Copy link

ghost commented Apr 12, 2023

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch, @kunalspathak
See info in area-owners.md if you want to be subscribed.

Issue Details

Description

GetProperty returns null for a static property on a class in the global namespace, only on OSX.

Reproduction Steps

Run the attached repro

Expected behavior

No exceptions thrown

Actual behavior

System.NullReferenceException : Object reference not set to an instance of an object.

Regression?

Yes, this used to work on SDK 8.0.100-preview.4.23171.22

Known Workarounds

No response

Configuration

SDK 8.0.100-preview.4.23210.1
macOS-11.6.7
x64

Other information

No response

Author: AndriySvyryd
Assignees: EgorBo
Labels:

area-CodeGen-coreclr, untriaged

Milestone: -

@AndriySvyryd
Copy link
Member Author

AndriySvyryd commented Apr 12, 2023

I have only been able to repro this on Helix machines via dotnet run.

It doesn't repro on macOS-x64 11.7.5

Here's an updated repro that builds the target assembly at runtime.

@AndriySvyryd
Copy link
Member Author

We also set COMPlus_EnableWriteXorExecute=0 before running this (it's a workaround for a different issue we've hit)

@EgorBo
Copy link
Member

EgorBo commented Apr 12, 2023

Thanks! I was able to repro and investigating now

@EgorBo
Copy link
Member

EgorBo commented Apr 12, 2023

Doesn't look to be #83911 as it still reproduces on Main when I disable that phase. Also reproduces in Debug mode + R2R=0 + JitMinOpts=1 so highly unlikely a JIT issue, still looking

@AndriySvyryd
Copy link
Member Author

Updating Microsoft.CodeAnalysis.CSharp from 4.2.0 to 4.5.0 resolves this issue. So, it can be closed as far as we are concerned.

@EgorBo
Copy link
Member

EgorBo commented Apr 12, 2023

I was bisecting it on locally built runtime and the regression or whatever it was is between [20..25] of march, bisecting further

@EgorBo
Copy link
Member

EgorBo commented Apr 12, 2023

so far looks like #83484, I'll check before-after in a moment to make sure

@EgorBo
Copy link
Member

EgorBo commented Apr 12, 2023

Yes, it's #83484 cc @jkotas

Steps for https://github.com/dotnet/runtime/files/11214690/ReflectionOSX.zip

# I was doing this using 8.0.100-preview.4.23210.1 SDK 
dotnet publish -c Release -r osx-x64 

then I was just executing output binary using corerun from Checked folder

~/prj/runtime/artifacts/bin/coreclr/osx.x64.Checked/corerun \
bin/Release/net8.0/osx-x64/publish/ReflectionOSX.dll

@JulieLeeMSFT JulieLeeMSFT added os-mac-os-x macOS aka OSX area-System.Reflection and removed area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI labels Apr 13, 2023
@ghost
Copy link

ghost commented Apr 13, 2023

Tagging subscribers to this area: @dotnet/area-system-reflection
See info in area-owners.md if you want to be subscribed.

Issue Details

Description

GetProperty returns null for a static property on a class in the global namespace, only on OSX.

Reproduction Steps

Run the attached repro

Expected behavior

No exceptions thrown

Actual behavior

System.NullReferenceException : Object reference not set to an instance of an object.

Regression?

Yes, this used to work on SDK 8.0.100-preview.4.23171.22

Known Workarounds

No response

Configuration

SDK 8.0.100-preview.4.23210.1
macOS-11.6.7
x64

Other information

No response

Author: AndriySvyryd
Assignees: EgorBo
Labels:

area-System.Reflection, os-mac-os-x, area-CodeGen-coreclr, untriaged

Milestone: -

@JulieLeeMSFT JulieLeeMSFT assigned jkotas and unassigned EgorBo Apr 13, 2023
@jkotas jkotas changed the title Reflection regression on OSX Reflection regression - Type.GetType does not skip leading . Apr 15, 2023
@jkotas
Copy link
Member

jkotas commented Apr 15, 2023

@EgorBo Thank you for taking a look!

The problem is in Type.GetType API and it affects all platforms (it is not OSX specific). I have updated the description. assembly.GetType(".GlobalNamespaceContextModel") is the statement in the repro that triggered the issue.

The issue is that Type.GetType implementation had a quirk that made it ignore leading . in the type name. The new managed type name parser is missing this quirk.

jkotas added a commit to jkotas/runtime that referenced this issue Apr 18, 2023
Ignore leading '.' for global typenames for compatibility with earlier .NET versions.

Fixes dotnet#84644
jkotas added a commit that referenced this issue Apr 18, 2023
Ignore leading '.' for global typenames for compatibility with earlier .NET versions.

Fixes #84644
@ghost ghost locked as resolved and limited conversation to collaborators May 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Reflection os-mac-os-x macOS aka OSX untriaged New issue has not been triaged by the area owner
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants