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 deny(unused_lifetimes) to all the crates that have deny(internal). #61735

Merged
merged 12 commits into from
Jun 11, 2019

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Jun 11, 2019

@Zoxc brought up, regarding #61722, that we don't force the removal of unused lifetimes.
Turns out that it's not that bad to enable for compiler crates (I wonder why it's not warn by default?).

I would've liked to enable single_use_lifetimes as well, but #53738 makes it unusable for now.

For the rustfmt commit, I used rust-lang/rustfmt#1324 (comment), and manually filtered out some noise.

r? @oli-obk cc @rust-lang/compiler

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 11, 2019
@@ -3,6 +3,8 @@
/// Basically a workaround; see [this comment] for details.
///
/// [this comment]: https://github.com/rust-lang/rust/issues/34511#issuecomment-373423999
// FIXME(eddyb) false positive, the lifetime parameter is "phantom" but needed.
#[allow(unused_lifetimes)]
pub trait Captures<'a> { }
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 really be in core::marker

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather impl Trait not need it at all.

@oli-obk
Copy link
Contributor

oli-obk commented Jun 11, 2019

I wonder why it's not warn by default?

because of the false positives you encountered. Rustc lints have a very high bar.

@bors r+

@bors
Copy link
Contributor

bors commented Jun 11, 2019

📌 Commit 765fd84ddb3f6d9288a4114f4f5de730986064db has been approved by oli-obk

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 11, 2019
@@ -13,6 +13,7 @@

#![deny(rust_2018_idioms)]
#![deny(internal)]
#![deny(unused_lifetimes)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both deny(internal) and deny(unused_lifetimes) can be enabled through rustbuild (src\bootstrap\bin\rustc.rs) rather than through all crates individually, similarly to deny(warnings)/deny(bare_trait_objects).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point. cc @flip1995

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did this locally and it revealed rustc_data_structures and rustc_macros were missing deny(internal) and deny(unused_lifetimes).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like default_hash_types may be a problem.
Unlike other lints it's not universally applicable and is not a no-op for non-compiler crates.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's even worse, the rustbuild thing seems to apply to all crates, not just compiler ones.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the rustbuild thing seems to apply to all crates

I expected that, but also expected internal lints to not report anything for non-compiler crates (since they don't have TyCtxt etc).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah default_hash_types is also explicitly allowed in libstd for example.

Most of the internal lints shouldn't report anything in most of the crates. I will figure this out in #61545

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason why these are deny instead of warn? This means these are hard errors even with deny-warnings = false.

@eddyb
Copy link
Member Author

eddyb commented Jun 11, 2019

@bors rollup=never p=2

@bors

This comment has been minimized.

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 11, 2019
@rust-highfive

This comment has been minimized.

@eddyb
Copy link
Member Author

eddyb commented Jun 11, 2019

@petrochenkov @oli-obk I've backed out the rustbuild changes, everything seemed fine but changing src/bootstrap/bin/rustc.rs doesn't trigger a rebuild. Turns out core, std etc. would be affected too.

@bors r=oli-obk

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 11, 2019
@rust-highfive
Copy link
Collaborator

Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
The command "curl -fo /home/travis/stamp https://s3-us-west-1.amazonaws.com/rust-lang-ci2/rust-ci-mirror/2017-03-17-stamp-x86_64-unknown-linux-musl" failed. Retrying, 3 of 3.
curl: (56) GnuTLS recv error (-54): Error in the pull function.
The command "curl -fo /home/travis/stamp https://s3-us-west-1.amazonaws.com/rust-lang-ci2/rust-ci-mirror/2017-03-17-stamp-x86_64-unknown-linux-musl" failed 3 times.
travis_time:end:008aebd8:start=1560266168582463854,finish=1560267292706318734,duration=1124123854880
The command "case "$TRAVIS_OS_NAME" in linux) travis_retry curl -fo $HOME/stamp https://s3-us-west-1.amazonaws.com/rust-lang-ci2/rust-ci-mirror/2017-03-17-stamp-x86_64-unknown-linux-musl && chmod +x $HOME/stamp && export PATH=$PATH:$HOME ;; osx) if [[ "$SCRIPT" == "./x.py dist" ]]; then travis_retry brew update && travis_retry brew install xz && travis_retry brew install swig; fi && travis_retry curl -fo /usr/local/bin/sccache https://s3-us-west-1.amazonaws.com/rust-lang-ci2/rust-ci-mirror/2018-04-02-sccache-x86_64-apple-darwin && chmod +x /usr/local/bin/sccache && travis_retry curl -fo /usr/local/bin/stamp https://s3-us-west-1.amazonaws.com/rust-lang-ci2/rust-ci-mirror/2017-03-17-stamp-x86_64-apple-darwin && chmod +x /usr/local/bin/stamp && travis_retry curl -f http://releases.llvm.org/7.0.0/clang+llvm-7.0.0-x86_64-apple-darwin.tar.xz | tar xJf - && export CC=`pwd`/clang+llvm-7.0.0-x86_64-apple-darwin/bin/clang && export CXX=`pwd`/clang+llvm-7.0.0-x86_64-apple-darwin/bin/clang++ && export AR=ar ;; esac" failed and exited with 56 during .
Your build has been stopped.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@eddyb
Copy link
Member Author

eddyb commented Jun 11, 2019

@bors retry (network error?)

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 11, 2019
@bors
Copy link
Contributor

bors commented Jun 11, 2019

⌛ Testing commit 1d720ec with merge 5f3656c...

bors added a commit that referenced this pull request Jun 11, 2019
Add deny(unused_lifetimes) to all the crates that have deny(internal).

@Zoxc brought up, regarding #61722, that we don't force the removal of unused lifetimes.
Turns out that it's not that bad to enable for compiler crates (I wonder why it's not `warn` by default?).

I would've liked to enable `single_use_lifetimes` as well, but #53738 makes it unusable for now.

For the `rustfmt` commit, I used rust-lang/rustfmt#1324 (comment), and manually filtered out some noise.

r? @oli-obk cc @rust-lang/compiler
@bors
Copy link
Contributor

bors commented Jun 11, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: oli-obk
Pushing 5f3656c to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 11, 2019
@bors bors merged commit 1d720ec into rust-lang:master Jun 11, 2019
@rust-highfive
Copy link
Collaborator

📣 Toolstate changed by #61735!

Tested on commit 5f3656c.
Direct link to PR: #61735

💔 clippy-driver on windows: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk @phansch, @rust-lang/infra).
💔 clippy-driver on linux: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk @phansch, @rust-lang/infra).

rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request Jun 11, 2019
Tested on commit rust-lang/rust@5f3656c.
Direct link to PR: <rust-lang/rust#61735>

💔 clippy-driver on windows: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk @phansch, @rust-lang/infra).
💔 clippy-driver on linux: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk @phansch, @rust-lang/infra).
@eddyb eddyb deleted the must-use-life branch June 12, 2019 03:36
bors added a commit to rust-lang/rust-clippy that referenced this pull request Jun 12, 2019
Remove wrong lifetime from LintContext

Rustup rust-lang/rust#61735

changelog: none
bors added a commit to rust-lang/rust-clippy that referenced this pull request Jun 12, 2019
Remove wrong lifetime from LintContext

Rustup rust-lang/rust#61735

changelog: none
bors added a commit that referenced this pull request Jun 12, 2019
rustc: replace `TyCtxt<'a, 'gcx, 'tcx>` with `TyCtxt<'gcx, 'tcx>`.

This first lifetime parameter of `TyCtxt` has been phantom for a while, thanks to @Zoxc, but was never removed, and I'm doing this now in preparation for removing the `'gcx`/`'tcx` split.

I wasn't going to do this as a separate step, and instead start converting uses of `TyCtxt` to a single-lifetime alias of it (e.g. `type TyCx<'tcx> = TyCtxt<'tcx, 'tcx, 'tcx>;`) but it turns out (as @Zoxc rightly predicted) that there is far more fallout from not needing a lifetime for the first parameter of `TyCtxt`.

That is, going from `TyCtxt<'a, 'gcx, 'tcx>` to `TyCtxt<'tcx, 'gcx, 'tcx>` (the first commit in this PR) has the largest amount of fallout out of all the changes we might make (because it can require removing the `'a` parameter of `struct`s containing `tcx: TyCtxt<'a, ...>`), and is the hardest to automate (because `'a` is used everywhere, not just with `TyCtxt`, unlike, say `'gcx, 'tcx` -> `'tcx`).

So I'm submitting this now to get it out of the way and reduce further friction in the future.

**EDIT**: for the `rustfmt` commit, I used rust-lang/rustfmt#1324 (comment), and manually filtered out some noise, like in #61735, but unlike that PR, there was also a weird bug to work around.
It should be reviewed separately, and dropped if unwanted.

cc @rust-lang/compiler r? @nikomatsakis
bors added a commit that referenced this pull request Jun 14, 2019
Unify all uses of 'gcx and 'tcx.

This is made possible by @Zoxc landing #57214 (see #57214 (comment) for the decision).

