Analyzer doesn't warn for comparison of unrelated type extension values

[tjarvstrand]

The below code doesn't cause the analyzer to emit any warning for comparison of unrelated types:

extension type A(String value) implements String {}
extension type B(String value) implements String {}

void main() {
  final a = A('a');
  final b = B('b');
  if (a == b) {
    print('no');
  }
}

Flutter doctor:

[✓] Flutter (Channel stable, 3.19.1, on macOS 13.5.1 22G90 darwin-arm64, locale en-SE)
    • Flutter version 3.19.1 on channel stable at /Users/xxx/.cache/asdf/installs/flutter/3.19.1-stable
    • Upstream repository https://github.com/flutter/flutter.git
    • Framework revision abb292a07e (13 days ago), 2024-02-20 14:35:05 -0800
    • Engine revision 04817c99c9
    • Dart version 3.3.0
    • DevTools version 2.31.1

[lrhn]

Should it?

If the representation types are related, and == cannot be overridden by extension types, then the values can be equal. You are comparing two strings here, and it's not even that much of a secret since both types implement String.

An extension type which implements its representation type is as close as we can come to an "open" extension type, which makes no secret of its representation type. I'd be fine with not having any warning about comparing two strings with different sets of additional methods.

[eernstg]

Agreeing with @lrhn that it may be fine to omit the diagnostic for extension types that implement non-extension types that wouldn't cause a diagnostic, it does make sense to emit a diagnostic for other unrelated extension types:

extension type Inch(double value) {...}
extension type Centimeter(double value) {...}

void main() {
  print(Inch(1) == Centimeter(1)); // 'true', deceptively.
}

[tjarvstrand]

I have to say it's pretty confusing.

I have two wrapper types that I'd like to behave as Strings but with added type safety:

extension type EmailAddress(String value) implements String {}
extension type PhoneNumber(String value) implements String {}

While I can perhaps understand why it would make sense engineering-wise (knowing what a type extension does) for these to be comparable without a warning, it seems awkward and error prone from a developer experience point-of-view.

1. For the naive/inexperienced developer (such as me, a few hours ago) reading the above, there is nothing indicating that at compile-time, these types will sometimes be considered the same type and sometimes not.
2. I can't really imagine a use case where comparing an EmailAddress to a PhoneNumber (or and Inch to a Centimeter) would not be a mistake (I already made that mistake once, which is why I opened this issue), so it would be very helpful if the analyzer would let me know.
3. If comparing the raw values is actually what I really want to do, that is easily accomplished anyway.

[eernstg]

I proposed that we should use a keyword (e.g., open extension type vs. regular "non-open" ones) to indicate this distinction, but the rest of the language team wasn't happy about adding even more keywords to this kind of declaration.

So we have to rely on the softer property that an "open" extension type implements a non-extension type T (and it is then enforced that T is a supertype of the representation type, which includes the case where T is the representation type), and all other extension types are "non-open".

This basically means that if you have implements String then your extension type is assumed to have the property that every object of type String is an appropriate representation object for that extension type.

That's also the reason why you can do myString as EmailAddress without any warning from cast_into_extension_type, or switch (myString) { EmailAddress() => whatever }.

there is nothing indicating that at compile-time, these types will sometimes be considered the same type and sometimes not

They will not be considered the same type. In particular, an EmailAddress cannot be assigned to a variable of type PhoneNumber without a cast, or vice versa. But it might be reasonable to say that == on those two types is acceptable.

However, at this point you've convinced me that we should use a stricter approach: unrelated_type_equality_checks should simply use the same rule for extension types that it is already documented to use for all other types:

DON'T Compare references of unrelated types

So, based on the fact that EmailAddress is not a subtype of PhoneNumber, and PhoneNumber is not a subtype of EmailAddress, an == expression with those two types as receiver and operand should give rise to a warning.

This means that we will keep the "level of incompatibility" for assignments and for == similar, and that seems reasonable to me.

[tjarvstrand]

However, at this point you've convinced me that we should use a stricter approach...

That's great! Thanks for listening 🙂

[eernstg]

Thanks!

(It's up to the analyzer team to make a decision, though.)

[srawlins]

This one makes me feel squirmy. ☹️ If an EmailAddress should not be compared to a PhoneNumber, two extension types that both implement String, then surely they shouldn't be able to be equal to each other. But they can be. I don't like implementing this one way statically and another way at runtime. ☹️

What we'll be saying with this proposed change is, "You should not compare these two things. Oh, don't be fooled, they could totally be equal to each other. But you still shouldn't compare them." I think that is precisely the request. I'm not against it; I just want to phrase it this way.

I'll note that "unrelatedness" is a shared computation between unrelated_type_equality_checks and some other rules like collection_methods_unrelated_type. So this change will mean some other things will get flagged, like:

bool f(List<PhoneNumber> list, EmailAddress address) => list.contains(address);

[eernstg]

If an EmailAddress should not be compared to a PhoneNumber, two extension types that both implement String, then surely they shouldn't be able to be equal to each other.

I think the relevant question is the following: Is it going to be helpful to anyone if we insist that an EmailAddress and a PhoneNumber can be compared for equality, with no hint from our tools that this might be a mistake? I suspect that it would rarely be working as intended --- it's probably going to yield the result false, which is probably the only reasonable result, but it seems likely that the question is wrong in the first place.

Can we come up with a situation where unrelated extension types conceptually ought to be comparable, that is, a situation where we really want unrelated_type_equality_checks to remain silent? Otherwise, I'd recommend that we use the same rule as for other types: Don't compare unrelated types for equality.