-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Implement 'Re-implementing PartialEq::ne
' lint
#1307
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clippy_dump is around a week old ;) but you're right, it should get documented. Did it help you at all in this case?
I'll look into the compiletest issues, we should simply fix that, it has annoyed me before
PARTIALEQ_NE_IMPL, | ||
impl_item.span, | ||
"Re-implementing `PartialEq::ne`") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should also give a help message telling the user what to do( remove the impl)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any suggestion for wording? Maybe just appending ... is unnecessary
?
@@ -1,5 +1,5 @@ | |||
#!/bin/sh | |||
rm -rf target*/*so | |||
cargo build --lib && cp -R target target_recur && cargo rustc -- -Zextra-plugins=clippy -Ltarget_recur/debug -Dclippy_pedantic -Dclippy || exit 1 | |||
cargo build --lib && cp -R target target_recur && cargo rustc --lib -- -Zextra-plugins=clippy -Ltarget_recur/debug -Dclippy_pedantic -Dclippy || exit 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh, how did this ever work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh.... This is dogfood.sh, nevermind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, seems like nobody's run this ever since cargo-clippy
appeared 😄 .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cargo test will actually run a separate implementation of dogfood, so there wasn't any need for that script as far as I knew
MetaItemKind::Word(ref word) => word == "clippy_dump", | ||
_ => false, | ||
}) | ||
attr::contains_name(attrs, "clippy_dump") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neat cleanup, didn't know about this function
The other PR conflicted with this one. You need to rebase/merge and rerun util/update_lints.sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clippy_dump is around a week old ;) but you're right, it should get documented. Did it help you at all in this case?
Oh right, I tried it on the impl and got the helpful output
item ``
visibility inherited from outer item
error: internal compiler error: ../src/librustc/hir/map/mod.rs:356: local_def_id: no entry for `9`, which has a map of `Some(NotPresent)`
:D . I've added a commit to fix the error.
PARTIALEQ_NE_IMPL, | ||
impl_item.span, | ||
"Re-implementing `PartialEq::ne`") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any suggestion for wording? Maybe just appending ... is unnecessary
?
@@ -1,5 +1,5 @@ | |||
#!/bin/sh | |||
rm -rf target*/*so | |||
cargo build --lib && cp -R target target_recur && cargo rustc -- -Zextra-plugins=clippy -Ltarget_recur/debug -Dclippy_pedantic -Dclippy || exit 1 | |||
cargo build --lib && cp -R target target_recur && cargo rustc --lib -- -Zextra-plugins=clippy -Ltarget_recur/debug -Dclippy_pedantic -Dclippy || exit 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, seems like nobody's run this ever since cargo-clippy
appeared 😄 .
Sgtm |
Ok, changed and squashed. |
Travis says you forgot to run util/update_lints.py |
Fixed |
Thanks! Great lint and nice cleanups |
Closes #86
@oli-obk I didn't track the time for implementing this, but most of it was probably spent on figuring out why
compiletest
kept saying0 unexpected errors found, 1 expected errors not found
. In the end, I copied and ran therustc
cmdline for that file separately, which showed that there was indeed an unexpected and very simple error:main function not found
:| . Apparentlycompiletest
may swallow unexpected errors.So perhaps
CONTRIBUTING.md
should include some hints for debugging lints, including mentioning theTESTNAME
env var and#[clippy_dump]
.