Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Assume conventionally named unresolved method references are properties or events #2677

Merged
merged 4 commits into from
May 20, 2022

Conversation

fowl2
Copy link
Contributor

@fowl2 fowl2 commented Apr 26, 2022

eg. decompile to .Property instead of .get_Property()

Problem

Two assemblies:

using System;

namespace UnknownNamespace
{
    public class UnknownClass<T>
    {
        public T UnknownProperty { get; set; }
        public event EventHandler<T> OnEvent;

        public T this[T index]
        {
            get => default;
            set { }
        }

        public T this[T x, T y]
        {
            get => default;
            set { }
        }

    }
}

And

using System;

namespace ClassLibrary1
{
    public class Class1 : EventArgs
    {
        public void Method()
        {
            var instance = new UnknownNamespace.UnknownClass<Class1>();

            var valueOfProp = instance.UnknownProperty;
            instance.UnknownProperty = valueOfProp;

            instance.OnEvent += Instance_OnEvent;
            instance.OnEvent -= Instance_OnEvent;
            
            var valueOfIndexer = instance[this];
            instance[this] = valueOfIndexer;

            var valueOfMultiIndexer = instance[this, this];
            instance[this, this] = valueOfMultiIndexer;
        }

        private void Instance_OnEvent(object sender, EventArgs e)
        {
            throw new NotImplementedException();
        }
    }
}

If the first is unresolvable/missing, the second can't desugar properties or event handlers and decompiles to:

public void Method()
{
	UnknownClass<Class1> instance = new UnknownClass<Class1>();
	Class1 valueOfProp = instance.get_UnknownProperty();
	instance.set_UnknownProperty(valueOfProp);
	instance.add_OnEvent((EventHandler<Class1>)Instance_OnEvent);
	instance.remove_OnEvent((EventHandler<Class1>)Instance_OnEvent);
	Class1 valueOfIndexer = instance.get_Item(this);
	instance.set_Item(this, valueOfIndexer);
	Class1 valueOfMultiIndexer = instance.get_Item(this, this);
	instance.set_Item(this, this, valueOfMultiIndexer);
}

Solution

While technically we can't be 100% sure, it's a pretty safe bet to assume calls to get_, add_, etc. are properties and event handers.

With this PR, the method decompiles to this:

public void Method()
{
	UnknownClass<Class1> instance = new UnknownClass<Class1>();
	Class1 valueOfProp = (instance.UnknownProperty = instance.UnknownProperty);
	instance.OnEvent += Instance_OnEvent;
	instance.OnEvent -= Instance_OnEvent;
	Class1 valueOfIndexer = (instance[this] = instance[this]);
	Class1 valueOfMultiIndexer = (instance[this, this] = instance[this, this]);
}
  • Any comments on the approach taken, its consistency with surrounding code, etc.
  1. This creates a new FakeProperty or FakeEvent for each method reference, which means that eg. getters and setter won't be associated with each other. It's consistent in that UsedBy still won't work, I guess.

  2. I'm not sure how raise_ (.fire) works, but I implemented it anyway to fit the pattern.

  3. I don't think it needs a configuration to disable/enable, as the regular desugaring option should work.

  • Which part of this PR is most in need of attention/improvement?
  • At least one test covering the code changed

@dgrunwald
Copy link
Member

For tests, I think ILPrettyTests would be the best fit -- with an .il input file, there's no need for the referenced assemblies to exist.
e.g. see the existing UnknownTypes.il test

@fowl2
Copy link
Contributor Author

fowl2 commented Apr 27, 2022

resolved review comment and added a test :)

@dgrunwald dgrunwald merged commit 672cff6 into icsharpcode:master May 20, 2022
@fowl2 fowl2 deleted the GuessFakeMethodAccessor branch May 25, 2022 02:58
siegfriedpammer added a commit that referenced this pull request Jun 13, 2022
… detect set-accessors of FakeProperties.

Bug likely introduced due to an oversight in #2677.
clin1234 pushed a commit to clin1234/ILSpy that referenced this pull request Jun 26, 2022
…to properly detect set-accessors of FakeProperties.

Bug likely introduced due to an oversight in icsharpcode#2677.
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants