Skip to content

refactor(invalid_upcast_comparisons): move to under operators, simplify #15956

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
Merged
merged 1 commit into from
Oct 28, 2025
Merged

refactor(invalid_upcast_comparisons): move to under operators, simplify #15956

merged 1 commit into from
Oct 28, 2025
+82 −95

Conversation

@ada4a
Copy link
Contributor

@ada4a ada4a commented Oct 26, 2025

The last two commits could arguably be split off into a separate PR -- let me know if that's desired.

changelog: none

@rustbot rustbot added needs-fcp S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Oct 26, 2025
@rustbot
Copy link
Collaborator

rustbot commented Oct 26, 2025

r? @samueltardieu

rustbot has assigned @samueltardieu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@ada4a ada4a removed the needs-fcp label Oct 26, 2025
@github-actions
Copy link

github-actions bot commented Oct 26, 2025 •
edited
Loading

Lintcheck changes for e71b46f

Lint Added Removed Changed
clippy::invalid_upcast_comparisons 0 1 0

This comment will be updated if you push new changes

@ada4a
Copy link
Contributor Author

ada4a commented Oct 26, 2025

Right. I did put the lint under the "not from expansion" branch of operators, and it seems to have removed an FP here -- as $num comes from outside the macro, its type can differ across macro invocations, and so the suggestion of the lint isn't always applicable

samueltardieu
samueltardieu requested changes Oct 27, 2025
View reviewed changes
Copy link
Member

@samueltardieu samueltardieu left a comment •
edited by rustbot
Loading

Choose a reason for hiding this comment

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

It's fine to keep them in the same PR, however given that it won't compile after the first commit and the cumulative changes are small, it would probably be best to squash the commits.

View changes since this review

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Oct 27, 2025
@ada4a ada4a force-pushed the invalid_upcast_comparisons branch from 73ae142 to 7a76c96 Compare October 27, 2025 09:56
@ada4a
Copy link
Contributor Author

ada4a commented Oct 27, 2025 •
edited
Loading

@rustbot ready

Left the non-compiling commit separate as per #15925 (comment)

@ada4a ada4a removed the needs-fcp label Oct 27, 2025
@ada4a
Copy link
Contributor Author

ada4a commented Oct 27, 2025

Off-topic: the fact that the needs-fcp label gets re-added on every force-push, even when there are no new changes to declared_lints.rs, is... unfortunate

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Oct 27, 2025
@samueltardieu
Copy link
Member

Off-topic: the fact that the needs-fcp label gets re-added on every force-push, even when there are no new changes to declared_lints.rs, is... unfortunate

You can also let it, the reviewers will know what to do.

@samueltardieu
Copy link
Member

Left the non-compiling commit separate as per #15925 (comment)

Different reviewers, different preferences I guess. I really dislike broken states. Would you please squash them?

@ada4a
refactor(invalid_upcast_comparisons): move to under operators
e71b46f 
and clean up a bit:
- put the lint functions in the order they're called
- simplify `upcast_comparison_bounds_err`
  - It first handled `Eq`/`Ne`, then one pair of cases with `Lt`/Le`,
    then another pair of cases with `Lt`/`Le` -- turn that into a single
    `match rel`.
  - It used a pattern of `if invert { a } else { b }`, which I found to
    be hard because of the need to keep in mind the implicit `invert &&`
    and `!invert &&` when looking at `a` and `b`. The form
    `(invert && a) || (!invert && b)` should be a bit more explicit,
    while compiling down to the same thing
  - The conditions were already partially written in the order `lb`,
    `norm_rhs_val`, `ub` -- change the ones that weren't
@ada4a ada4a force-pushed the invalid_upcast_comparisons branch from 7a76c96 to e71b46f Compare October 28, 2025 19:36
samueltardieu
samueltardieu approved these changes Oct 28, 2025
View reviewed changes
Copy link
Member

@samueltardieu samueltardieu left a comment •
edited by rustbot
Loading

Choose a reason for hiding this comment

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

Thanks!

View changes since this review

@samueltardieu samueltardieu added this pull request to the merge queue Oct 28, 2025
Merged via the queue into rust-lang:master with commit d74200a Oct 28, 2025
11 checks passed
@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Oct 28, 2025
@ada4a ada4a deleted the invalid_upcast_comparisons branch October 29, 2025 09:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@samueltardieu samueltardieu samueltardieu approved these changes

Assignees

@samueltardieu samueltardieu

Labels

needs-fcp

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ada4a @rustbot @samueltardieu