in which inferable outlives-requirements are linted #53013
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rust-highfive
added
the
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
zackmdavis
force-pushed
the
infer_outlints
branch
2 times, most recently
from
fb2cdaf
to
0ba13a5
Compare
|
The job Click to expand the log.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 |
|
(CI failure is tidy/forgetting-to-bless-after-tidy-fix triviality, can fix tonight. Should also test multi-predicate where clauses, for which I expect this initial implementation to not get the span/suggestion right.) |
alexcrichton
reviewed
Aug 3, 2018
src/librustc_lint/builtin.rs
Outdated
| hir::LifetimeName::Static => true, | ||
| _ => false | ||
| }; | ||
| if is_static && !cx.tcx.features().infer_static_outlives_requirements { |
There was a problem hiding this comment.
Should the is_static condition be dropped here as well? Ideally we wouldn't fire lints unless the fix works becaue the crate already has the feature enabled
There was a problem hiding this comment.
infer_static_outlives_requirements (note, not the same as infer_outlives_requirements) is a new feature gate specifically for the special case of static lifetimes that there were concerns about.
I was reluctant to gate the entire lint on infer_outlives_requirements since we're so close to stabilizing it, but I guess it's not very much work (and not much work to undo when we stabilize, even if that's only a few weeks from now) ...
There was a problem hiding this comment.
Ah yeah due to the way cargo fix works it's best to ignore this lint unless the feature is turned on as otherwise lots of false positives get emitted
|
Looks good to me, thanks! I think we'll also want a test to ensure the lint doesn't fire when the feature isn't enabled, but otherwise r=me |
zackmdavis
force-pushed
the
infer_outlints
branch
from
0ba13a5
to
534febb
Compare
|
Update—
It turns out that getting the spans right in the presence of multiple bounds and multi-predicate where clauses is actually pretty involved. I'm pretty close to getting this right in full generality (see all the new code in the UI test), but not quite there yet. (And out of time for today.) |
zackmdavis
force-pushed
the
infer_outlints
branch
from
534febb
to
1e0510c
Compare
|
Well, that was intense, but I think the work here is done!—but unfortunately, the run-rustfix test is blocked on rust-lang/rustfix#141 (which I can also work on). So this should be ready to merge after that gets fixed and either the feature gets stabilized (which @toidiu is going to do) or we add a trivial don't-lint-if-feature-gated conditional here. ETA the day after RustConf? |
|
The job Click to expand the log.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 |
zackmdavis
added a commit
to zackmdavis/rustfix
that referenced
this pull request
Aug 18, 2018
Cargo respects this environment variable (to specify the path to what rustc binary to use), but the Rustfix test suite did not. However, this capability is useful when developing new compiler diagnostics that one wants Rustfix to be able to handle (this being inspired by the endeavor that is rust-lang/rust#53013).
zackmdavis
added a commit
to zackmdavis/rustfix
that referenced
this pull request
Aug 20, 2018
Cargo respects this environment variable (to specify the path to what rustc binary to use), but the Rustfix test suite did not. However, this capability is useful when developing new compiler diagnostics that one wants Rustfix to be able to handle (this being inspired by work on the endeavor that is rust-lang/rust#53013).
|
☔ The latest upstream changes (presumably #53653) made this pull request unmergeable. Please resolve the merge conflicts. |
src/librustc_lint/builtin.rs
Outdated
| // infer-outlives for 'static is still feature-gated (tracking issue #44493) | ||
| continue; | ||
| } | ||
| bound_spans.push((i, bound.span())); |
There was a problem hiding this comment.
So this just removes all outlives requirements -- but I think there are still cases where outlives declarations are required, though they are somewhat obscure. You can find an example in the "where outlives requirements are still required" section of the RFC.
Probably the right way to do this would be to actually run the inferred_outlives_of query and check whether this bound appears in the result. If so, it can be removed.
There was a problem hiding this comment.
Thanks for the pointer! 💖
zackmdavis
force-pushed
the
infer_outlints
branch
from
1e0510c
to
27b2237
Compare
|
@alexcrichton cc @nikomatsakis 🏁 We now actually use the The lint now no-ops if the A new compile-pass UI test with empty expected output verifies that we don't lint the example of a non-inferable outlives-bound from the RFC (whereas previous revisions of this PR did erroneously lint). Some of the UI test examples for which the lint emits multiple spans were split into a separate file because rust-lang/rustfix#141 prevents the suggestions from being verified with the |
|
Nice! This looks generally good to me (good tests, good code/comments/etc), but I'm not looking too much at the particulars as I'm not really overly familiar with them. @nikomatsakis do you think you'll have a chance to review this more closely? If not I can always r+ :) |
|
Ping from triage @nikomatsakis: Some feedback has been requested from you for this PR. |
1 similar comment
|
Ping from triage @nikomatsakis: Some feedback has been requested from you for this PR. |
|
(This is going to need a rebase on #53793 anyway.) |
TimNN
added
S-blocked
zackmdavis
force-pushed
the
infer_outlints
branch
from
27b2237
to
1c346a7
Compare
zackmdavis
removed
the
S-blocked
|
@nikomatsakis @alexcrichton rebased (now that infer-outlives is stablized) and removed the feature gates 🏁 |
TimNN
added
the
S-waiting-on-review
nikomatsakis
approved these changes
Sep 27, 2018
|
|
||
| let ty_lt_names = inferred_outlives.iter().filter_map(|pred| { | ||
| let binder = match pred { | ||
| ty::Predicate::TypeOutlives(binder) => binder, |
There was a problem hiding this comment.
You can also have a scenario like
struct Foo<'a, 'b: 'a> {
x: &'a &'b u32
}It'd be nice to remove those as well!
There was a problem hiding this comment.
Another example (much more obscure) is with projections:
struct Foo<'a, T: Iterator> where T::Item: 'a {
item: T::Item,
}
|
@zackmdavis so sorry for the delay, this slipped my radar for a while :( r=me if you want to land as is, in which case we should file a follow-up issue for the others |
RFC 2093 (tracking issue rust-lang#44493) lets us leave off commonsensically inferable `T: 'a` outlives requirements. (A separate feature-gate was split off for the case of 'static lifetimes, for which questions still remain.) Detecting these was requested as an idioms-2018 lint. It turns out that issuing a correct, autofixable suggestion here is somewhat subtle in the presence of other bounds and generic parameters. Basically, we want to handle these three cases: • One outlives-bound. We want to drop the bound altogether, including the colon— MyStruct<'a, T: 'a> ^^^^ help: remove this bound • An outlives bound first, followed by a trait bound. We want to delete the outlives bound and the following plus sign (and hopefully get the whitespace right, too)— MyStruct<'a, T: 'a + MyTrait> ^^^^^ help: remove this bound • An outlives bound after a trait bound. We want to delete the outlives lifetime and the preceding plus sign— MyStruct<'a, T: MyTrait + 'a> ^^^^^ help: remove this bound This gets (slightly) even more complicated in the case of where clauses, where we want to drop the where clause altogether if there's just the one bound. Hopefully the comments are enough to explain what's going on! A script (in Python, sorry) was used to generate the hopefully-sufficiently-exhaustive UI test input. Some of these are split off into a different file because rust-lang/rustfix#141 (and, causally upstream of that, rust-lang#53934) prevents them from being `run-rustfix`-tested. We also make sure to include a UI test of a case (copied from RFC 2093) where the outlives-bound can't be inferred. Special thanks to Niko Matsakis for pointing out the `inferred_outlives_of` query, rather than blindly stripping outlives requirements as if we weren't a production compiler and didn't care. This concerns rust-lang#52042.
zackmdavis
force-pushed
the
infer_outlints
branch
from
1c346a7
to
032d97f
Compare
|
Rebased to remove the commit adding Filed #54630 for the false-negatives. @bors r=nikomatsakis |
|
📌 Commit 032d97f has been approved by |
bors
added
S-waiting-on-bors
|
⌛ Testing commit 032d97f with merge 2c92d2a043063c566e9141a5317a55f2993fe655... |
|
💔 Test failed - status-travis |
bors
added
S-waiting-on-review
|
The job Click to expand the log.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 |
|
@bors: retry |
bors
added
S-waiting-on-bors
bors
added a commit
that referenced
this pull request
Sep 29, 2018
in which inferable outlives-requirements are linted RFC 2093 (tracking issue #44493) lets us leave off these commonsensically inferable `T: 'a` outlives requirements. (A separate feature-gate was split off for the case of 'static lifetimes, for which questions still remain.) Detecting these was requested as an idioms-2018 lint. Resolves #52042, an item under the fabulous metaïssue #52047. It's plausible that this shouldn't land until after `infer_outlives_requirements` has been stabilized ([final comment period started](#44493 (comment)) 4 days ago), but I think there's also a strong case to not-wait in order to maximize the time that [Edition Preview 2](https://internals.rust-lang.org/t/rust-2018-release-schedule-and-extended-beta/8076) users have to kick at it. (It's allow by default, so there's no impact unless you explicitly turn it or the rust-2018-idioms group up to `warn` or higher.) Questions— * Is `explicit-outlives-requirements` a good name? (I chose it as an [RFC 344](https://github.com/rust-lang/rfcs/blob/master/text/0344-conventions-galore.md#lints)-compliant "inversion" of the feature-gate name, `infer_outlives_requirements`, but I could imagine someone arguing that the word `struct` should be part of the name somewhere, for specificity.) * Are there any false-positives or false-negatives? @nikomatsakis [said that](#52042 (comment)) getting this right would be "fairly hard", which makes me nervous that I'm missing something. The UI test in the initial submission of this pull request just exercises the examples [given in the Edition Guide](https://rust-lang-nursery.github.io/edition-guide/2018/transitioning/ownership-and-lifetimes/struct-inference.html).  r? @alexcrichton
|
☀️ Test successful - status-appveyor, status-travis |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
RFC 2093 (tracking issue #44493) lets us leave off these
commonsensically inferable
T: 'aoutlives requirements. (A separatefeature-gate was split off for the case of 'static lifetimes, for
which questions still remain.) Detecting these was requested as an
idioms-2018 lint.
Resolves #52042, an item under the fabulous metaïssue #52047.
It's plausible that this shouldn't land until after
infer_outlives_requirementshas been stabilized (final comment period started 4 days ago), but I think there's also a strong case to not-wait in order to maximize the time that Edition Preview 2 users have to kick at it. (It's allow by default, so there's no impact unless you explicitly turn it or the rust-2018-idioms group up towarnor higher.)Questions—
Is
explicit-outlives-requirementsa good name? (I chose it as an RFC 344-compliant "inversion" of the feature-gate name,infer_outlives_requirements, but I could imagine someone arguing that the wordstructshould be part of the name somewhere, for specificity.)Are there any false-positives or false-negatives? @nikomatsakis said that getting this right would be "fairly hard", which makes me nervous that I'm missing something. The UI test in the initial submission of this pull request just exercises the examples given in the Edition Guide.
r? @alexcrichton