Skip to content

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

Merged
merged 4 commits into from
Oct 31, 2016

Conversation

Kha
Copy link
Contributor

@Kha Kha commented Oct 30, 2016

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 saying 0 unexpected errors found, 1 expected errors not found. In the end, I copied and ran the rustc cmdline for that file separately, which showed that there was indeed an unexpected and very simple error: main function not found :| . Apparently compiletest may swallow unexpected errors.

So perhaps CONTRIBUTING.md should include some hints for debugging lints, including mentioning the TESTNAME env var and #[clippy_dump].

Copy link
Contributor

@oli-obk oli-obk left a 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`")
}
Copy link
Contributor

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)

Copy link
Contributor Author

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
Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Contributor Author

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 😄 .

Copy link
Contributor

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")
Copy link
Contributor

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

@oli-obk
Copy link
Contributor

oli-obk commented Oct 30, 2016

The other PR conflicted with this one. You need to rebase/merge and rerun util/update_lints.sh

Copy link
Contributor Author

@Kha Kha left a 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`")
}
Copy link
Contributor Author

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
Copy link
Contributor Author

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 😄 .

@oli-obk
Copy link
Contributor

oli-obk commented Oct 30, 2016

Maybe just appending ... is unnecessary?

Sgtm

@Kha
Copy link
Contributor Author

Kha commented Oct 30, 2016

Maybe just appending ... is unnecessary?

Sgtm

Ok, changed and squashed.

@oli-obk
Copy link
Contributor

oli-obk commented Oct 30, 2016

Travis says you forgot to run util/update_lints.py

@Kha
Copy link
Contributor Author

Kha commented Oct 30, 2016

Fixed

@oli-obk oli-obk merged commit 053896b into rust-lang:master Oct 31, 2016
@oli-obk
Copy link
Contributor

oli-obk commented Oct 31, 2016

Thanks! Great lint and nice cleanups

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants