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

More rustfix tests #4408

Merged
merged 8 commits into from Aug 29, 2019
Merged

More rustfix tests #4408

merged 8 commits into from Aug 29, 2019

Conversation

phansch
Copy link
Member

@phansch phansch commented Aug 18, 2019

cc #3630

This is probably easier reviewed per-commit.

changelog: none

@flip1995 flip1995 added the S-waiting-on-review label Aug 18, 2019
@phansch
Copy link
Member Author

phansch commented Aug 19, 2019

Had to run fmt for the rename test, so this should be ready to go. I really should integrate the formatter in my Clippy workflow somehow..

@phansch
Copy link
Member Author

phansch commented Aug 19, 2019

Also added 582048c, which changes the suggestion of float_cmp to HasPlaceholders. The suggestion includes an 'errors' placeholder currently, so it's notMachineApplicable.

tests/ui/int_plus_one.stderr Outdated Show resolved Hide resolved
|
LL | #[allow(dead_code)]
| ^^^^^^^^^^^^^^^^^^^ help: if you just forgot a `!`, use: `#![allow(dead_code)]`
|
= note: `-D clippy::useless-attribute` implied by `-D warnings`

error: useless lint attribute
--> $DIR/useless_attribute.rs:7:1
--> $DIR/useless_attribute.rs:8:1
Copy link
Member

@flip1995 flip1995 Aug 26, 2019

Choose a reason for hiding this comment

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

Does it also emit this suggestion if the attribute is not on top of the file? In that case, the suggestion would be wrong: Playground

Copy link
Member Author

@phansch phansch Aug 28, 2019

Choose a reason for hiding this comment

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

Yes it does :/ The following diff causes an incorrect suggestion:

diff --git a/tests/ui/useless_attribute.rs b/tests/ui/useless_attribute.rs
index c6fc5531..3b708dd9 100644
--- a/tests/ui/useless_attribute.rs
+++ b/tests/ui/useless_attribute.rs
@@ -1,5 +1,6 @@
 // run-rustfix
 // aux-build:proc_macro_derive.rs
+// aux-build:macro_rules.rs
 
 #![warn(clippy::useless_attribute)]
 #![warn(unreachable_pub)]
@@ -46,4 +47,7 @@ mod a {
     pub use self::b::C;
 }
 
+#[allow(dead_code)]
+extern crate macro_rules;
+
 fn main() {}

I wonder if we can detect if there have been any items before the attribute. If yes, then we should not trigger the lint.

Copy link
Member Author

@phansch phansch Aug 28, 2019

Choose a reason for hiding this comment

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

I'm going to try and use the items field of hir::Crate to check and see if the current item being checked is the first one. I'm not sure if that's the most reliable way, though 🤔

Copy link
Member Author

@phansch phansch Aug 28, 2019

Choose a reason for hiding this comment

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

Wait, items is a BTreeMap, so I'll have to check how/if that collection works wrt. sorting order 🤔

extern crate clippy_lints;

#[macro_use]
extern crate proc_macro_derive;
Copy link
Member

@flip1995 flip1995 Aug 26, 2019

Choose a reason for hiding this comment

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

Would it also emit the lint, when you add #[cfg_attr(..)] here?

Copy link
Member Author

@phansch phansch Aug 28, 2019

Choose a reason for hiding this comment

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

Not this particular case, because #[macro_use] with extern crate is whitelisted. It will trigger if it's changed like in the other diff, though. Both cases would be fixed by checking for previous items.

phansch added 7 commits Aug 28, 2019
@flip1995
Copy link
Member

flip1995 commented Aug 28, 2019

Since the useless_attribute lint is buggy, I'd suggest to just change it to MaybeIncorrect and fix it in another PR. Everything else LGTM.

@phansch
Copy link
Member Author

phansch commented Aug 28, 2019

I changed it to MaybeIncorrect and filed #4467 to track the suggestion issue 👍

@flip1995
Copy link
Member

flip1995 commented Aug 29, 2019

Thanks for the ping. I re-reviewed this yesterday, but forgot to approve 😄

@bors r+

@bors
Copy link
Contributor

bors commented Aug 29, 2019

📌 Commit eeeadf3 has been approved by flip1995

bors added a commit that referenced this issue Aug 29, 2019
More rustfix tests

<!--
Thank you for making Clippy better!

We're collecting our changelog from pull request descriptions.
If your PR only updates to the latest nightly, you can leave the
`changelog` entry as `none`. Otherwise, please write a short comment
explaining your change.

If your PR fixes an issue, you can add "fixes #issue_number" into this
PR description. This way the issue will be automatically closed when
your PR is merged.

If you added a new lint, here's a checklist for things that will be
checked during review or continuous integration.

- [ ] Followed [lint naming conventions][lint_naming]
- [ ] Added passing UI tests (including committed `.stderr` file)
- [ ] `cargo test` passes locally
- [ ] Executed `./util/dev update_lints`
- [ ] Added lint documentation
- [ ] Run `./util/dev fmt`

[lint_naming]: https://rust-lang.github.io/rfcs/0344-conventions-galore.html#lints

Note that you can skip the above if you are just opening a WIP PR in
order to get feedback.

Delete this line and everything above before opening your PR -->

cc #3630

This is probably easier reviewed per-commit.

changelog: none
@bors
Copy link
Contributor

bors commented Aug 29, 2019

Testing commit eeeadf3 with merge 888b736...

@bors
Copy link
Contributor

bors commented Aug 29, 2019

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

@bors bors merged commit eeeadf3 into rust-lang:master Aug 29, 2019
2 checks passed
@phansch phansch deleted the more_rustfix_tests branch Aug 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants