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

Simplify MetadataBuilder.GetConstantType. #702

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

teo-tsirpanis
Copy link
Contributor

@teo-tsirpanis teo-tsirpanis commented Oct 29, 2020

The value of the "Type" column in the constant table is now determined from the object's type; not the field or parameter's declared type, avoiding calling the resolver when the constant is an enum.

Type.GetTypeCode() has been verified to return the underlying type of enum types: Type.GetTypeCode(typeof(DayOfWeek)) returns TypeCode.Int32 for example.

Fixes #886.

The value of the "Type" column in the constant table is now determined from the object's type; not the field or parameter's declared type.
@jbevain
Copy link
Owner

jbevain commented Oct 29, 2020

Thanks for the PR.

When Cecil reads an enum constant, it's not going to instantiate an enum, meaning the boxed value will be of the enum's underlying type.

So something might be off in the case where the field's type is an enum.

@teo-tsirpanis
Copy link
Contributor Author

When Cecil reads an enum constant, it's not going to instantiate an enum, meaning the boxed value will be of the enum's underlying type.

So something might be off in the case where the field's type is an enum.

This is not a problem at all. Try running the following code:

using System;
public static class C {
    public static void Main() {
        object x = 3;
        Console.WriteLine((DayOfWeek) x);
    }
}

It prints Wednesday. The CLR seems to handle enum<->integer conversions pretty transparently, even in boxed values.

Besides, your concerns are towards Cecil's constant reader, while this PR eliminates the use of the assembly resolver from the constant writer.

The type of a constant is declared twice. Once, at the field's declaration as a type metadata token, and once again at the Constant metadata table, as a simple, 1-byte long enumeration, specifying the constant's underlying type. The former is set outside MetadataBuilder.AddConstant without the need to resolve any assembly, and the latter does not care whether we store an int32 or an enum backed by an int32 and can be derived by directly looking at the boxed value's TypeCode.

Mismatches between the two constant types can happen and it's the developer's responsibility to avoid them. Otherwise we have cases that dnSpy displays as public const string Foo = 59;.

@jbevain
Copy link
Owner

jbevain commented Oct 30, 2020

Lol, thanks for the C# lesson I didn't ask for.

What I'm saying above is that this is changing how a piece of metadata read from an assembly is going to be written back.

enum Letter { A, B }

class Foo {
    public const Letter FirstLetter = L.A;
}

If the constant record is serialized as an enum in the original assembly, writing it with Cecil will write a constant record using the underlying type of the enum.

It doesn't mean that it doesn't work. Cecil doesn't do a perfect job at writing back what it read, but it's trying.

It also doesn't mean that it's not an acceptable trade-off.

@teo-tsirpanis
Copy link
Contributor Author

I am glad that the PR is under consideration but I still can't understand the explanation of your trade-off.

[...] the constant record is serialized as an enum [...]

The values of a constant record are serialized as primitive values, not enums. This is the generated IL of your above snippet:

image

The valuetype Letter underlined in blue is determined from FieldReference.FieldType, not the constant's type or internal representation.

It doesn't mean that it doesn't work. Cecil doesn't do a perfect job at writing back what it read, but it's trying.

It also doesn't mean that it's not an acceptable trade-off.

Again, I am glad that the PR is under consideration. :-)

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.

Constant serialization
2 participants