Setter for PropertyDefinition.SetMethod and PropertyDefinition.GetMethod

[oSumAtrIX]

Problem Description

AsmResolver is currently lacking the ability to set the properties SetMethod and GetMethod of a PropertyDefinition.

Proposal

Implement a setter for the properties SetMethod and GetMethod of a PropertyDefinition in order to be able to set get and set accessors of properties.

Alternatives

A property can be replaced by its get and set accessors and a corresponding field to host the value.

Additional context

Missing setter for PropertyDefinition.GetMethod in AsmResolver (current state):

Existing setter for PropertyDef.GetMethod in Dnlib (feature request):

[Washi1337]

Note: property and event methods can already be added and/or modified through the Semantics property.

If we were to add this setter to these kinds of properties, what would happen when the property e.g. has multiple getters defined? (This is invalid metadata but technically possible)

[oSumAtrIX]

If a getter or setter method is present, the method marked as getter or setter should be overwritten.
If no getter or setter method definition is present in the method semantics, the new method definition would be added to the semantics and marked as getter or setter.
If multiple getters or setters are possible, GetMethod and SetMethod properties should be changed to a collection.

[Washi1337]

Changing GetMethod and others to a collection seems very unintuitive for the end-user, considering that valid metadata will at most have one get method for every property. It also introduces more syntax noise, as it would require using square brackets and an index for accessing a get method. I think for such a rare occurrence it is not worth the extra steps.

Perhaps we should change the description of GetMethod to only get or set the first occurrence of a get method? We should maybe then consider the exception of writing null, which I think would make more sense if it would remove all of them rather than just the first one.

[oSumAtrIX]

While your proposal is a solution, the problem is not worth a hacky one like this. I think people would get confused on why specifically getter/setter x is being treated the way it is (even if its the first occurence).
They would encounter a logical problem caused through code design:

1. Try, if using the property actually affects the method they have in question
2. Based on the first occurence, need to switch to the semantics property, which would make the GetMethod or SetMethod kinda useless at the first point.

Changing the properties to collections is also very unintuitive as you said as well. The properties are not called GetMethods or SetMethods for a reason.

I would leave this issue open for further proposals.

[ds5678]

I think these should stay as getter-only properties. They only return the first get/set method, and changing this would be a silent breaking change. Taking that into account, a setter would be unintuitive.

However, I have a proposal:

//needs a better name
public void ApplyGetterAndSetter(MethodDefinition? getMethod, MethodDefinition? setMethod)
{
    //Check that declaring types are not null and the same?
    Semantics.Clear();
    if(getMethod is not null)
        Semantics.Add(new MethodSemantics(getMethod, MethodSemanticsAttributes.Getter));
    if(setMethod is not null)
        Semantics.Add(new MethodSemantics(setMethod, MethodSemanticsAttributes.Setter));
}

[Washi1337]

Side note: If we're going for something like this, then for consistency I think we should also add the equivalent for the add/remove/fire methods of an EventDefinition.