Skip to content

Conversation

@Yozer
Copy link
Member

@Yozer Yozer commented Jan 28, 2022

Fixes #813

Reworked explicit interface implementations for properties/parametrized properties and methods.
Now all explicit implementations have a delegate call to the implementation.
This simplifies the generated code (no casting is required) and fixes some issues like:

  • Having a renamed implementation that is abstract. The old solution would generate delegating calls for abstract members which is not correct.
  • ReadOnly/WriteOnly explicit interface implementations would have getter/setter which doesn't compile.
  • Having a virtual implementation that is being overridden later by a child class. The old solution could change the logic.
  • Having one property that implements two interfaces and there is a naming difference only in the second member e.g.:
    Property A As Integer Implements IBar.A, IFoo.B

@Yozer Yozer marked this pull request as ready for review February 2, 2022 14:04
Copy link
Member

@GrahamTheCoder GrahamTheCoder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code changes look to be in the right place doing the right sorts of things and the tests look as extensive as this complicated area requires. I haven't gone through and followed the logic of it all for myself, but based on those things and your previous contributions I'm happy to merge.

I'll just give you a chance to fix the minor style point and/or flag (with an inline github comment) any areas you're not sure of or would particularly like a second pair of eyes on, thanks!

@Yozer
Copy link
Member Author

Yozer commented Feb 4, 2022

@GrahamTheCoder Thanks, I tried to write as many tests as I can to cover some weird cases.
Most of them I took from a huge VB project that I'm converting now. After these changes, all issues with interfaces were resolved.

I can still think of some rare cases that will convert incorrectly. Should I create issues for them so others are at least aware?

  1. Property in the interface being implemented non-public setter/getter i.e.:
Interface IFoo     
   Property Prop As Integer 
End Interface
Class Foo 
   Implements IFoo
   Property Prop As Integer Implements IFoo.Prop
        Get
        End Get
        Friend Set
        End Set
    End Property
End Class

The converter will generate a property with an internal setter.

  1. Not related with explicit interfaces but: Accessing backing field of the autogenerated property where MyClass is involved:
Class Foo 
    Overridable Property Prop As Integer = 5

    Sub Test()
        _Prop = 10 ' This should convert to MyClassProp = 10 not to Prop = 10
        Dim isCorrect = MyClass.Prop = 10 ' After conversion this will return 5instead of 10 because we wrote to Child.Prop
    End Sub
End Class
Class Child
    Inherits Foo
    Overrides Property Prop As Integer = 20
End Class
    public class Foo
    {
        public int MyClassProp { get; set; } = 5;

        public virtual int Prop
        {
            get => MyClassProp;

            set => MyClassProp = value;
        }

        public void Test()
        {
            Prop = 10; // This should convert to MyClassProp = 10 not to Prop = 10
            bool isCorrect = MyClassProp == 10; // After conversion this will return 5 instead of 10 because we wrote to Child.Prop
        }
    }

    public class Child : Foo
    {
        public override int Prop { get; set; } = 20;
    }

@GrahamTheCoder
Copy link
Member

Great work, thanks! Sometimes I consolidate things so the overall number of issues stays managable, but I think these are sufficiently distinct so I've created them as separate issues.

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.

VB -> C#: Explicit interface implementation adds a setter for readonly property.

2 participants