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

Update package:collection for the new strict_top_level_inference lint #735

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

devoncarew
Copy link
Member

  • update package:collection readme to get analysis to run

  • I’ve reviewed the contributor guide and applied the relevant portions to this PR.
Contribution guidelines:

Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.

Copy link

github-actions bot commented Dec 20, 2024

PR Health

Breaking changes ✔️
Package Change Current Version New Version Needed Version Looking good?
collection Non-Breaking 1.19.1 1.20.0-wip 1.20.0 ✔️
Changelog Entry ✔️
Package Changed Files

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

Coverage ⚠️
File Coverage
pkgs/collection/lib/src/unmodifiable_wrappers.dart 💔 75 % ⬇️ 3 %

This check for test coverage is informational (issues shown here will not fail the PR).

This check can be disabled by tagging the PR with skip-coverage-check.

API leaks ✔️

The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.

Package Leaked API symbols
License Headers ✔️
// Copyright (c) 2024, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.
Files
no missing headers

All source files should start with a license header.

@devoncarew
Copy link
Member Author

From the new strict_top_level_inference lint, we see two sets of issues here:

   info - lib/src/unmodifiable_wrappers.dart:199:7 - Missing type annotation. Try adding a type annotation. - strict_top_level_inference
   info - lib/src/unmodifiable_wrappers.dart:203:7 - Missing type annotation. Try adding a type annotation. - strict_top_level_inference

That's from https://github.com/dart-lang/core/blob/main/pkgs/collection/lib/src/unmodifiable_wrappers.dart#L199:

  /// Throws an [UnsupportedError];
  /// operations that change the map are disallowed.
  set first(_) => _throw();

  /// Throws an [UnsupportedError];
  /// operations that change the map are disallowed.
  set last(_) => _throw();

Here, I don't know what purpose first and last are for; they're not overrides from Map.

The next set of issues:

   info - test/extensions_test.dart:2228:20 - Missing type annotation. Try adding a type annotation. - strict_top_level_inference
   info - test/extensions_test.dart:2228:23 - Missing type annotation. Try adding a type annotation. - strict_top_level_inference
   info - test/extensions_test.dart:2228:27 - Missing type annotation. Try adding a type annotation. - strict_top_level_inference

are from https://github.com/dart-lang/core/blob/main/pkgs/collection/test/extensions_test.dart#L2228:

Never unreachable([_, __, ___]) => fail('Unreachable');

Perhaps we should not include wildcard variables in the lint?

cc @lrhn, @natebosch, @srawlins

@lrhn
Copy link
Member

lrhn commented Dec 20, 2024

The setters should just be removed from the Map wrapper, they're mistakes.

Other than that, we could exclude wildcards, but since the lint should only trigger if you don't inherit a type, I think the method should actually be explicit about its types.

Or, more relevantly, since it's in the test directory, it is not public API, and it should probably just be private.
Or the entire test directory should be exempt. (Don't know if we'd have to make a second lint off you want to pour it back in.)


/// Throws an [UnsupportedError];
/// operations that change the map are disallowed.
set first(_) => _throw();
Copy link
Contributor

@jakemac53 jakemac53 Dec 20, 2024

Choose a reason for hiding this comment

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

Technically this is breaking (statically) to remove these?

We might want to just deprecate them 🤷‍♂️ . But should at least verify we don't know of any usages (I would expect none though since nobody really uses this type directly in APIs).

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm happy to deprecate first and check for any real usage asynchronously.

@devoncarew devoncarew changed the title Update README.md Update package:collection for the new strict_top_level_inference lint Dec 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants