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.

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

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

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

github-actions bot commented Dec 29, 2024

PR Health

Changelog Entry ✔️
Package Changed Files

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

// 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

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

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

@@ -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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

check(...).deepEquals(...) fails to describe a large-ish Map
2 participants