Fixes #3521 - DimAuto equality broken

[tig]

Fixes

• Fixes DimAuto equality #3521

Proposed Changes/Todos

• Add unit tests proving bad behavior
• Engineer fix

Pull Request checklist:

• I've named my PR in the form of "Fixes #issue. Terse description."
• My code follows the style guidelines of Terminal.Gui - if you use Visual Studio, hit CTRL-K-D to automatically reformat your files before committing.
• My code follows the Terminal.Gui library design guidelines
• I ran dotnet test before commit
• I have made corresponding changes to the API documentation (using /// style comments)
• My changes generate no new warnings
• I have checked my code and corrected any poor grammar or misspellings
• I conducted basic QA to assure all features are working

[dodexahedron]

Suggestion for a simpler, cleaner, and more robust implementation in the long comment in this review.

[tig]

    [Fact]
    public void DimFunc_Equal ()
    {
        Func<int> f1 = () => 0;
        Func<int> f2 = () => 0;

        Dim dim1 = Func (f1);
        Dim dim2 = Func (f2);
        Assert.Equal (dim1, dim2);

        f2 = () => 1;
        dim2 = Func (f2);
        Assert.NotEqual (dim1, dim2);
    }

After switching to record this test is failing.

I think it was bogus before. f1 and f2 are different functions and thus dim1 and dim2 are not the same. Right?

    [Fact]
    public void DimFunc_Equal ()
    {
        Func<int> f1 = () => 0;
        Func<int> f2 = () => 0;

        Dim dim1 = Func (f1);
        Dim dim2 = Func (f1);
        Assert.Equal (dim1, dim2);

        dim2 = Func (f2);
        Assert.NotEqual (dim1, dim2);

        f2 = () => 1;
        dim2 = Func (f2);
        Assert.NotEqual (dim1, dim2);
    }

I think this test is correct. Correct?

[dodexahedron]

Will take a look shortly, when I'm at my PC again.

[dodexahedron]

    [Fact]
    public void DimFunc_Equal ()
    {
        Func<int> f1 = () => 0;
        Func<int> f2 = () => 0;

        Dim dim1 = Func (f1);
        Dim dim2 = Func (f2);
        Assert.Equal (dim1, dim2);

        f2 = () => 1;
        dim2 = Func (f2);
        Assert.NotEqual (dim1, dim2);
    }

After switching to record this test is failing.

I think it was bogus before. f1 and f2 are different functions and thus dim1 and dim2 are not the same. Right?

💯

    [Fact]
    public void DimFunc_Equal ()
    {
        Func<int> f1 = () => 0;
        Func<int> f2 = () => 0;

        Dim dim1 = Func (f1);
        Dim dim2 = Func (f1);
        Assert.Equal (dim1, dim2);

        dim2 = Func (f2);
        Assert.NotEqual (dim1, dim2);

        f2 = () => 1;
        dim2 = Func (f2);
        Assert.NotEqual (dim1, dim2);
    }

I think this test is correct. Correct?

Looks better, yes.

Just for reference, here is how equality of delegates works:

Two delegates of the same type with the same targets, methods, and invocation lists are considered equal.

If the two delegates are not of the same type, they are not considered equal.

The methods and targets are compared for equality as follows:

• If the two methods being compared are both static and are the same method on the same class, the methods are considered equal and the targets are also considered equal.

• If the two methods being compared are instance methods and are the same method on the same object, the methods are considered equal and the targets are also considered equal.

• Otherwise, the methods are not considered to be equal and the targets are also not considered to be equal.

Two invocation lists are considered identical if they have the same order and the corresponding elements from the two lists represent the same method and target.

Aside from that, I think I need to pull the branch and take a look at a couple of other things about the code.

One thing is that the use of new all over the place in them looks odd to me, in the context of the diffs in github. But that doesn't give me the whole picture of course, so I want to see it in-situ.

[dodexahedron]

Also, the whole delegate equality thing got me thinking...

I want to be sure your intent is aligned with what this code will do, or at least allow.

Remember that all delegates you can write in dotnet are MulticastDelegate, which is a mutable reference type which contains an invocation list. But it's also a special type treated interestingly by the compiler in potentially unexpected ways, because of when they're evaluated.

Take this simple program for example, which showcases both delegate fun and record behavior (immutable value by default): https://dotnetfiddle.net/wYactJ

The reason two of those instances are "pointless" is because SimplePositionalRecord, PositionalRecordWithImmutableProperty, and PositionalRecordWithImmutablePropertyBackedByReadOnlyField are exactly the same thing.

Note how we can append to the one in mutable, and the method is added. Yet, even though they're all given a reference to the same delegate instance, none of them affect each other, which the second two sets of invocations demonstrate.

The delegate is created and evaluated at time of instantiation. That adds some color to the delegate equality rules above, but it still isn't the whole story.

Extra unnecessary babbling that is safe to ignore

IL-wise, the delegate instances that are allocated by the += calls are all fields of the main Program class of type Action<string>? with these names:

• <0>__FirstAction
• <1>__SecondAction
• '<2>__ThirdAction'
• '<3>__FourthAction'

And the methods they each refer to, respectively, are:

• <<Main>$>g__FirstAction|0_0
• <<Main>$>g__SecondAction|0_1
• <<Main>$>g__ThirdAction|0_2
• <<Main>$>g__FourthAction|0_3

If we hadn't used Action, and instead assigned a signature-compatible custom delegate we created, we'd have type casts AND no equality in the IL, thanks to the extra classes for the delegates. But it's legal to do that.

If you use lambdas or other anonymous delegates, it all gets even messier, as those each create whole-ass classes, too, also created and evaluated at instantiation, not invocation (which is actually exactly what creates the closure capture problem you may have seen VS warn you of sometimes).

[dodexahedron]

Added a couple of minor comments about explicit properties in records not being needed when they're already defined in the primary constructor.

[dodexahedron]

Also, it's been an absolute hell of a couple of weeks...

Things are clearing up, though, so I'm gonna have more time and desire to work on TG and/or TG-connected stuff over the next few days, finally. 🥳

[tig]

Also, it's been an absolute hell of a couple of weeks...

Things are clearing up, though, so I'm gonna have more time and desire to work on TG and/or TG-connected stuff over the next few days, finally. 🥳

Sorry to hear. Life can be hard sometimes.

Thanks for looking at this. Learned a bunch.

[dodexahedron]

Also, it's been an absolute hell of a couple of weeks...
Things are clearing up, though, so I'm gonna have more time and desire to work on TG and/or TG-connected stuff over the next few days, finally. 🥳

Sorry to hear. Life can be hard sometimes.

Thanks for looking at this. Learned a bunch.

Excellent! That was the hope. :)

It's an under-utilized feature, but a really handy one (records, I mean). They can save you lots of effort and give some consistency to implementation that you no longer have to do yourself. I love 'em when they can be used.

Since all record types have value semantics out of the box (unless you manually do bad things to break it), record classes, in particular, have the neat property of enabling treatment of them as value types, but they're still reference types, which can be useful in certain scenarios.

These can and should at some point be turned into readonly record struct types, but we can do that in another pass. The use of them still looks basically the same at the point of consumption, but they're actually structs at that point.

readonly is necessary for those to make them immutable. record struct without readonly is mutable by default (not sure why they went that way, but whatev).

The reason I wouldn't say just do it now is because it would be smart to add in and ref [readonly] for them, in various method parameter uses, for best efficiency. While that could just be blanket applied, a more precise touch is best (though not mandatory).

Yeah. Life gets interesting when perfect storms of vendor failures plus deadlines plus otherwise innocent changes that happen to coincide time-wise with some of those failures come together to make a ton of extra work. But hey - job security, I guess? 😅

[dodexahedron]

Oh and more record use info:

If you want a property declared in the primary constructor to have a default value, like a field initializer, the standard optional parameter syntax is how you do it. It gets you both initialization with no need for callers to supply all parameters, as well as explicit construction without the initialization happening, all in the same line.

[dodexahedron]

And you can always (and it's a good idea to) add the IEqualityOperators<TheRecordType,TheRecordType,bool> interface to them, because they all already support it. Another thing I'm not sure why they didn't just make default behavior in the generated code.

ReSharper will suggest it for you, so you can add it with a click, rather than writing it yourself.

IEquatable<T> and IComparable<T> are therefore also pretty easy to implement, but they do require at least letting R# generate the basic form of them. Totally optional of course, but handy sometimes and enhances their use with things like ordered collections.

[dodexahedron]

Sent you tig#38 as a quick suggestion, primarily because of AoT.