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

Use .interfaceimpl type syntax #2903

Merged
merged 3 commits into from
Mar 2, 2023

Conversation

ltrzesniewski
Copy link
Contributor

@ltrzesniewski ltrzesniewski commented Feb 19, 2023

Problem

I noticed the following output when decompiling an assembly:

image

That implements .custom part seemed wrong, and indeed, ildasm uses another syntax instead: .interfaceimpl type (see here).

Here is the result:

image

Solution

  • Any comments on the approach taken, its consistency with surrounding code, etc.

    This PR moves the custom attributes on interface implementations inside the type body and adds one .interfaceimpl type declaration for each interface implementation which has a custom attribute.

  • Which part of this PR is most in need of attention/improvement?

    While implementing this, I noticed that ilasm is inconsistent with ildasm when multiple attributes are applied to an interface implementation. I believe ilasm is wrong here, and reported the following issue:
    ilasm does not apply multiple custom attributes on interface implementations dotnet/runtime#82373

    Because of this, the unit test only applies a single attribute on the interface implementation. I didn't want to add an additional .expected.il file just because of this inconsistency.

    This PR implements the same behavior as in ildasm.

    As for the UI, I wasn't sure if I should highlight the type keyword in green (which looked wrong) or in blue (which could lead to many false positives), so I left it as-is.

  • At least one test covering the code changed

@ltrzesniewski
Copy link
Contributor Author

ltrzesniewski commented Feb 19, 2023

BTW this is how dotPeek displays an interface implementation with two custom attributes:

image

Note the indentation, which clarifies the intent.

@siegfriedpammer
Copy link
Member

  • As for the UI, I wasn't sure if I should highlight the type keyword in green (which looked wrong) or in blue (which could lead to many false positives), so I left it as-is.

As ilasm treats type as a keyword and fails to assemble a method that is named type: error : syntax error at token 'type'. I would say that you can safely highlight it, there would be no confusion in existing code.

@ltrzesniewski
Copy link
Contributor Author

Right, I should have tested it. I added the type keyword.

But upon checking, I noticed that single-quoted identifiers are still highlighted as kewords. This also applies to other keywords such as instance:

image

So I also fixed that in this PR:

image

@dgrunwald dgrunwald merged commit 52bab53 into icsharpcode:master Mar 2, 2023
@ltrzesniewski ltrzesniewski deleted the interfaceimpl branch March 2, 2023 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants