Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

unrelated_type_equality_checks doesn't trigger on sibling types #58090

Open
sigurdm opened this issue Dec 17, 2019 · 5 comments
Open

unrelated_type_equality_checks doesn't trigger on sibling types #58090

sigurdm opened this issue Dec 17, 2019 · 5 comments
Labels
analyzer-linter Issues with the analyzer's support for the linter package area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. linter-false-negative linter-set-core P2 A bug or feature request we're likely to work on type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)

Comments

@sigurdm
Copy link
Contributor

sigurdm commented Dec 17, 2019

Either the description is off, or the lint. Not sure which.

Reading the documentation for #unrelated_type_equality_checks says:

Comparing references of a type where neither is a subtype of the other most likely will return false and might not reflect programmer's intent.

If we have a simplified version of the protobufEnum world:

class C {}

class A extends C {}

class B extends C {}

main() {
  print(A() == B());
}

Then A is not a subtype of B nor is B a subtype of A, and from the description it seems I would get a lint, but when I try it I don't.

If sibling types where considered unrelated it would be useful for prottobuf enums, they should never be compared between different enum types, as the answer is always false, even if the underlying value is the same.

Related to internal issue: b/146122892

@bwilkerson
Copy link
Member

The description doesn't fully describe what's currently implemented, so it should be updated. On the other hand, what's currently implemented might not be ideal and might want to be adjusted.

I believe that the description was accurate at the time it was written. Then someone pointed out that there are sometimes classes, such as CartesianPoint and PolarPoint that can be compared even though neither is a subclass of the other, and that usually such classes have a common superclass. As a result, the rule was relaxed. But it's certainly not the case that all classes with a common superclass can be compared to each other (such as the case you ran into), so I think the lint is too permissive.

The problem, as if often the case with lints, is that it isn't possible to determine user intent without having a way for users to express that intent.

One possible solution would be to introduce an annotation, such as @equalityWorksAcrossSubclasses (ok, that's way too long, but...) that would tell the linter when to allow equality checks. There's an interface (Comparable) that could also be used, but it expresses a bigger API than just ==, so it too would be too limiting in some cases.

@pq pq added the type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) label Dec 23, 2020
@LucaDillenburg
Copy link

Also, when different enums are compared, there is no error. For example:

enum Enum1 { a, b, c }
enum Enum2 { d, e, f }
void someFunction1() {
  if (Enum1.a == Enum2.d) { // should have a linter error, but it doesn't
    // ...
  }
}

@bwilkerson
Copy link
Member

There is a PR out to fix the lint for enums: dart-lang/linter#3272.

@srawlins srawlins added the P2 A bug or feature request we're likely to work on label Sep 23, 2022
@srawlins
Copy link
Member

I think we should keep the current behavior for the general case. In a type hierarchy, two expressions may have different static types, but for accidental reasons, and it is still natural to ask ==.

But for @sigurdm 's case, we can either make a pragmatic exception, since protos should generally be considered unrelated to each other, unless they have the same static type. But maybe protos will become final? And then we can just use finality.

@srawlins
Copy link
Member

Filed google/protobuf.dart#820

@devoncarew devoncarew added analyzer-linter Issues with the analyzer's support for the linter package area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. labels Nov 18, 2024
@devoncarew devoncarew transferred this issue from dart-lang/linter Nov 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
analyzer-linter Issues with the analyzer's support for the linter package area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. linter-false-negative linter-set-core P2 A bug or feature request we're likely to work on type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants