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

Operator as should not be used with non-nullable value type. #1903

Closed
yyjdelete opened this issue Jan 9, 2020 · 3 comments
Closed

Operator as should not be used with non-nullable value type. #1903

yyjdelete opened this issue Jan 9, 2020 · 3 comments
Labels
Bug C# Decompiler The decompiler engine itself

Comments

@yyjdelete
Copy link
Contributor

Tested with master + roslyn 3.4.0

Seems roslyn 3.1.0 start to use an new mode and can not be proper translated by ILSpy.

See also
https://sharplab.io/#v2:D4AQTAjAsAULIGYAE4kGEkG9ZNy5IEAbEgJYB2ALkgLIQA8AKgHwAUjSlAlFjnvyADsSVtVIBnMlTI8A/GSQAuJAFoIAbj64AvlvwpiU6jQRM2AewBGAKwCmAY2rdeMfgOGiykimLkLlapqueLow2kA=

Orig Code

M3 works well, and it is just to be used to compare with M1

    public static int M1<T>(T t) {
        return (t is int i) ? i : -1;
    }
    public static int M3<T>(object t) {//This one works, just use it to compare with M1
        return (t is int i) ? i : -1;
    }

IL

    .method public hidebysig static 
        int32 M1<T> (
            !!T t
        ) cil managed 
    {
        // Method begins at RVA 0x2050
        // Code size 36 (0x24)
        .maxstack 1
        .locals init (
            [0] int32
        )

        IL_0000: ldarg.0
        IL_0001: box !!T
        IL_0006: isinst [System.Runtime]System.Int32
        IL_000b: brfalse.s IL_0020

        IL_000d: ldarg.0
        IL_000e: box !!T
        IL_0013: isinst [System.Runtime]System.Int32
        IL_0018: unbox.any [System.Runtime]System.Int32
        IL_001d: stloc.0
        IL_001e: br.s IL_0022

        IL_0020: ldc.i4.m1
        IL_0021: ret

        IL_0022: ldloc.0
        IL_0023: ret
    } // end of method C::M1

//This one works, just use it to compare with M1
    .method public hidebysig static 
        int32 M3<T> (
            object t
        ) cil managed 
    {
        // Method begins at RVA 0x2080
        // Code size 21 (0x15)
        .maxstack 1
        .locals init (
            [0] int32
        )

        IL_0000: ldarg.0
        IL_0001: isinst [System.Runtime]System.Int32
        IL_0006: brfalse.s IL_0011

        IL_0008: ldarg.0
        IL_0009: unbox.any [System.Runtime]System.Int32
        IL_000e: stloc.0
        IL_000f: br.s IL_0013

        IL_0011: ldc.i4.m1
        IL_0012: ret

        IL_0013: ldloc.0
        IL_0014: ret
    } // end of method C::M3

Expected

//Will get the below code if you change typeof t from T to object

    public static int M1<T>(T t)
    {
        if (t is int)
        {
            return (int)t;
        }
        return -1;
    }

Actual

    public static int M1<T>(T t)
    {
        if (t is int)
        {
            return t as int;//<--Should not use `as` here.
        }
        return -1;
    }

Old Code from Roslyn 2.10

Decompile code

public static int M1<T>(T t)
{
	T val;
	if ((val = t) is int)
	{
		return (int)(object)val;
	}
	return -1;
}
.method public hidebysig static 
	int32 M1<T> (
		!!T t
	) cil managed 
{
	// Method begins at RVA 0x2050
	// Code size 33 (0x21)
	.maxstack 2
	.locals init (
		[0] int32 i,
		[1] !!T
	)

	IL_0000: ldarg.0
	IL_0001: dup
	IL_0002: stloc.1
	IL_0003: box !!T
	IL_0008: isinst [System.Runtime]System.Int32
	IL_000d: brfalse.s IL_001d

	IL_000f: ldloc.1
	IL_0010: box !!T
	IL_0015: unbox.any [System.Runtime]System.Int32
	IL_001a: stloc.0
	IL_001b: br.s IL_001f

	IL_001d: ldc.i4.m1
	IL_001e: ret

	IL_001f: ldloc.0
	IL_0020: ret
} // end of method Class1::M1
@yyjdelete yyjdelete changed the title Operator as should be used with non-nullable value type. Operator as should not be used with non-nullable value type. Jan 9, 2020
@siegfriedpammer siegfriedpammer added Bug C# Decompiler The decompiler engine itself labels Feb 26, 2020
@dgrunwald
Copy link
Member

The new instruction sequence:

isinst [System.Runtime]System.Int32
unbox.any [System.Runtime]System.Int32

is somewhat tricky to translate in isolation:
If the input object is not an Int32, it will throw NullReferenceException, whereas a (int)obj cast will throw InvalidCastException.

What's happening here is that this example gets handled by a special-case that was intended for nullable as int? or generic as T (where T:class) casts, which also emit this isinst+unbox.any code pattern.

@dgrunwald
Copy link
Member

If I fix the as T? special-case to not trigger in this case, we end up with:

public static int M1<T>(T t)
{
	if (t is int)
	{
		return (int)(object)/*isinst with value type is only supported in some contexts*/;
	}
	return -1;
}

which is not exactly an improvement.

Not sure what we can do here without implementing full pattern matching support (#2048).
Maybe emit something like: return (int)(t is int ? (object)t : null)?

@dgrunwald
Copy link
Member

New decompiled code:

	public static int M1<T>(T t)
	{
		if (t is int)
		{
			object obj = t;
			return (int)((obj is int) ? obj : null);
		}
		return -1;
	}

We now split the statement involving isinst into multiple statements as necessary to ensure that we end up with a supported context for the isinst.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 10, 2020
ElektroKill added a commit to dnSpyEx/ILSpy that referenced this issue Aug 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug C# Decompiler The decompiler engine itself
Projects
None yet
Development

No branches or pull requests

3 participants