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 macro checks to lints #48855

Closed
Manishearth opened this issue Mar 8, 2018 · 12 comments
Closed

Add macro checks to lints #48855

Manishearth opened this issue Mar 8, 2018 · 12 comments
Assignees
Labels
A-edition-2018-lints Area: Lints supporting the 2018 edition A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. C-enhancement Category: An issue proposing an enhancement or a PR with one. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Manishearth
Copy link
Member

From #48385

Many of our lints trigger even when the code is within a macro defined by an external crate. They should include macro checks.

Clippy has a function for this already.

We should include this in librustc::lint somewhere, and use it for various lints. It should not be used for future compat lints.

One possible thing to do here is to work it into the lint framework itself, so that non-future-compat lints which are triggered on macro spans will not actually be emitted.

We should still have the helper function since some lints may involve multiple important spans so they need the ability to check them all.

Ideally there would be a way to turn this off, though -- in case you want to find issues in your macros. It's a tricky problem.

Willing to mentor the basic issue of importing the macro checks and applying them in most lints (or working it into the framework).

@Manishearth Manishearth added A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. labels Mar 8, 2018
@Manishearth
Copy link
Member Author

@Dylan-DPC would you like to work on this?

@Manishearth Manishearth added the E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. label Mar 8, 2018
@oli-obk
Copy link
Contributor

oli-obk commented Mar 9, 2018

I'm not sure, but @flip1995 might be already on it (see #48364)

@Manishearth
Copy link
Member Author

So that's focusing on span_suggestion -- we need to both fix that and fix the general lint case, except for future compat lints

@oli-obk
Copy link
Contributor

oli-obk commented Mar 9, 2018

In clippy we are doing this on a case-by-case basis. Do we really want to blanket ignore all lints in local expansions of external macros?

@Manishearth
Copy link
Member Author

Honestly I don't know. Future compat lints must show, but aside from that the cases where we want to still show a lint on a macro expansion is when the linted code is completely within the invocation, which can't be detected easily right now.

@Dylan-DPC-zz
Copy link

@Manishearth Ye sure 👍

@Manishearth
Copy link
Member Author

Any updates?

@Dylan-DPC-zz
Copy link

@Manishearth haven't started with it. Will work on it this weekend.

@Dylan-DPC-zz
Copy link

@Manishearth is there a common place where all lints are used? or i have to hunt down each lint implementation?

@Manishearth
Copy link
Member Author

they all implement EarlyLintPass or LateLintPass. The implementations are scattered.

What you want to do is tweak span_lint and similar functions to do the macro check, except for when the lint is a future compat lint (ignore this part for now)

@XAMPPRocky XAMPPRocky added C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 14, 2018
@nrc nrc added the A-edition-2018-lints Area: Lints supporting the 2018 edition label Jul 2, 2018
@nrc
Copy link
Member

nrc commented Jul 2, 2018

From @cramertj on Discord: problematic interaction with #[derive(fail)]

warning: trait objects without an explicit `dyn` are deprecated
   --> src/lib.rs:144:25
    |
144 |         #[derive(Debug, Fail)]
    |                         ^^^^ help: use `dyn`: `dyn (Fail)`

@alexcrichton
Copy link
Member

I've revived #49755 at #52467, and I've confirmed @nrc that the warning with derive(Fail) no longer happens after that PR

@alexcrichton alexcrichton self-assigned this Jul 17, 2018
bors added a commit that referenced this issue Jul 20, 2018
Squash all lints tied to foreign macros by default

This PR is a continuation of #49755 (thanks for the initial jump-start @Dylan-DPC!) and is targeted at solving #48855. This change updates the lint infrastructure to, by default, ignore all lints emitted for code that originates in a foreign macro. For example if `println!("...")` injects some idiomatic warnings these are all ignored by default. The rationale here is that for almost all lints there's no action that can be taken if the code originates from a foreign lint.

Closes #48855
Closes #52483
Closes #52479
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-edition-2018-lints Area: Lints supporting the 2018 edition A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. C-enhancement Category: An issue proposing an enhancement or a PR with one. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

6 participants