dart-lang / lints

Official Dart lint rules; the core and recommended set of lints suggested by the Dart team.
https://pub.dev/packages/lints
BSD 3-Clause "New" or "Revised" License
118 stars 30 forks source link

Remove `avoid_null_checks_in_equality_operators` from recommended #200

Closed srawlins closed 2 months ago

srawlins commented 3 months ago

We introduced a new Warning called NON_NULLABLE_EQUALS_PARAMETER a few releases ago. It warns when the parameter of an operator == override has a nullable type:

"The parameter type of '==' operators should be non-nullable."

I didn't realize it at the time, but that new warning, plus null safety, basically replace the avoid_null_checks_in_equality_operators lint rule. This rule reports doing any null-check work on a nullable parameter of an operator == override.

As I found out when I migrated the tests from the legacy framework, the rule is a bit non-sensical now because of the redundancy. Every test case either has a WarningCode.NON_NULLABLE_EQUALS_PARAMETER, if it features a nullable parameter, or it features another warning like WarningCode.UNNECESSARY_NULL_COMPARISON_TRUE or StaticWarningCode.INVALID_NULL_AWARE_OPERATOR, if the parameter is non-nullable and is compared to null.

devoncarew commented 2 months ago

cc @natebosch, @goderbauer, and @lrhn for thoughts

lrhn commented 2 months ago

Are there exceptions to the NON_NULLABLE_EQUALS_PARAMETER warning. For example of the parameter type is neither nullable nor non-nullable?

class C<T extends C<T>?> {
  bool operator ==(covariant T o) => ...
}

where T is potentially nuallble. Or even

extension type Maybe(Object? o) { 
  bool check(C me) { ... }
}

class C {
  bool operator ==(covariant Maybe other) => other.check(me);
}

where you won't get a warning, and you can't really provide a non-potentially nullable type with the same effect. In that case, the author may feel inclined to do an == null check. (On the other hand, that's such a convoluted case, that I doubt anyone will hit it, and if they do, I'm not sure I want to help them.)

Probably fine.

natebosch commented 2 months ago

Removing SGTM.

devoncarew commented 2 months ago

resolution: sgtm to remove

srawlins commented 2 months ago

Are there exceptions to the NON_NULLABLE_EQUALS_PARAMETER warning. For example of the parameter type is neither nullable nor non-nullable?

I get "invalid override" for this example, and for the extension type example; it seems even with the "covariant" keyword, these are illegal? But no "NON_NULLABLE_EQUALS_PARAMETER" warning. Given this code:

class C<T extends C<T>?> {
  bool operator ==(covariant T o) => false;
}

class D {
  bool operator ==(covariant int? o) => false;
}

void main() {}

both CFE and analyzer report that C.== and D.== are invalid. NON_NULLABLE_EQUALS_PARAMETER is only reported when the parameter type is Object? or dynamic.

devoncarew commented 2 months ago

closed by https://github.com/dart-lang/lints/pull/201