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.

Sign up for GitHub

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

Jump to bottom

Fix a type error that occurs comparing two large maps with deepEquals. #2442

Merged
merged 6 commits into from
Jan 17, 2025
Merged

Fix a type error that occurs comparing two large maps with deepEquals. #2442

merged 6 commits into from
Jan 17, 2025

Conversation

matanlurey
Copy link
Contributor

Closes #2441.

I am not 100% sure why this occurs, and why the repro case needs to be of a certain design/size, but here ya go.

@matanlurey matanlurey requested a review from natebosch December 29, 2024 02:21
@matanlurey matanlurey requested a review from a team as a code owner December 29, 2024 02:21
@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 29, 2024 •
edited
Loading

PR Health

Changelog Entry ✔️
Package Changed Files

Changes to files need to be accounted for in their respective changelogs.

jakemac53
jakemac53 reviewed Jan 2, 2025
View reviewed changes
pkgs/checks/lib/src/describe.dart Outdated
// List<Iterable<String>> which ends up as a type error in dart:collection
// when the underlying list is actually a List<List<String>>. See
// https://github.com/dart-lang/test/issues/2441 for more details.
elements.replaceRange(_maxItems - 1, elements.length, <List<String>>[
Copy link
Contributor

@jakemac53 jakemac53 Jan 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Another option could be to make this a top level variable (const?)? Then we wouldn't have to force inference and it would be shared across invocations.

@jakemac53
Copy link
Contributor

Interesting real world case of a covariant generics fail.... I haven't seen one of these maybe ever in actual practice haha.

@jakemac53
Copy link
Contributor

jakemac53 commented Jan 2, 2025 •
edited
Loading

Fwiw this is the actual culprit line that creates a List<List<String>> https://github.com/dart-lang/test/blob/master/pkgs/checks/lib/src/describe.dart#L48.

A different solution would be to convert this to return an iterable, which may have (good or bad) performance characteristics, something like this:

      return key.take(key.length - 1).followedBy(
          ['${key.last}: ${value.first}']).followedBy(value.skip(1));

There is a similar issue here https://github.com/dart-lang/test/blob/master/pkgs/checks/lib/src/describe.dart#L88

jakemac53
jakemac53 reviewed Jan 2, 2025
View reviewed changes
pkgs/checks/test/extensions/map_test.dart Outdated
@@ -29,6 +29,65 @@ void main() {
test('values', () {
check(_testMap).values.contains(1);
});
test('can be described failing compared to another large map', () {
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like the same issue exists for lists so it would be good to also test that (although I think your fix will work for both).

@matanlurey
Copy link
Contributor Author

It might be a few days until I return to this PR (side-project) so it will need some support if you want it merged sooner :)

@jakemac53
Copy link
Contributor

No worries, I don't think there is a rush on this. If you do need to hand it off though let us know and one of us could probably take it over.

@natebosch
Use a const list, test the literal function directly
de0079a 
Use a separate const list so the inference does not use the argument
context.

Add a test for specifically pretty printing large collections. Remove
the test that indirectly tested the behavior.
@natebosch
Merge branch 'master' into fix-deep-map-equals-type-failure
fbb06f0
@natebosch natebosch requested a review from jakemac53 January 14, 2025 21:16
@matanlurey
Copy link
Contributor Author

Thanks for taking it over!

@natebosch natebosch merged commit 073ef8b into dart-lang:master Jan 17, 2025
58 checks passed
copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Jan 23, 2025
@devoncarew
[deps] rev core, ecosystem, http, test, web, webdev
a1e5451 
Revisions updated by `dart tools/rev_sdk_deps.dart`.

core (https://github.com/dart-lang/core/compare/7a71ad6..72a2060):
  72a20603  2025-01-17  Nate Bosch  Remove sorting of allowedHelp maps (dart-lang/core#852)
  a59cbeaf  2025-01-13  Kevin Moore  [os_detect] move to pkg:web, prepare publish (dart-lang/core#850)
  48c3c458  2025-01-13  Kevin Moore  [path] fix tests to compile/run with wasm (dart-lang/core#851)
  2ac228bd  2025-01-09  Devon Carew  update lint issue templates (dart-lang/core#849)
  513fa2f0  2025-01-08  Devon Carew  fix a line break issue in the package table (dart-lang/core#846)
  eb74f032  2025-01-06  Devon Carew  Update package:collection for the new `strict_top_level_inference` lint (dart-lang/core#735)

ecosystem (https://github.com/dart-lang/ecosystem/compare/efe4ee4..682c8ef):
  682c8ef  2025-01-17  Moritz  Update `dart_apitool` version in health workflow. (dart-lang/ecosystem#338)
  94b4cea  2025-01-14  Kevin Moore  [puppy] actually rename command to `run`, sort directories, better logs (dart-lang/ecosystem#337)
  f2d3178  2025-01-10  Devon Carew  rename the 'map' command to 'run' (dart-lang/ecosystem#336)
  25c57b8  2025-01-10  Kevin Moore  [puppy] Introduce a package for misc Dart CLI tools (dart-lang/ecosystem#335)
  21e81c5  2025-01-08  Kevin Moore  [corpus] update to latest analyzer, bump min SDK (dart-lang/ecosystem#334)

http (https://github.com/dart-lang/http/compare/6ecd13a..27184eb):
  27184eb  2025-01-21  Brian Quinlan  Remove bespoke code that handled blocking callbacks (dart-lang/http#1450)
  7271367  2025-01-20  Kevin Moore  [http] prepare v1.3.0 release (dart-lang/http#1451)
  7f50fc5  2025-01-15  Brian Quinlan  Prepare cupertino_http 2.0.2 for release (dart-lang/http#1449)
  2bc4cc9  2025-01-15  Brian Quinlan  Fix incorrect response processing (dart-lang/http#1448)
  531d3e5  2025-01-09  Kevin Moore  [http2, conformance_tests] update dependencies (dart-lang/http#1443)
  f0bcf02  2025-01-06  Brian Quinlan  Add tests to verify NUL, CR & LF header value behavior (dart-lang/http#1440)

test (https://github.com/dart-lang/test/compare/f364fc8..7fc9521):
  7fc95218  2025-01-17  Nate Bosch  Drop reference to legacy communication protocol (dart-lang/test#2446)
  073ef8bd  2025-01-17  Matan Lurey  Fix a type error that occurs comparing two large maps with `deepEquals`. (dart-lang/test#2442)
  b5bfba54  2025-01-17  Nate Bosch  Unblock CI: Ignore deprecation and skip a test (dart-lang/test#2445)

web (https://github.com/dart-lang/web/compare/af5de5e..fd3d988):
  fd3d988  2025-01-14  KennethHung  Adapt paths for different development environments (dart-lang/web#336)
  7f440e4  2025-01-10  KennethHung  Remove renames where the type doesn't exist anymore (dart-lang/web#334)
  3b06da9  2025-01-09  KennethHung  Add missing FileReader event getters (dart-lang/web#333)

webdev (https://github.com/dart-lang/webdev/compare/e72f365..4a92ba5):
  4a92ba58  2025-01-13  Ben Konyi  [ DWDS ] Reset version to 24.3.3-wip (dart-lang/webdev#2569)
  cc5aff69  2025-01-10  Ben Konyi  [ DWDS ] Relax DDS constraint, prepare for 24.3.2 release
  562ab622  2025-01-10  Ben Konyi  [ DWDS ] Reset version to 24.3.2-wip
  a7c8fe76  2025-01-10  Ben Konyi  Allow for setting custom DDS port (dart-lang/webdev#2546)
  7dc5c3a9  2025-01-09  Ben Konyi  Rename copied SDK directory prefix from 'sdk copy' to 'sdk_copy' (dart-lang/webdev#2557)
  659e1dfc  2025-01-08  Jessy Yameogo  Added support for some debugging APIs with the DDC library bundle format - part 5 (dart-lang/webdev#2549)
  fcd906fc  2025-01-07  Srujan Gaddam  Add ignores for deprecated web libraries with a TODO and move dwds to 24.4.0-wip (dart-lang/webdev#2559)

Change-Id: I3e847d24146bfb6891e77098591b785a0d15a445
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/405243
Auto-Submit: Devon Carew <devoncarew@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Konstantin Shcheglov <scheglov@google.com>
copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Jan 23, 2025
@devoncarew
Revert "[deps] rev core, ecosystem, http, test, web, webdev"
0be3448 
This reverts commit a1e5451.

Reason for revert: it looks like golem is broken

Original change's description:
> [deps] rev core, ecosystem, http, test, web, webdev
>
> Revisions updated by `dart tools/rev_sdk_deps.dart`.
>
> core (https://github.com/dart-lang/core/compare/7a71ad6..72a2060):
>   72a20603  2025-01-17  Nate Bosch  Remove sorting of allowedHelp maps (dart-lang/core#852)
>   a59cbeaf  2025-01-13  Kevin Moore  [os_detect] move to pkg:web, prepare publish (dart-lang/core#850)
>   48c3c458  2025-01-13  Kevin Moore  [path] fix tests to compile/run with wasm (dart-lang/core#851)
>   2ac228bd  2025-01-09  Devon Carew  update lint issue templates (dart-lang/core#849)
>   513fa2f0  2025-01-08  Devon Carew  fix a line break issue in the package table (dart-lang/core#846)
>   eb74f032  2025-01-06  Devon Carew  Update package:collection for the new `strict_top_level_inference` lint (dart-lang/core#735)
>
> ecosystem (https://github.com/dart-lang/ecosystem/compare/efe4ee4..682c8ef):
>   682c8ef  2025-01-17  Moritz  Update `dart_apitool` version in health workflow. (dart-lang/ecosystem#338)
>   94b4cea  2025-01-14  Kevin Moore  [puppy] actually rename command to `run`, sort directories, better logs (dart-lang/ecosystem#337)
>   f2d3178  2025-01-10  Devon Carew  rename the 'map' command to 'run' (dart-lang/ecosystem#336)
>   25c57b8  2025-01-10  Kevin Moore  [puppy] Introduce a package for misc Dart CLI tools (dart-lang/ecosystem#335)
>   21e81c5  2025-01-08  Kevin Moore  [corpus] update to latest analyzer, bump min SDK (dart-lang/ecosystem#334)
>
> http (https://github.com/dart-lang/http/compare/6ecd13a..27184eb):
>   27184eb  2025-01-21  Brian Quinlan  Remove bespoke code that handled blocking callbacks (dart-lang/http#1450)
>   7271367  2025-01-20  Kevin Moore  [http] prepare v1.3.0 release (dart-lang/http#1451)
>   7f50fc5  2025-01-15  Brian Quinlan  Prepare cupertino_http 2.0.2 for release (dart-lang/http#1449)
>   2bc4cc9  2025-01-15  Brian Quinlan  Fix incorrect response processing (dart-lang/http#1448)
>   531d3e5  2025-01-09  Kevin Moore  [http2, conformance_tests] update dependencies (dart-lang/http#1443)
>   f0bcf02  2025-01-06  Brian Quinlan  Add tests to verify NUL, CR & LF header value behavior (dart-lang/http#1440)
>
> test (https://github.com/dart-lang/test/compare/f364fc8..7fc9521):
>   7fc95218  2025-01-17  Nate Bosch  Drop reference to legacy communication protocol (dart-lang/test#2446)
>   073ef8bd  2025-01-17  Matan Lurey  Fix a type error that occurs comparing two large maps with `deepEquals`. (dart-lang/test#2442)
>   b5bfba54  2025-01-17  Nate Bosch  Unblock CI: Ignore deprecation and skip a test (dart-lang/test#2445)
>
> web (https://github.com/dart-lang/web/compare/af5de5e..fd3d988):
>   fd3d988  2025-01-14  KennethHung  Adapt paths for different development environments (dart-lang/web#336)
>   7f440e4  2025-01-10  KennethHung  Remove renames where the type doesn't exist anymore (dart-lang/web#334)
>   3b06da9  2025-01-09  KennethHung  Add missing FileReader event getters (dart-lang/web#333)
>
> webdev (https://github.com/dart-lang/webdev/compare/e72f365..4a92ba5):
>   4a92ba58  2025-01-13  Ben Konyi  [ DWDS ] Reset version to 24.3.3-wip (dart-lang/webdev#2569)
>   cc5aff69  2025-01-10  Ben Konyi  [ DWDS ] Relax DDS constraint, prepare for 24.3.2 release
>   562ab622  2025-01-10  Ben Konyi  [ DWDS ] Reset version to 24.3.2-wip
>   a7c8fe76  2025-01-10  Ben Konyi  Allow for setting custom DDS port (dart-lang/webdev#2546)
>   7dc5c3a9  2025-01-09  Ben Konyi  Rename copied SDK directory prefix from 'sdk copy' to 'sdk_copy' (dart-lang/webdev#2557)
>   659e1dfc  2025-01-08  Jessy Yameogo  Added support for some debugging APIs with the DDC library bundle format - part 5 (dart-lang/webdev#2549)
>   fcd906fc  2025-01-07  Srujan Gaddam  Add ignores for deprecated web libraries with a TODO and move dwds to 24.4.0-wip (dart-lang/webdev#2559)
>
> Change-Id: I3e847d24146bfb6891e77098591b785a0d15a445
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/405243
> Auto-Submit: Devon Carew <devoncarew@google.com>
> Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
> Commit-Queue: Konstantin Shcheglov <scheglov@google.com>

Change-Id: I27c20ef89095a7d602c1a609d5533a27a7683a47
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/405445
Commit-Queue: Konstantin Shcheglov <scheglov@google.com>
Auto-Submit: Devon Carew <devoncarew@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jakemac53 jakemac53 jakemac53 approved these changes

@natebosch natebosch Awaiting requested review from natebosch

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

check(...).deepEquals(...) fails to describe a large-ish Map
3 participants
@matanlurey @jakemac53 @natebosch