cecil icon indicating copy to clipboard operation
cecil copied to clipboard

Simplify MetadataBuilder.GetConstantType.

Open teo-tsirpanis opened this issue 4 years ago • 4 comments

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.

teo-tsirpanis avatar Oct 29 '20 17:10 teo-tsirpanis

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.

jbevain avatar Oct 29 '20 23:10 jbevain

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;.

teo-tsirpanis avatar Oct 30 '20 00:10 teo-tsirpanis

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.

jbevain avatar Oct 30 '20 00:10 jbevain

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. :-)

teo-tsirpanis avatar Oct 30 '20 07:10 teo-tsirpanis