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 lint deprecation for tool lints #4363

Merged
merged 5 commits into from Aug 14, 2019
Merged

Conversation

@phansch
Copy link
Member

@phansch phansch commented Aug 9, 2019

changelog: Allow tool lints (clippy::*) to be deprecated

Our lint deprecation previously didn't work for tool lints, because
register_removed was registering lints to be removed without the
clippy prefix.

Fixes #4349

@phansch
Copy link
Member Author

@phansch phansch commented Aug 9, 2019

Note that the current change means, that this will stop deprecation warnings when using non-tool lints, like #[allow(assign_ops)]. There will just be an unknown_lint warning for non-tool lints.

cc @flip1995

@flip1995
Copy link
Member

@flip1995 flip1995 commented Aug 9, 2019

Note that the current change means, that this will stop deprecation warnings when using non-tool lints

When I implemented the tool lints, I was aware of this problem, but decided to leave it untouched until we heave to deprecate another lint. The reasoning behind that was, that people who still use the deprecated lints, probably won't use tool lints. That reasoning still applies, so the best case would be to allow deprecating both tool lints and non-tool lints. But IIRC that wasn't that easy to implement (otherwise I would have done it (probably 😄)).

I think every maintained crate swapped to tool lints by now and we're on the safe side to make this change?

@phansch
Copy link
Member Author

@phansch phansch commented Aug 9, 2019

allow deprecating both tool lints and non-tool lints.

I haven't tried it, but wouldn't it be enough to register both the tool version and the non-tool version as removed (on the Clippy side)?

@flip1995
Copy link
Member

@flip1995 flip1995 commented Aug 9, 2019

There will just be an unknown_lint warning for non-tool lints.

This warning is not optimal, but I think as long as there is some warning about deprecated lints, this should be fine.

"`Vec::as_mut_slice` has been stabilized in 1.7",
);
store.register_removed(
"str_to_string",
"clippy::str_to_string",
"using `str::to_string` is common even today and specialization will likely happen soon",
Copy link
Member

@flip1995 flip1995 Aug 9, 2019

Choose a reason for hiding this comment

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

"specialization will likely happen soon" 15e55f5 lol 😄

@flip1995
Copy link
Member

@flip1995 flip1995 commented Aug 9, 2019

I haven't tried it, but wouldn't it be enough to register both the tool version and the non-tool version as removed (on the Clippy side)?

We can try it. But that should only be required for lints that were deprecated before tool lints existed. So I would place them in a separate function or write a deprecate_old! macro to do this.

@phansch phansch force-pushed the fix_lint_deprecation branch 2 times, most recently from 5aee384 to 9378def Aug 12, 2019
@phansch
Copy link
Member Author

@phansch phansch commented Aug 12, 2019

@flip1995 should be good to go now
edit: oops, need to fix a method call first

tests/ui/deprecated.rs Outdated Show resolved Hide resolved
@flip1995
Copy link
Member

@flip1995 flip1995 commented Aug 14, 2019

Thanks!

@bors r+

@bors
Copy link
Contributor

@bors bors commented Aug 14, 2019

📌 Commit e406ab5 has been approved by flip1995

bors added a commit that referenced this issue Aug 14, 2019
Update lint deprecation for tool lints

changelog: Allow tool lints (`clippy::*`) to be deprecated

Our lint deprecation previously didn't work for tool lints, because
`register_removed` was registering lints to be removed _without_ the
`clippy` prefix.

Fixes #4349
@bors
Copy link
Contributor

@bors bors commented Aug 14, 2019

Testing commit e406ab5 with merge d37233c...

@bors
Copy link
Contributor

@bors bors commented Aug 14, 2019

💔 Test failed - checks-travis

@matthiaskrgr
Copy link
Member

@matthiaskrgr matthiaskrgr commented Aug 14, 2019

Looks spurious?

   Compiling cargo_metadata v0.8.1

   Compiling clippy_lints v0.0.212 (C:\Users\travis\build\rust-lang\rust-clippy\clippy_lints)

    Finished dev [unoptimized + debuginfo] target(s) in 4m 27s

+++ cargo test --features 'debugging deny-warnings'

error: unable to unlink old fallback exe

info: caused by: Access is denied. (os error 5)

info: backtrace:

stack backtrace:

   0: <no info> (0x72aa68)

   1: <no info> (0x72aeca)

   2: <no info> (0x7275db)

   3: <no info> (0x5511eb)

   4: <no info> (0x5b9049)

   5: <no info> (0x470d5c)

   6: <no info> (0x46d302)

   7: <no info> (0x407bb6)

   8: <no info> (0x473032)

   9: <no info> (0x4013f8)

  10: <no info> (0x40151b)

  11: BaseThreadInitThunk (0x7ffcc8f03034)

@bors retry

@phansch
Copy link
Member Author

@phansch phansch commented Aug 14, 2019

@bors retry yup!

@bors
Copy link
Contributor

@bors bors commented Aug 14, 2019

Testing commit e406ab5 with merge 06430e7...

bors added a commit that referenced this issue Aug 14, 2019
Update lint deprecation for tool lints

changelog: Allow tool lints (`clippy::*`) to be deprecated

Our lint deprecation previously didn't work for tool lints, because
`register_removed` was registering lints to be removed _without_ the
`clippy` prefix.

Fixes #4349
@bors
Copy link
Contributor

@bors bors commented Aug 14, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: flip1995
Pushing 06430e7 to master...

@bors bors merged commit e406ab5 into rust-lang:master Aug 14, 2019
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants