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

fulfill expectations in check_unsafe_derive_deserialize #12804

Merged
merged 2 commits into from
May 21, 2024

Conversation

B14CK313
Copy link
Contributor

@B14CK313 B14CK313 commented May 15, 2024

The utility function clippy_utils::fulfill_or_allowed is not used because using it would require to move the check for allowed after the check iterating over all inherent impls of the type, doing possibly unnecessary work.
Instead, is_lint_allowed is called as before, but additionally, once certain that the lint should be emitted, span_lint_hir_and_then is called instead of span_lint_and_help to also fulfill expectations.

Note: as this is my first contribution, please feel free to nitpick or request changes. I am happy to adjust the implementation.

fixes: #12802

changelog: fulfill expectations in [unsafe_derive_deserialize]

@rustbot
Copy link
Collaborator

rustbot commented May 15, 2024

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Manishearth (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label May 15, 2024
Copy link
Member

@y21 y21 left a comment

Choose a reason for hiding this comment

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

This should also ideally have a test to make sure that #[expect] now actually works (in tests/ui/unsafe_derive_serialize.rs).

clippy_lints/src/derive.rs Outdated Show resolved Hide resolved
The utility function `clippy_utils::fulfill_or_allowed` is not used because
using it would require to move the check for allowed after the check
iterating over all inherent impls of the type, doing possibly
unnecessary work.
Instead, `is_lint_allowed` is called as before, but additionally, once
certain that the lint should be emitted, `span_lint_hir_and_then` is called
instead of `span_lint_and_help` to also fulfill expectations.

fixes: rust-lang#12802

changelog: fulfill expectations in `check_unsafe_derive_deserialize`
Adding `#![feature(lint_reasons)]` to the top of the file
also changed the line numbers in the expected error output.
@B14CK313 B14CK313 changed the title check if expectation can be fulfilled in check_unsafe_derive_deserialize fulfill expectations in check_unsafe_derive_deserialize May 16, 2024
@Manishearth
Copy link
Member

r? @y21

just got back from my trip and have a large backlog

@rustbot rustbot assigned y21 and unassigned Manishearth May 21, 2024
@y21
Copy link
Member

y21 commented May 21, 2024

Looks good, thanks! @bors r+


If you're interested in a similar followup PR, there's another lint in this file (DERIVE_PARTIAL_EQ_WITHOUT_EQ) that also uses span_lint_and_sugg and has the same problem as this one here, which is that #[expect] has no effect on it (demo). Can r? me in the PR if you want to pick that up. If not I'll try to get to that eventually

@bors
Copy link
Collaborator

bors commented May 21, 2024

📌 Commit 1b6c916 has been approved by y21

It is now in the queue for this repository.

@rustbot

This comment was marked as resolved.

@bors
Copy link
Collaborator

bors commented May 21, 2024

⌛ Testing commit 1b6c916 with merge a5293a2...

bors added a commit that referenced this pull request May 21, 2024
fulfill expectations in `check_unsafe_derive_deserialize`

The utility function `clippy_utils::fulfill_or_allowed` is not used because using it would require to move the check for allowed after the check iterating over all inherent impls of the type, doing possibly unnecessary work.
Instead, `is_lint_allowed` is called as before, but additionally, once certain that the lint should be emitted, `span_lint_hir_and_then` is called instead of `span_lint_and_help` to also fulfill expectations.

Note: as this is my first contribution, please feel free to nitpick or request changes. I am happy to adjust the implementation.

fixes: #12802

changelog [`unsafe_derive_deserialize`]: fulfill expectations in `check_unsafe_derive_deserialize`
@bors
Copy link
Collaborator

bors commented May 21, 2024

💔 Test failed - checks-action_test

@y21
Copy link
Member

y21 commented May 21, 2024

@bors retry

@bors
Copy link
Collaborator

bors commented May 21, 2024

⌛ Testing commit 1b6c916 with merge ea535c9...

@bors
Copy link
Collaborator

bors commented May 21, 2024

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: y21
Pushing ea535c9 to master...

@bors bors merged commit ea535c9 into rust-lang:master May 21, 2024
5 checks passed
bors added a commit that referenced this pull request May 24, 2024
fulfill expectations in `check_partial_eq_without_eq`

This is a followup to #12804, fixing a similar issue for `derive_partial_eq_without_eq` by using `span_lint_hir_and_then` instead of `span_lint_and_sugg`.

Additionally tests for both `#[allow(clippy::derive_partial_eq_without_eq)]` and `#[expect(clippy::derive_partial_eq_without_eq)]` are added.

changelog:[`derive_partial_eq_without_eq`]: fulfill expectations
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

#[expect(...)] attribute not working for clippy::unsafe_derive_deserialize
5 participants