Deprecates `hashValues` and `hashList` #30674

werainkhatri · 2022-01-04T19:52:51Z

Deprecates hashValues and hashList in favour of Object.hash and Object.hashAll respectively.

Supposed to land after below listed PRs

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on
writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

flutter-dashboard · 2022-01-04T19:52:53Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Hixie · 2022-01-06T20:14:24Z

lib/ui/hash_codes.dart

-                                    result = _Jenkins.combine(result, arg19);
-                                    if (!identical(arg20, _hashEnd)) {
-                                      result = _Jenkins.combine(result, arg20);
-                                      // I can see my house from here!


i'm sad that my joke is going away :-(

Sorry to take that away 😢 . I loved it btw! Made me laugh IRL.

Your joke lives on 🖖🏾

lib/ui/hash_codes.dart

lib/ui/painting.dart

lib/web_ui/lib/src/ui/hash_codes.dart

Hixie · 2022-01-06T20:17:21Z

lib/web_ui/test/hash_codes_test.dart

@@ -1,127 +0,0 @@
-// Copyright 2013 The Flutter Authors. All rights reserved.


I would keep this file (with suitable //ignores for the deprecation warnings and with the numbers adjusted as necessary). That will act as an effective test that the deprecated API still works.

What problem motivated this test?
It seems very specific to some issue.

cc @yjbanov

Restored the test.

rakudrama

It would be worth validating performance after this change.

Are there any regressions in code that predominantly uses the changes to Object.hash
Are there any regressions in unmigrated code that still uses hashValues?

lib/ui/hash_codes.dart

rakudrama · 2022-01-12T16:50:25Z

lib/ui/hash_codes.dart

-// Borrowed from the dart sdk: sdk/lib/math/jenkins_smi_hash.dart.
-class _Jenkins {
-  static int combine(int hash, Object? o) {
-    assert(o is! Iterable);


How important is this assertion?

Object.hash has no similar assertion, and never will, since it is possible to have an Iterable that does define a well-behaved hashCode/==, e.g. BuiltList https://pub.dev/documentation/built_collection/latest/built_collection/BuiltList/hashCode.html)

cc @Hixie @sigurdm

I don't think it is very important - I think it is guarding against thinking you are doing a deep hash while you most probably are not (the default List has identity-based hashCode). This is not foolproof, and for example won't guard against Maps that have the same issue.

I agree that if an Iterable implements a non-identity hashCode then the assertion is not-needed.

Not sure if there is a way of asking that question.

rakudrama · 2022-01-12T16:57:04Z

lib/web_ui/test/hash_codes_test.dart

@@ -1,127 +0,0 @@
-// Copyright 2013 The Flutter Authors. All rights reserved.


What problem motivated this test?
It seems very specific to some issue.

lib/web_ui/lib/src/ui/hash_codes.dart

flutter-dashboard · 2022-02-03T14:51:52Z

This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold.

skia-gold · 2022-04-14T03:58:25Z

Gold has detected about 38 new digest(s) on patchset 5.
View them at https://flutter-engine-gold.skia.org/cl/github/30674

werainkhatri · 2022-04-14T13:27:35Z

@Hixie is this ready to merge?

werainkhatri · 2022-05-25T13:13:58Z

waiting for flutter/packages#1581 to be merged, to prevent any rolling deprecation issues :)

a-siva · 2022-05-31T23:35:43Z

This appears to break the framework builds and the Flutter HHH builds

jason-simmons · 2022-06-01T00:46:02Z

Landed flutter/flutter@116d657 in the framework, which makes it compatible with this engine PR.

* deprecate hashValues and hashList * rm unused imports * missing periods and const lists * improves hashing of lists * ignore: avoid_classes_with_only_static_members * migrate new classes * refactor: update version of deprecation

flutter-dashboard bot added the needs tests label Jan 4, 2022

werainkhatri marked this pull request as draft January 4, 2022 20:30

werainkhatri force-pushed the deprecate-hash branch from 56f04c4 to 23220a6 Compare January 5, 2022 13:34

werainkhatri changed the title ~~Deprecates hashValues and hashList~~ [WIP] Deprecates hashValues and hashList Jan 5, 2022

flutter-dashboard bot added the platform-web Code specifically for the web engine label Jan 5, 2022

werainkhatri force-pushed the deprecate-hash branch from 23220a6 to c2913f9 Compare January 5, 2022 16:16

werainkhatri marked this pull request as ready for review January 6, 2022 19:25

werainkhatri changed the title ~~[WIP] Deprecates hashValues and hashList~~ Deprecates hashValues and hashList Jan 6, 2022