Skip to content
This repository has been archived by the owner on Nov 20, 2024. It is now read-only.

Recommend using runtimeType in hash_and_equals #1943

Merged
merged 1 commit into from
Aug 24, 2021
Merged

Conversation

micimize
Copy link
Contributor

@micimize micimize commented Jan 8, 2020

Description

Using is in operator == is usually wrong as it makes the == operator asymmetric.
dart-lang/sdk#36004 (comment)

I based my early == overrides on the hash_and_equals docs, so I'm sure others have done the same

> Using `is` in `operator ==` is usually wrong as it makes the `==` operator asymmetric.
dart-lang/sdk#36004 (comment)
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@micimize
Copy link
Contributor Author

micimize commented Jan 8, 2020

@googlebot I signed it!

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes and removed cla: no labels Jan 8, 2020
@pq
Copy link
Member

pq commented Jan 10, 2020

@bwilkerson

@bwilkerson
Copy link
Member

I disagree with Hixie on this one. The use of is can make an equals method asymmetric, but does not guarantee that it is. Note also dart-lang/sdk#36004 (comment), which correctly states that using runtimeType is also not always the the right way to implement equality.

The truth of the matter is that defining equality correctly is hard and there is no single right way to do so. If there were, the equality would be defined by the language rather than being something that developers had to define.

If we want to update the docs to reflect that, then I think the better way to do so would be to have multiple different "good" examples to illustrate that there isn't a one-size-fits-all solution.

As for this specific change, there's no reason to check both is and runtimeType. If the runtime types are the same then is will aways return true.

@micimize
Copy link
Contributor Author

micimize commented Jan 10, 2020

is causes asymmetry if the class is extended (dartpad, gist). I believe extending classes is fairly common, so I think runtimeType should be the default recommendation, as it is the safest.

is was just to cast the value and fit the original example's style. Maybe I could amend the PR to

class Better {
  final int value;
  Better(this.value);
  @override
  bool operator ==(Object other){
    if (other.runtimeType != runtimeType){
      return false;
    }
    final Better typedOther = other;
    return typedOther.runtimeType == runtimeType &&
      typedOther.value == value;
  }
  @override
  int get hashCode => value.hashCode;
}

/// We know this class will not be extended, and can therefore use `is` rather than `runtimeType`
class _Better {
  final int value;
  _Better(this.value);
  @override
  bool operator ==(Object other) => other is _Better && other.value == value;
  @override
  int get hashCode => value.hashCode;
}

Seems a little long winded though

@bwilkerson
Copy link
Member

It causes asymmetry if the class is extended (...), but I do think it should be the default recommendation, as it is the safest.

Are all of the "it"s above referring to the use of is, or are the second and third referring to runtimeType?

@micimize
Copy link
Contributor Author

@bwilkerson edited for clarification:

is causes asymmetry if the class is extended. I believe extending classes is fairly common, so I think runtimeType should be the default recommendation, as it is the safest.

Would my proposed changes be preferable?

@micimize
Copy link
Contributor Author

@bwilkerson cleaning up old PRs – any more thoughts on this?

@bwilkerson
Copy link
Member

No, no new thoughts. I still don't object to the change, but I think it would be better if the docs had more examples of good styles to point out to users there there isn't any single "right" way to write equals.

@pq Do you want to land this as-is?

@pq
Copy link
Member

pq commented Aug 24, 2021

I agree the docs could be improved and also that this doesn't need to wait in the meantime. Thanks for your patience @micimize!

What's a prefered name and email for AUTHORS?

@micimize
Copy link
Contributor Author

Thanks @pq! – Michael Joseph Rosenthal <rosenthalm93@gmail.com> would be good if you want to add it for something this small.

@pq pq merged commit b50ce34 into dart-lang:master Aug 24, 2021
@pq pq mentioned this pull request Aug 24, 2021
@micimize micimize deleted the patch-1 branch August 25, 2021 00:50
copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Aug 23, 2023
> Using `is` in `operator ==` is usually wrong as it makes the `==` operator asymmetric.
#36004 (comment)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants