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

Add default_trait_access lint #2844

Merged
merged 3 commits into from Jun 19, 2018

Conversation

Projects
None yet
4 participants
@illicitonion
Copy link
Contributor

commented Jun 14, 2018

No description provided.

@oli-obk

This comment has been minimized.

Copy link
Collaborator

commented Jun 14, 2018

A bunch of other tests are failing, can you have a look?

@mati865

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2018

New lint found warnings in old tests.

@illicitonion illicitonion changed the title Add default_trait_access lint WIP: Add default_trait_access lint Jun 15, 2018

@illicitonion

This comment has been minimized.

Copy link
Contributor Author

commented Jun 15, 2018

Looks like I still have a little more debugging to do around derived Default implementations :)

@illicitonion illicitonion changed the title WIP: Add default_trait_access lint Add default_trait_access lint Jun 18, 2018

@illicitonion

This comment has been minimized.

Copy link
Contributor Author

commented Jun 18, 2018

Ok, this should be good to review now - there's one TODO for which any pointers would be appreciated!

fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) {
if_chain! {
if let ExprCall(ref path, ..) = expr.node;
if !any_parent_is_automatically_derived(cx.tcx, expr.id);

This comment has been minimized.

Copy link
@oli-obk

oli-obk Jun 19, 2018

Collaborator

You found a workaround for the currently broken in_macro! Sweet.

@rust-lang-nursery/clippy do we want to add this to the in_macro check? It's probably expensive to do, but we could monoize it via a thread local HashMap

This comment has been minimized.

Copy link
@illicitonion

illicitonion Jun 19, 2018

Author Contributor

This actually doesn't check for in_macro - I just checked, and this will still warn for things in macros; this is here because the this code: https://github.com/rust-lang/rust/blob/master/src/libsyntax_ext/deriving/default.rs implements #[derive(Default)] by codegenning a call to Default::default which shows up in the AST by the time late checks are happening.

This comment has been minimized.

Copy link
@oli-obk

oli-obk Jun 19, 2018

Collaborator

I should have formulated this better:

in_macro is broken and does not help us anymore in detecting derives. You just found a way to detect derives again, and we should make in_macro also call your function in order to make it work again.

This comment has been minimized.

Copy link
@illicitonion

illicitonion Jun 19, 2018

Author Contributor

Fabulous! :D

@oli-obk oli-obk merged commit 4839c79 into rust-lang:master Jun 19, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@oli-obk

This comment has been minimized.

Copy link
Collaborator

commented Jun 19, 2018

Anyway, that discussion should not be blocking this PR.

@oli-obk

This comment has been minimized.

Copy link
Collaborator

commented Jun 19, 2018

In case you are looking for something to do, you could modify our in_macro function to additionally take a cx and a NodeId, and if the original function body would return false, also run your check.

You'd probably end up touching every single file in clippy_lints, so it is a little bit of effort. The fallout should be a bunch of ui tests suddenly getting fixed :D

@Manishearth

This comment has been minimized.

Copy link
Member

commented Jun 25, 2018

Hey, should this be pedantic? This is a pretty common pattern.

@Manishearth

This comment has been minimized.

Copy link
Member

commented Jun 25, 2018

Discussion moved to #2872

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.