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.

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

Change leak check and suspicious auto trait lint warning messages #120716

Merged
merged 3 commits into from
Feb 20, 2024
Merged

Change leak check and suspicious auto trait lint warning messages #120716

merged 3 commits into from
Feb 20, 2024

Conversation

spastorino
Copy link
Member

@spastorino spastorino commented Feb 6, 2024 •
edited
Loading

The leak check lint message "this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!" is misleading as some cases may not be phased out and could end being accepted. This is under discussion still.

The suspicious auto trait lint the change in behavior already happened, so the new message is probably more accurate.

r? @lcnr

Closes #93367

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 6, 2024
@spastorino spastorino changed the title Change some lint msgs Change leak check and suspicious auto trait lint warning messages Feb 6, 2024
@Jules-Bertholet
Copy link
Contributor

Jules-Bertholet commented Feb 6, 2024 •
edited
Loading

I think the "a future release" wording from the original (vs ambiguous "the future") is valuable. Perhaps even go further, "a future Rust release"?

@spastorino
Copy link
Member Author

I think the "a future release" wording from the original (vs ambiguous "the future") is valuable. Perhaps even go further, "a future Rust release"?

Yes, what you suggest seems also good to me. Would leave a decision up to @lcnr when they do a review.

@bors
Copy link
Contributor

bors commented Feb 8, 2024

☔ The latest upstream changes (presumably #120558) made this pull request unmergeable. Please resolve the merge conflicts.

lcnr
lcnr reviewed Feb 8, 2024
View reviewed changes
compiler/rustc_lint_defs/src/builtin.rs Outdated Show resolved Hide resolved
compiler/rustc_lint_defs/src/builtin.rs Outdated Show resolved Hide resolved
@spastorino spastorino force-pushed the change-some-lint-msgs branch 2 times, most recently from dc9d685 to b306762 Compare February 8, 2024 15:22
@fmease fmease mentioned this pull request Feb 10, 2024
Open
@spastorino spastorino force-pushed the change-some-lint-msgs branch 3 times, most recently from b21d3ce to 29cd7a2 Compare February 14, 2024 21:19
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@lcnr
Copy link
Contributor

lcnr commented Feb 15, 2024

removing the suspicious_auto_trait_impls lint should just accept the code, not always error in these cases.

@rustbot
Copy link
Collaborator

rustbot commented Feb 15, 2024

rust-analyzer is developed in its own repository. If possible, consider making this change to rust-lang/rust-analyzer instead.

cc @rust-lang/rust-analyzer

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@spastorino spastorino force-pushed the change-some-lint-msgs branch 2 times, most recently from 0f5446e to 7bab48c Compare February 15, 2024 13:56
@bors
Copy link
Contributor

bors commented Feb 16, 2024

☔ The latest upstream changes (presumably #120881) made this pull request unmergeable. Please resolve the merge conflicts.

@spastorino spastorino force-pushed the change-some-lint-msgs branch 2 times, most recently from 271485f to af7925f Compare February 17, 2024 01:49
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@spastorino spastorino force-pushed the change-some-lint-msgs branch 2 times, most recently from 35ea987 to aaf24d4 Compare February 17, 2024 02:13
@bors
Copy link
Contributor

bors commented Feb 18, 2024

☔ The latest upstream changes (presumably #121034) made this pull request unmergeable. Please resolve the merge conflicts.

lcnr
lcnr reviewed Feb 19, 2024
View reviewed changes
compiler/rustc_lint_defs/src/builtin.rs
/// were previously unsound, quite subtle, and have been recently modified.
/// This change caused the automatic implementation to be disabled in more
/// cases, potentially breaking some code.
pub SUSPICIOUS_AUTO_TRAIT_IMPLS,
Copy link
Contributor

Choose a reason for hiding this comment

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

there's a "removed_lints" storage somewhere, the lint should be mentioned there, otherwise the error on allow(suspicious_auto_trait_impls) is suboptimal, it's done via LintStore::register_removed.

@lcnr
Copy link
Contributor

lcnr commented Feb 19, 2024 •
edited
Loading

r=me after updating description and final nit. please also add "closes #93367"

@spastorino spastorino force-pushed the change-some-lint-msgs branch 2 times, most recently from c1d94dd to 13575bc Compare February 19, 2024 20:39
@rust-lang rust-lang deleted a comment from rustbot Feb 19, 2024
@spastorino
Copy link
Member Author

@bors r=lcnr

@bors
Copy link
Contributor

bors commented Feb 20, 2024

📌 Commit 4803f17 has been approved by lcnr

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 20, 2024
@Noratrieb Noratrieb mentioned this pull request Feb 20, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 20, 2024
@bors
Auto merge of rust-lang#121327 - Nilstrieb:rollup-zxcwwwy, r=Nilstrieb
5af2130 
Rollup of 10 pull requests

Successful merges:

 - rust-lang#120716 (Change leak check and suspicious auto trait lint warning messages)
 - rust-lang#121195 (unstable-book: Separate testing and production sanitizers)
 - rust-lang#121205 (Merge `CompilerError::CompilationFailed` and `CompilerError::ICE`.)
 - rust-lang#121233 (Move the extra directives for `Mode::CoverageRun` into `iter_header`)
 - rust-lang#121256 (Allow AST and HIR visitors to return `ControlFlow`)
 - rust-lang#121307 (Drive-by `DUMMY_SP` -> `Span` and fmt changes)
 - rust-lang#121308 (Add regression test for rust-lang#103369)
 - rust-lang#121310 (Remove an old hack for rustdoc)
 - rust-lang#121311 (Make `is_nonoverlapping` `#[inline]`)
 - rust-lang#121319 (return `ty::Error` when equating `ty::Error`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 5b8b435 into rust-lang:master Feb 20, 2024
11 checks passed
@rustbot rustbot added this to the 1.78.0 milestone Feb 20, 2024
@bors
Copy link
Contributor

bors commented Feb 20, 2024

⌛ Testing commit 4803f17 with merge 5af2130...

rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 20, 2024
@rust-timer
Unrolled build for rust-lang#120716
cfbc193 
Rollup merge of rust-lang#120716 - spastorino:change-some-lint-msgs, r=lcnr

Change leak check and suspicious auto trait lint warning messages

The leak check lint message "this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!" is misleading as some cases may not be phased out and could end being accepted. This is under discussion still.

The suspicious auto trait lint the change in behavior already happened, so the new message is probably more accurate.

r? `@lcnr`

Closes rust-lang#93367
@lcnr lcnr mentioned this pull request Feb 20, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lcnr lcnr lcnr left review comments

Assignees

@lcnr lcnr

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.78.0
Development

Successfully merging this pull request may close these issues.

Tracking Issue for sound builtin auto trait impls
6 participants
@spastorino @Jules-Bertholet @bors @rust-log-analyzer @lcnr @rustbot