ltrzesniewski / InlineIL.Fody

Inject arbitrary IL code at compile time.
MIT License
240 stars 17 forks source link

[Proposal] Referencing user-defined operators #22

Closed sakno closed 3 years ago

sakno commented 3 years ago

MethodRef contains brilliant static methods for referencing property getter/setter, constructor etc. But there is no way to reference user-defined operators, such as op_Implicit. Could you please add methods for that?

sakno commented 3 years ago

And yes, I understand that it may be difficult because op_Implicit overloaded methods distinguishable only by their return types in some cases.

ltrzesniewski commented 3 years ago

You're right: there's currently no way to reference a given op_Implicit or op_Explicit if there are overloads that differ only by the return type, so a special method on MethodRef is needed for those.

The other operators can currently be accessed just like normal methods (new MethodRef(typeof(...), "op_Addition") for instance), but having a helper method for those would be nice.

sakno commented 3 years ago

One more thing, different .NET languages may have unique operators. For instance, Visual Basic has op_Exponent operator which is not supported by C#.

ltrzesniewski commented 3 years ago

Ugh. Thanks for telling me this. So VB uses op_Exponent, F# uses op_Exponentiation, and none of them are mentioned in ECMA-335 of course. 😄

It's a mess, so I probably won't add specific support for operators that are not mentioned in ECMA-335. You'll still be able to access them like a normal method though.

sakno commented 3 years ago

Sure, I'm OK with that.

ltrzesniewski commented 3 years ago

I'm currently thinking about adding the following methods to MethodRef: https://github.com/ltrzesniewski/InlineIL.Fody/commit/5320178c8617463b94973be82e035ef400c74985#diff-570e2235451ba891fef856dd55beaccc

There's one thing I don't really like in this API though: it forces you to provide both operand types for binary operators, but at least one of them will necessarily be equal to the declaring type. On the other hand, this signature makes it clear which overload will be picked, so maybe that's not really an issue.

sakno commented 3 years ago

IMHO, it's better to have separated methods for each operator. It's redundant for you as an author of this library, but pretty clear for users. However, it's fine if you force consumer to specify operand types explicitly. Let''s examine the following correct code:

public class C {
    public static int operator +(int x, C y) => 0;
    public static int operator +(C y, int x) => 1;
}

As you can see, the operator differs in the order of operands. It's not possible to resolve this situation without explicit definition of operand types.

sakno commented 3 years ago

In case of op_Implicit and op_Explicit it's not needed to specify conversion direction. You can explore this automatically by searching for both cases.

sakno commented 3 years ago

If, nevertheless, a decision is made to leave enums for operator you have to take into account that enum value can be created incorrectly from semantics point of view, but correctly from syntactic point of view. e.g. (BinaryOperator)42

sakno commented 3 years ago

Additionally, it would be nice if you'll add generic versions of these methods.

ltrzesniewski commented 3 years ago

IMHO, it's better to have separated methods for each operator. It's redundant for you as an author of this library, but pretty clear for users.

These are small helper methods that I don't expect to be used that much, so I prefer having a smaller API surface. It's much easier to implement and maintain that way.

As you can see, the operator differs in the order of operands. It's not possible to resolve this situation without explicit definition of operand types.

Yes, that's the reason why I chose to have the 3 types on the binary operator method. I guess it's OK to leave it as-is.

In case of op_Implicit and op_Explicit it's not needed to specify conversion direction.

It's necessary to disambiguate the operators in the following case:

public class C {
    public static implicit operator int(C c) => default;
    public static implicit operator C(int i) => default;
}

you have to take into account that enum value can be created incorrectly from semantics point of view

Yes, I thought about that, I intend to validate the parameter value.

Additionally, it would be nice if you'll add generic versions of these methods.

Hmm I don't think it would fit very well into the API, but I'll think about it.

ltrzesniewski commented 3 years ago

Ok, I've implemented this but haven't released a new version yet, because I'm not sure if I should add an additional overload to the constructor and Method method of MethodRef which would let the user resolve a given op_Implicit/op_Explicit method by return type.

On the one hand, I'd like having a general-purpose method which can access everything, but on the other hand, ECMA-335 permits overloading by return type only for op_Implicit/op_Explicit, so I'm not sure if adding an overload with a weird signature just because of this would be a good idea. MethodRef.Operator handles that.

The signature would need to be something like:

public MethodRef(TypeRef type, string methodName, TypeRef returnType, int genericParameterCount, params TypeRef[] parameterTypes)

But this just doesn't feel like a good API to me because it could be confusing given the other overloads. I think I'll just end up releasing v1.6.0 as-is. What do you think?

sakno commented 3 years ago

@ltrzesniewski , general-purpose overload is good idea because the two methods may have the same signature and even the same return type but differs in modreq modifiers. From ECMA-335 point of view, this is the valid overload.

ltrzesniewski commented 3 years ago

This is now available in v1.6.0.