Skip to content
This repository has been archived by the owner on Feb 25, 2025. It is now read-only.

Deprecates hashValues and hashList #30674

Merged
merged 7 commits into from
May 31, 2022
Merged

Deprecates hashValues and hashList #30674

merged 7 commits into from
May 31, 2022

Conversation

werainkhatri
Copy link
Member

@werainkhatri werainkhatri commented Jan 4, 2022 •
edited
Loading

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

Fixes flutter/flutter#85431

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
Copy link

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.

@werainkhatri werainkhatri marked this pull request as draft January 4, 2022 20:30
@werainkhatri werainkhatri changed the title Deprecates hashValues and hashList [WIP] Deprecates hashValues and hashList Jan 5, 2022
@flutter-dashboard flutter-dashboard bot added the platform-web Code specifically for the web engine label Jan 5, 2022
@werainkhatri werainkhatri marked this pull request as ready for review January 6, 2022 19:25
@werainkhatri werainkhatri changed the title [WIP] Deprecates hashValues and hashList Deprecates hashValues and hashList Jan 6, 2022
Hixie
Hixie reviewed Jan 6, 2022
View reviewed changes
lib/ui/hash_codes.dart Outdated
result = _Jenkins.combine(result, arg19);
if (!identical(arg20, _hashEnd)) {
result = _Jenkins.combine(result, arg20);
// I can see my house from here!
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your joke lives on 🖖🏾

Hixie
Hixie reviewed Jan 6, 2022
View reviewed changes
Hixie
Hixie reviewed Jan 6, 2022
View reviewed changes
Hixie
Hixie reviewed Jan 6, 2022
View reviewed changes
Hixie
Hixie reviewed Jan 6, 2022
View reviewed changes
Hixie
Hixie reviewed Jan 6, 2022
View reviewed changes
lib/web_ui/test/hash_codes_test.dart Outdated
@@ -1,127 +0,0 @@
// Copyright 2013 The Flutter Authors. All rights reserved.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @yjbanov

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Restored the test.

@werainkhatri werainkhatri requested a review from Hixie January 6, 2022 23:30
rakudrama
rakudrama reviewed Jan 12, 2022
View reviewed changes
Copy link
Contributor

@rakudrama rakudrama left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated
// 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);
Copy link
Contributor

@rakudrama rakudrama Jan 12, 2022 •
edited by werainkhatri
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @Hixie @sigurdm

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

lib/web_ui/test/hash_codes_test.dart Outdated
@@ -1,127 +0,0 @@
// Copyright 2013 The Flutter Authors. All rights reserved.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@werainkhatri werainkhatri force-pushed the deprecate-hash branch 2 times, most recently from 124a00b to 1740683 Compare January 14, 2022 11:13
@werainkhatri werainkhatri mentioned this pull request Jan 24, 2022
Open
@flutter-dashboard
Copy link

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.

@werainkhatri werainkhatri force-pushed the deprecate-hash branch 3 times, most recently from c9c8d24 to 7782a0d Compare March 5, 2022 03:34
@skia-gold
Copy link

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

@werainkhatri
Copy link
Member Author

@Hixie is this ready to merge?

@werainkhatri werainkhatri added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label May 25, 2022
@werainkhatri werainkhatri removed the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label May 25, 2022
@werainkhatri
Copy link
Member Author

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

@werainkhatri werainkhatri merged commit 87c64bc into flutter:main May 31, 2022
@werainkhatri werainkhatri deleted the deprecate-hash branch May 31, 2022 15:50
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request May 31, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 31, 2022
@engine-flutter-autoroll
87c64bc Deprecates hashValues and hashList (flutter/engine#30674)
1b4f0f3
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request May 31, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 31, 2022
@engine-flutter-autoroll
87c64bc Deprecates hashValues and hashList (flutter/engine#30674)
4e8f96c
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request May 31, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 31, 2022
@engine-flutter-autoroll
87c64bc Deprecates hashValues and hashList (flutter/engine#30674)
2735c6a
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request May 31, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 31, 2022
@engine-flutter-autoroll
87c64bc Deprecates hashValues and hashList (flutter/engine#30674)
38fef4a
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request May 31, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 31, 2022
@engine-flutter-autoroll
87c64bc Deprecates hashValues and hashList (flutter/engine#30674)
b03f4ea
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request May 31, 2022
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 31, 2022
@engine-flutter-autoroll
87c64bc Deprecates hashValues and hashList (flutter/engine#30674)
d0d45aa
@a-siva
Copy link
Contributor

a-siva commented May 31, 2022

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

@jason-simmons
Copy link
Member

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

@werainkhatri werainkhatri added the tech-debt Technical debt, code quality, testing, etc. label Jun 1, 2022
houhuayong pushed a commit to houhuayong/engine that referenced this pull request Jun 21, 2022
@werainkhatri @houhuayong
Deprecates hashValues and hashList (flutter#30674)
0162162 
* 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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kevmoo kevmoo kevmoo left review comments

@rakudrama rakudrama rakudrama left review comments

@sigurdm sigurdm sigurdm left review comments

@Hixie Hixie Hixie approved these changes

Assignees
No one assigned
Labels
needs tests platform-web Code specifically for the web engine tech-debt Technical debt, code quality, testing, etc.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Deprecate hashValues and hashList in favor of Object.hash/hashAll.
8 participants
@werainkhatri @skia-gold @a-siva @jason-simmons @kevmoo @Hixie @rakudrama @sigurdm