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

Improve enum #2763

Closed
CreateAndInject opened this issue Aug 15, 2022 · 5 comments
Closed

Improve enum #2763

CreateAndInject opened this issue Aug 15, 2022 · 5 comments
Labels
Bug Decompiler The decompiler engine itself

Comments

@CreateAndInject
Copy link
Contributor

Original :

public static void Test(int value)
{
    var color = (KnownColor)value;
    switch (color)
    {
        case KnownColor.DarkBlue:
            Console.WriteLine();
            break;
        case KnownColor.DarkCyan:
            Console.WriteLine();
            break;
        case KnownColor.DarkGoldenrod:
            Console.WriteLine();
            break;
        case KnownColor.DarkGray:
            Console.WriteLine();
            break;
        case KnownColor.DarkGreen:
            Console.WriteLine();
            break;
        case KnownColor.DarkKhaki:
            Console.WriteLine();
            break;
    }
}

ILSpy :

public static void Test(int value)
{
	switch (value)
	{
	case 49:
		Console.WriteLine();
		break;
	case 50:
		Console.WriteLine();
		break;
	case 51:
		Console.WriteLine();
		break;
	case 52:
		Console.WriteLine();
		break;
	case 53:
		Console.WriteLine();
		break;
	case 54:
		Console.WriteLine();
		break;
	}
}
@CreateAndInject CreateAndInject added Bug Decompiler The decompiler engine itself labels Aug 15, 2022
@mpMelnikov
Copy link

mpMelnikov commented Sep 14, 2022

Hi @siegfriedpammer,

I have investigated the issue. The replacement of enum with int happens somewhere inside StatementTransform. I can catch the moment when this replacement happens. In this stack trace, there is such a comment:

// TODO: inlining of small integer types might be semantically incorrect,
// but we can't avoid it this easily without breaking lots of tests.

Can it be related? Does it make sense to try to fix it or does it look like a potentially major change?

@dgrunwald
Copy link
Member

That's unrelated, with "small integer types" that comment means types smaller than int which are implicitly extended to I4 as they are loaded onto the IL evaluation stack.

mpMelnikov pushed a commit to mpMelnikov/ILSpy that referenced this issue Sep 15, 2022
Enum in switch was replaced with the int incorrectly in some cases.
@mpMelnikov
Copy link

mpMelnikov commented Sep 15, 2022

@dgrunwald
I have created a pull request. It is my first attempt to contribute to ILSpy, so I understand that the solution can be totally incorrect. At least it gives some insight where the issue is, and I hope it can be a starting point.

Also, I found another bug.
This method

    public static void EnumReplacedWithIntIncorrectly_AndIncorrectConditionGenerated_AndUnusedVariables(int intValue)
    {
        var color = (KnownColor)intValue;
        var color2 = (KnownColor)intValue;
        var color3 = (KnownColor)intValue;
        if (color == KnownColor.DarkBlue)
        {

        }

        if (color2 == KnownColor.DarkBlue)
        {

        }
    }

is generated like this:

	public static void EnumReplacedWithIntIncorrectly_AndIncorrectConditionGenerated_AndUnusedVariables(int intValue)
	{
		KnownColor color = (KnownColor)intValue;
		KnownColor color2 = (KnownColor)intValue;
		if (color == KnownColor.DarkBlue)
		{
		}
		if (color2 != KnownColor.DarkBlue)// '!=' instead of '==' here
		{
		}
	}

Do you remember if it was already registered?

One more question. I tried to use pre-commit script as required, but I constantly get an error even though no spaces were introduced (only tabs). Is pre-commit script supported and working correctly now?

@siegfriedpammer
Copy link
Member

// '!=' instead of '==' here

Empty blocks cannot be distinguished, so the code is effectively semantically equivalent, and the conditions entirely depend on the generated IL code.

If you add statements to the bodies, the code is decompiled exactly as expected:

https://sharplab.io/#v2:EYLgtghglgdgNAExAagD4AEBMBGAsAKAIFMYBXMAAgGkYB7AdxgGFaAbWgJwoG8KARCBwDWAIVakicfoKEBxDkRJSAmkVbt6UgEpEEU+YvgUxEigF8CBdAGYKWCkx4EKLu7fTYAbHYAsFAKJkYDoADqwQAMa6AOpQAC4AFgCSMHEpEZwKEXGsAJ4A+gCCMAjpmUTZLCXxULQwsiREHBBxukUlAKpkAM66AGqCUBDArETdABSwcRRTA+JEAJTOrtzLrq4AboIUGexcALwU4zQMzGycC7MQ8wDca+sUW1y7nJgUh8d0jCx7l6lzEju+Aem22Lw4tg+J2+5w4fziAKIQJBLigADMjuD3odoWc9gA6ATCEyLe7rVbAlHrDwATnGACIiaJ5vSFsiURZCJSQejMbC3vscV88ZxCTISUtuQ8KVTqdg6YzxSy2ZYpetOeqCGYgA=

@siegfriedpammer
Copy link
Member

One more question. I tried to use pre-commit script as required, but I constantly get an error even though no spaces were introduced (only tabs). Is pre-commit script supported and working correctly now?

Which version of dotnet-format are you using? Our experience is that it is quite fragile and will break easily, that's why we use one specific version, as can be seen here: https://github.com/icsharpcode/ILSpy/blob/master/.github/workflows/build-ilspy.yml#L33

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug Decompiler The decompiler engine itself
Projects
None yet
Development

No branches or pull requests

4 participants