A bit of context for the approach: just like #61722, this is *not* how I originally intended to go about this, but @Zoxc and my own experimentation independently resulted in the same conclusion:
The interim alias `type TyCx<'tcx> = TyCtxt<'tcx, 'tcx>;` attempt required more work (adding `use`s), even only for handling the `TyCtxt<'tcx, 'tcx>` case and not the general `TyCtxt<'gcx, 'tcx>` one.

What this PR is based on is the realization that `'gcx` is a special-enough name that it can be replaced, without caring for context, with `'tcx`, and then repetitions of the name `'tcx` be compacted away.
After that, only a small number of error categories remained, each category easily dealt with with either more mass replacements (e.g. `TyCtxt<'tcx, '_>` -> `TyCtxt<'tcx>`) or by hand.

For the `rustfmt` commit, I used rust-lang/rustfmt#1324 (comment), and manually filtered out some noise, like in #61735 and #61722, and like the latter, there was also a weird bug to work around.
It should be reviewed separately, and dropped if unwanted (in this PR it's pretty significant).

cc @rust-lang/compiler r? @nikomatsakis
Centril added a commit to Centril/rust that referenced this pull request Jul 5, 2019
Implement another internal lints

cc rust-lang#49509

This adds ~~two~~ one internal lint~~s~~:
1. LINT_PASS_IMPL_WITHOUT_MACRO: Make sure, that the `{declare,impl}_lint_pass` macro is used to implement lint passes. cc rust-lang#59669
2. ~~USAGE_OF_TYCTXT_AND_SPAN_ARGS: item 2 on the list in rust-lang#49509~~

~~With 2. I wasn't sure, if this lint should be applied everywhere. That means a careful review of 0955835 would be great. Also 73fb9b4 allows this lint on some functions. Should I also apply this lint there?~~

TODO (not directly relevant for review):
- [ ] rust-lang#59316 (comment) (not sure yet, if this works or how to query for `rustc_private`, since it's not in [`Features`](https://doc.rust-lang.org/nightly/nightly-rustc/syntax/feature_gate/struct.Features.html) 🤔 cc @eddyb)
- [x] rust-lang#61735 (comment)
- [x] Check explicitly for the `{declare,impl}_lint_pass!` macros

r? @oli-obk
Centril added a commit to Centril/rust that referenced this pull request Jul 5, 2019
Implement another internal lints

cc rust-lang#49509

This adds ~~two~~ one internal lint~~s~~:
1. LINT_PASS_IMPL_WITHOUT_MACRO: Make sure, that the `{declare,impl}_lint_pass` macro is used to implement lint passes. cc rust-lang#59669
2. ~~USAGE_OF_TYCTXT_AND_SPAN_ARGS: item 2 on the list in rust-lang#49509~~

~~With 2. I wasn't sure, if this lint should be applied everywhere. That means a careful review of 0955835 would be great. Also 73fb9b4 allows this lint on some functions. Should I also apply this lint there?~~

TODO (not directly relevant for review):
- [ ] rust-lang#59316 (comment) (not sure yet, if this works or how to query for `rustc_private`, since it's not in [`Features`](https://doc.rust-lang.org/nightly/nightly-rustc/syntax/feature_gate/struct.Features.html) 🤔 cc @eddyb)
- [x] rust-lang#61735 (comment)
- [x] Check explicitly for the `{declare,impl}_lint_pass!` macros

r? @oli-obk
Centril added a commit to Centril/rust that referenced this pull request Jul 5, 2019
Implement another internal lints

cc rust-lang#49509

This adds ~~two~~ one internal lint~~s~~:
1. LINT_PASS_IMPL_WITHOUT_MACRO: Make sure, that the `{declare,impl}_lint_pass` macro is used to implement lint passes. cc rust-lang#59669
2. ~~USAGE_OF_TYCTXT_AND_SPAN_ARGS: item 2 on the list in rust-lang#49509~~

~~With 2. I wasn't sure, if this lint should be applied everywhere. That means a careful review of 0955835 would be great. Also 73fb9b4 allows this lint on some functions. Should I also apply this lint there?~~

TODO (not directly relevant for review):
- [ ] rust-lang#59316 (comment) (not sure yet, if this works or how to query for `rustc_private`, since it's not in [`Features`](https://doc.rust-lang.org/nightly/nightly-rustc/syntax/feature_gate/struct.Features.html) 🤔 cc @eddyb)
- [x] rust-lang#61735 (comment)
- [x] Check explicitly for the `{declare,impl}_lint_pass!` macros

r? @oli-obk
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants