Addition of a synthesized override of object.GetHashCode() in records not aligned with the latest design

[gafter]

The following test, which should compile without any warnings or errors according to the records specification, nevertheless compiles with errors

        [Fact]
        public void ObjectEquals_NN()
        {
            var source = @"
public class A
{
    public virtual new int GetHashCode() => 0;
    public A(A a) { }
    public A() { }
}
public record B : A {
}";
            var comp = CreateCompilation(source, parseOptions: TestOptions.RegularPreview);
            comp.VerifyDiagnostics(
                // (8,15): error CS8869: 'B.GetHashCode()' does not override the method from 'object'.
                // public record B : A {
                Diagnostic(ErrorCode.ERR_DoesNotOverrideMethodFromObject, "B").WithArguments("B.GetHashCode()").WithLocation(8, 15),
                // (8,15): warning CS0659: 'B' overrides Object.Equals(object o) but does not override Object.GetHashCode()
                // public record B : A {
                Diagnostic(ErrorCode.WRN_EqualsWithoutGetHashCode, "B").WithArguments("B").WithLocation(8, 15),
                // (8,19): error CS8864: Records may only inherit from object or another record
                // public record B : A {
                Diagnostic(ErrorCode.ERR_BadRecordBase, "A").WithLocation(8, 19)
                );
        }

The first error, ERR_DoesNotOverrideMethodFromObject, is incorrect because according to the specification "The record type includes a synthesized override of object.GetHashCode(). ". The next warning is also therefore incorrect. Finally, the error ERR_BadRecordBase is not supported by the specification, which permits other class types to be used as a base class.

[AlekseyTs]

The first error, ERR_DoesNotOverrideMethodFromObject, is incorrect because according to the specification "The record type includes a synthesized override of object.GetHashCode(). ".

It is correct in a sense that the synthesized GetHashCode cannot override object.GetHashCode() due to shadowing. Perhaps the error is not perfect, but this is an error scenario and shouldn't be compiler successfully.

The next warning is also therefore incorrect.

Similarly by design.

Finally, the error ERR_BadRecordBase is not supported by the specification, which permits other class types to be used as a base class.

This behavior follows the current design and is not related to GetHashCode at all - https://github.com/dotnet/csharplang/blob/master/meetings/2020/LDM-2020-06-22.md#inheritance-between-records-and-classes.

I will amend the specification shortly

[gafter]

It is correct in a sense that the synthesized GetHashCode cannot override object.GetHashCode() due to shadowing. Perhaps the error is not perfect, but this is an error scenario and shouldn't be compiler successfully.

Shadowing doesn't affect whether or not it overrides object.GetHashCode(). The spec says it overrides object.GetHashCode() and nothing in the spec says it doesn't override it, so it does. There is no sense in which it "cannot" override it: it is expressible in IL so the necessary IL can be generated.

[AlekseyTs]

The spec says it overrides object.GetHashCode() and nothing in the spec says it doesn't override it, so it does. There is no sense in which it "cannot" override it: it is expressible in IL so the necessary IL can be generated.

I understand your point of view. However, my interpretation of specification is different and I do not think the intent was to generate IL in the way that regular language rules do not support. I agree, the specification is somewhat unclear in this regard (thank you for pointing this out), and I am planning to submit clarifications for the specification.

[gafter]

As far as I can tell, the specification is clear and unambiguous on this point. But of course it should change if it is not expressing the intent.

[AlekseyTs]

The spec has been updated

[gafter]

The spec change doesn't really make any sense:

The record type includes a synthesized override

public override bool Equals(object? obj);

It is an error if the override is declared explicitly. It is an error if the method doesn't override object.Equals(object? obj) (for example, due to shadowing in intermediate base types, etc.).

The "example" isn't an example of anything implied by the language specification (either the base language specification or any modifications to it in the records specification). It is confusing because it suggests that the specification implies something that the specification doesn't imply.

If you want hiding (or sealing, etc) of object.Equals(object) to be an error in a record's base type, just say so. Otherwise it isn't an error.

[AlekseyTs]

@gafter

The specification clearly says: "It is an error if the method doesn't override object.Equals(object? obj)". It really doesn't matter what the reason is, and I don't believe the specification should specify them. The examples are given for the benefit of the reader. If you have suggestions about improving the specification, please open a spec bug and clearly state what do you think the spec should say.

As it stands right now, I believe the implementation is in line with the specification.

[gafter]

I am not writing the specification, I am reading it. This is your spec issue.

To be clear, a synthesized method doesn't override anything unless the specification says it does (there is no other text in the base language specification or the record specification to make it do so). So there is your first suggestion: say it does. If you want certain other situations to be an error, identify them and say they are an error.

Since you removed the part about it overriding object.Equals, it doesn't. Since the spec says that make it an error, it is (always) an error. I don't believe that is what you intend, so the specification is not correct. This issue tracks that fact.

[AlekseyTs]

Since you removed the part about it overriding object.Equals, it doesn't.

Note the override modifier in the declaration of the method.

[gafter]

According to the specification this member is synthesized and not declared. So it doesn’t have a “declaration”.

[AlekseyTs]

According to the specification this member is synthesized and not declared. So it doesn’t have a “declaration”.

Could you help me to clarify the specification? I think you've got the idea what behavior we want it to specify. Everybody is different and three members of the team, which signed off on my PR for the specification change, felt that the specification reflects their understanding for the feature behavior. I'll be happy to clarify it more.

[gafter]

There are two approaches you can use to clarify the specification.

1. Describe the member, its semantics, and any errors that should arise due to it. If you take this approach you would add something like "it is an error if object.GetHashCode is hidden or sealed in the base type."
2. Say that the member exists "as if" declared by the following declaration, and then describe any exceptions to that general rule and additional rules. For example, for the Equals members you probably do not intend that there be a diagnostic on the use of the nullable annotation on the argument if the source is in a context in which nullable annotations are not enabled.

You should probably pick one of these approaches and use it wherever possible, rather than switching between them arbitrarily.