Skip to content

Don't run allowed lints #75605

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

Don't run allowed lints #75605

wants to merge 3 commits into from

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Aug 16, 2020

Partial implementation of #74704.

Not sure exactly why some lints are still run - I think they're being added later with register_lint_pass? I don't understand the machinery in rustc_lint very well. However this still cuts off a solid percent of the runtime: #74718 (comment) and further improvements can be done in follow-up PRs.

jyn514 added 2 commits August 16, 2020 15:28
@jyn514
Don't run any lints at all ever
2712651 
This is still running some things, not sure why.

```
warning: hidden lifetime parameters in types are deprecated
  --> ui/lint/reasons.rs:21:29
   |
21 |     fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result {
   |                             ^^^^^^^^^^^^^^- help: indicate the anonymous lifetime: `<'_>`
   |
   = note: explicit anonymous lifetimes aid reasoning about ownership
note: the lint level is defined here
  --> ui/lint/reasons.rs:5:9
   |
5  | #![warn(elided_lifetimes_in_paths,
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^
```

However most of the lints have gone away.
@jyn514
Clean up lint pass
11f7dcc
@jyn514 jyn514 added A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. I-compiletime Issue: Problems and improvements with respect to compile times. labels Aug 16, 2020
@rust-highfive
Copy link
Contributor

r? @eddyb

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 16, 2020
@Mark-Simulacrum
Copy link
Member

I worry that such a blanket run-prevention may cause problems, e.g., future compatibility lints should still be shown in dependencies sometimes.

Are we confident none of the lints are actually emitting hard errors? I would feel uncomfortable doing this without an audit (and ideally some machinery to prevent it entirely).

@jyn514
Copy link
Member Author

jyn514 commented Aug 16, 2020

I don't know how to measure the performance improvements for this, AFAIK rustc-perf only runs on the last crate, not on any dependencies. Maybe we could change one of the benchmarks to use --cap-lints allow?

@jyn514
Copy link
Member Author

jyn514 commented Aug 16, 2020

Are we confident none of the lints are actually emitting hard errors? I would feel uncomfortable doing this without an audit (and ideally some machinery to prevent it entirely).

Hmm ... I kind of assumed that lints wouldn't ever emit hard errors, but I'm not sure whether that's actually the case. How would I find out?

@Mark-Simulacrum
Copy link
Member

Well, they probably shouldn't be, but I don't know of a good way to do that. I guess we can try to check the number of errors emitted before/after the pass and ICE if that's different, but that's pretty not great, and is a runtime check so we won't catch interesting cases probably.

We could also try and manually look through all the lint impls but that seems likely to miss things (especially since they're constantly changing).

@jyn514
Copy link
Member Author

jyn514 commented Aug 16, 2020

we can try to check the number of errors emitted before/after the pass and ICE if that's different

I can implement that for now and see if anything pops up.

@bors
Copy link
Collaborator

bors commented Aug 30, 2020

☔ The latest upstream changes (presumably #74862) made this pull request unmergeable. Please resolve the merge conflicts.

@jyn514 jyn514 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 31, 2020
@crlf0710 crlf0710 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 18, 2020
@jyn514
Copy link
Member Author

jyn514 commented Sep 18, 2020

Some places that should have an assert added:

  • fn late_lint_pass_crate
  • fn check_crate (for the per-module lints)
  • fn late_lint_crate (for -Z no-interleave-lints - still not entirely sure why that flag changes so much)
  • fn late_lint_mod
  • fn late_lint_mod_pass

probably others, so it gets early lints as well

@crlf0710 crlf0710 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 8, 2020
@crlf0710
Copy link
Member

crlf0710 commented Oct 8, 2020

Triage: There's merge conflicts now.

@jyn514
Copy link
Member Author

jyn514 commented Oct 8, 2020

The merge conflicts are not where the work is 😆 this needs a lot more work implementing #75605 (comment) before it can even be reviewed.

@jyn514
Copy link
Member Author

jyn514 commented Oct 8, 2020

So I don't lose it: the zulip conversation about this was https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/lint.20avoidance

@Dylan-DPC-zz
Copy link

marking this as draft

@Dylan-DPC-zz Dylan-DPC-zz marked this pull request as draft October 8, 2020 17:41
@jyn514
Copy link
Member Author

jyn514 commented Dec 18, 2020

I'm probably not going to get around to this.

@jyn514 jyn514 closed this Dec 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@eddyb eddyb

Labels
A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. I-compiletime Issue: Problems and improvements with respect to compile times. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@jyn514 @rust-highfive @Mark-Simulacrum @bors @crlf0710 @Dylan-DPC-zz @eddyb