Skip to content

Lint unrelated_type_equality_checks no longer recognizes Int64, Int32. #58939

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

Closed
lrhn opened this issue Nov 30, 2022 · 4 comments · Fixed by dart-archive/linter#3869
Closed
Labels
devexp-linter Issues with the analyzer's support for the linter package legacy-area-analyzer Use area-devexp instead. linter-false-positive linter-set-core P1 A high priority bug; for example, a single project is unusable or has many test failures

Comments

@lrhn
Copy link
Member

lrhn commented Nov 30, 2022

The lint unrelated_type_equality_checks has special-casing recognizing Int64 and Int32 from package:fixnum, to allow them to be compared to integers (which are otherwise an unrelated type).

This recognition fails after a refactoring of package:fixnum, which moved the classes into individual libraries, instead of using part files.

To Reproduce
Use tip-of-tree code from https://github.com/dart-lang/fixnum

Try to do if (FixNum(1) == 1) ... with the lint enabled. This used to not give a warning. Now it does.

Expected behavior

The Int64 and Int32 classes should still be recognized and special cased.

Additional context

The relevant code is:

bool _isFixNumIntX(DartType type) {
  if (type is! InterfaceType) {
    return false;
  }
  Element element = type.element;
  return (element.name == 'Int32' || element.name == 'Int64') &&
      element.library?.name == 'fixnum';
}

This approach depends on the package not changing, which is a fragile assumption to have for a separate package.

Consider instead checking whether the the classes are named Int32 or Int64, and the path to the library starts with package:fixnum/. That should cover most potential future refactorings of the package as well, because all it checks for is a class with one of those names provided by package:fixnum, without assuming anything about the internal structure of the package.

(That will make the lint harder to test, since you cannot just create a library with name fixnum and an Int64 type. Maybe retain the library name check as well, or change it to .name == 'fixnum_for_testing', so you can still trigger the lint without depending on package:fixnum. On the other hand, depending on the real package:fixnum for testing, as a dev-dependency, might help detecting if the lint breaks again.)

@pq
Copy link
Member

pq commented Nov 30, 2022

Oh! This is a great heads-up. When is the refactored package scheduled to be released? If it's before Dart 3.0, we should hustle to get a fix in today (or cherry-picked) for 2.19.

@pq pq added P1 A high priority bug; for example, a single project is unusable or has many test failures linter-false-positive labels Nov 30, 2022
@pq
Copy link
Member

pq commented Nov 30, 2022

Ah. Chatting with @devoncarew, it sounds like this issue is blocking a roll of fixnum into Google3 too so yeah, we'd better get it addressed.

@devoncarew
Copy link
Member

Yup, from https://github.com/dart-lang/sdk/blob/main/DEPS#L132, the roll issue is b/260609190.

copybara-service bot referenced this issue Nov 30, 2022
See: https://github.com/dart-lang/linter/issues/3868

(This should also unblock the fixnum internal roll.)

Change-Id: I9af710b075c6280ee55153c559f91ac6a0b21f3d
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/273007
Commit-Queue: Phil Quitslund <[email protected]>
Reviewed-by: Samuel Rawlins <[email protected]>
@lrhn
Copy link
Member Author

lrhn commented Dec 2, 2022

Solution looks good. Should work even if we end up doing different implementations for different platforms, so there'll be more than one Int64 class defined. They'll all be detected by this.

copybara-service bot referenced this issue Dec 12, 2022
* updates `unrelated_type_equality_checks` for `pkg:fixnum` restructuring
* updates `prefer_equal_for_default_values` to not report for SDKs >=2.19, where this lint is now an analyzer diagnostic.

(Note that these changes landed *before* the release cut-off. 😬)

Cherry-pick: https://dart-review.googlesource.com/c/sdk/+/274481
Fixes: https://github.com/dart/sdk/issues/56781234


---



pull `unrelated_type_equality_checks` fix

See: https://github.com/dart-lang/linter/issues/3868

(This should also unblock the fixnum internal roll.)

Change-Id: I9af710b075c6280ee55153c559f91ac6a0b21f3d


linter 1.31.0

Cherry-picks update of `prefer_equal_for_default_values` to be a no-op in SDKs `>=2.19`.

See: dart-archive/linter#3865

Change-Id: I2669f0279a0913bc8b3c5b0501a9869608883a14
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/274481
Reviewed-by: Brian Wilkerson <[email protected]>
Commit-Queue: Phil Quitslund <[email protected]>
@devoncarew devoncarew added devexp-linter Issues with the analyzer's support for the linter package legacy-area-analyzer Use area-devexp instead. labels Nov 19, 2024
@devoncarew devoncarew transferred this issue from dart-archive/linter Nov 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
devexp-linter Issues with the analyzer's support for the linter package legacy-area-analyzer Use area-devexp instead. linter-false-positive linter-set-core P1 A high priority bug; for example, a single project is unusable or has many test failures
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants