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

Tracking issue for RFC 2093: Infer T: 'x outlives requirements on structs #44493

Closed
2 of 3 tasks
aturon opened this issue Sep 11, 2017 · 37 comments
Closed
2 of 3 tasks

Tracking issue for RFC 2093: Infer T: 'x outlives requirements on structs #44493

aturon opened this issue Sep 11, 2017 · 37 comments
Labels
B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. 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. finished-final-comment-period The final comment period is finished for this PR / Issue. P-high High priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Milestone

Comments

@aturon
Copy link
Member

aturon commented Sep 11, 2017

This is a tracking issue for the RFC "Infer T: 'x outlives requirements on structs " (rust-lang/rfcs#2093).

Current status:

Implemented and we have decided to stabilize. We still need someone to make the stabilization PR! Mentoring instructions here:

https://github.com/rust-lang/rust/issue_comments#issuecomment-411781121

Until then, you can use this by adding #![feature(infer_outlives_requirements)] to your crate.

Steps:

Unresolved questions:

The interaction with 'static remains a bit unclear. For example, do we want to infer a T: 'static requirement on the type parameter T here? The code currently will do so, arguably leading to confusing errors.

#![feature(infer_outlives_requirements)]

struct Foo<T> { t: &'static T }

fn main() {
    let x = 3;
    let _ = Foo { t: &x };
}
@aturon aturon added B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Sep 11, 2017
@nikomatsakis nikomatsakis added E-needs-mentor WG-traits Working group: Traits, https://internals.rust-lang.org/t/announcing-traits-working-group/6804 labels Sep 15, 2017
@nikomatsakis nikomatsakis added this to the impl period milestone Sep 15, 2017
@nikomatsakis nikomatsakis added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-compiler-middle and removed WG-traits Working group: Traits, https://internals.rust-lang.org/t/announcing-traits-working-group/6804 labels Sep 15, 2017
@aturon aturon removed this from the impl period milestone Sep 15, 2017
@TimNN TimNN added the C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. label Sep 17, 2017
@nikomatsakis
Copy link
Contributor

Mentoring instructions

In the compiler, when we want to know what predicates are defined on something (e.g., a struct), we do that via the predicates_of query. Right now, this is implemented by this code in librusc_typeck. This code basically reads the HIR and produces exactly the results found there.

In terms of how to fit this inference into the compiler pipeline, then, I think we want to ensure that predicates_of includes these inferred predicates. To do that, we can redefine predicates_of in terms of two new queries, explicit_predicates_of and inferred_outlives_of -- basically, predicates_of would be the union of the two:

predicates_of(D) = explicit_predicates_of(D) + inferred_outlives_of(D)

explicit_predicates_of would be exactly the same as predicates_of is today (and only defined for local crates). It just returns the predicates the user typed. inferred_outlives_of, in contrast, will include the predicates that we are going to infer.

The very first PR, then, could be to introduce the explicit_predicates_of query and to make predicates_of just redirect to it (later, we will add inferred_outlives_of). This makes sense because we're going to need explicit_predicates_of when defining inferred_outlives_of.

Now we have to figure out how to implemented inferred_outlives_of. I think the basic structure here is going to be similar to how variance inference is setup. In terms of queries, variance inference is defined by two queries:

  • crate_variances -- returns a map containing the variance for every item in the crate
  • variances_of -- just selects an item of this map.

The crate_variances query is intended as an implementation detail of variance -- end-users should use variances_of for a particular item.

The reason for this particular setup is that we can't infer the variances for a single item in isolation; the variance for an item X depends on the contents of the struct, and there may be cycles. Since cycles are generally forbidden between queries, we instead compute the variances for ALL structs in the crate, and then use individual queries to extract the result.

So, for inferred_outlives_of, we will basically create a new query, probably living alongside variance in librustc_typeck, let's call it inferred_outlives_of. This query will defer to a crate-wide query inferred_outlives_crate (we should probably have named crate_variances as variances_crate...). Since we only infer predicates for struts, inferred_outlives_of can just return the empty set for any def-id that is not a struct.

This crate-wide computation will work much as it is described in the RFC -- there will be a set of constraints being inferred for each struct S, what the RFC calls A[S]. These sets will be sets of Predicate<'tcx> values. We will initialize them by filtering the explicit_predicates_of each struct to only include those of type TypeOutlives or RegionOutlives.

Next, we can walk the types of the fields declared in each struct. These can be obtained by invoking type_of(def_id) where def_id is the DefId of the struct. This should give back a type of the TyAdt variant; from this, you can extract the types of the fields. Something like this code from variance should give you the idea. For each field that references another struct S2, we need to create a link between those types and ensure that their implied predicates are a superset.

Once this is done, we should have some kind of inferred set of obligations for each struct. I think it'd be good to setup a unit-testing mechanism for this similar to the one we use for variance. Basically, some custom code that looks for a #[rustc_inferred_outlives] annotation and, when found, dumps out a "compilation error" that includes the results. This lets us write unit tests like this one, where we just check the result of this query.

So, in terms of first steps, these are good PRs to open:

  • Check predicates_of to be implemented in terms of a subsidiary predicate explicit_predicates_of.
  • Introduce inferred_outlives predicate and unit-testing mechanism. Initially, the predicate can always return an empty set of predicates, but we can add some tests nonetheless; they'll just initially show an empty result for the predicate.
  • Implement the predicate inference, fix the test, add more tests.

@nikomatsakis nikomatsakis added E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. and removed E-needs-mentor labels Sep 18, 2017
@toidiu
Copy link
Contributor

toidiu commented Sep 19, 2017

I would like to work on this!

@nikomatsakis
Copy link
Contributor

@toidiu hey, just checking in! I just r+'d that first PR, wondering if you'd had any time to mess around with the next few steps?

@toidiu
Copy link
Contributor

toidiu commented Sep 25, 2017

@nikomatsakis hey sry for the slow progress. I have have mainly been trying to wrap my head around all the different concepts and the repo structure. Have read the mentoring guide and rfc a few times now I am hoping to make some initial pr's early this week.

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this issue Sep 29, 2017
@toidiu toidiu mentioned this issue Oct 15, 2017
8 tasks
bors added a commit that referenced this issue Oct 16, 2017
#44493 add structure for inferred_outlives_of

#44493

- add placeholder for the final implementation of inferred_outlives_of
- add some placeholder tests
@nikomatsakis
Copy link
Contributor

Pseudocode of how the inference wants to work, I think:

// Compute a map from each struct/enum/union S to the **explicit**
// outlives predicates (`T: 'a`, `'a: 'b`) that the user wrote.
// Typically there won't be many of these, except in older code where
// they were mandatory. Nonetheless, we have to ensure that every such
// predicate is satisfied, so they form a kind of base set of requirements
// for the type.
let mut explicit_outlives_predicates = map();
for def_id in all_types() {
    let explicit_predicates = tcx.explicit_predicates(def_id);
    let filtered_predicates = explicit_predicates.iter().filter_map(is_outlives_predicate).collect();
    explicit_outlives_predicates.insert(
        def_id,
        filtered_predicates);
}

// Create the sets of inferred predicates for each type. These sets
// are initially empty but will grow during the inference step.
let mut inferred_outlives_predicates = map();
for def_id in all_types() {
    inferred_outlives_predicates.insert(def_id, empty_set());
}

// Now comes the inference part. We iterate over each type and figure out,
// for each of its fields, what outlives predicates are needed to make that field's
// type well-formed. Those get added to the `inferred_outlives_predicates` set
// for that type.
let mut changed = true;
while changed {
    changed = false;
    for def_id in all_types() {
        for field_ty in the HIR type definition {
            let required_predicates = required_predicates_for_type_to_be_wf(field_ty);
            inferred_outlives_predicates.extend(required_predicates);
            if new predicates were added { changed = true; }
        }
    }
}

// When we are done, `inferred_outlives_predicates` is the resulting map.

@Yoric
Copy link
Contributor

Yoric commented Dec 19, 2017

@toidiu Are you still working on this? If not, I'd be interested in taking a jab at this issue.

@toidiu
Copy link
Contributor

toidiu commented Dec 19, 2017

@Yoric yea still making progress and iterating on one PR so as not to queue up bors

@rfcbot rfcbot added the finished-final-comment-period The final comment period is finished for this PR / Issue. label Aug 9, 2018
@rfcbot
Copy link

rfcbot commented Aug 9, 2018

The final comment period, with a disposition to merge, as per the review above, is now complete.

@rfcbot rfcbot removed the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Aug 9, 2018
@nikomatsakis
Copy link
Contributor

Let's get this stabilized, then!

There are instructions for how to do so here:

https://forge.rust-lang.org/stabilization-guide.html

@toidiu — you would be up for that? You've seen this thing through from start to finish thus far!

@nikomatsakis nikomatsakis added 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. labels Aug 9, 2018
@nikomatsakis
Copy link
Contributor

I think we should also split off a new issue for the 'static feature gate...

@toidiu
Copy link
Contributor

toidiu commented Aug 9, 2018

@nikomatsakis yep would be glad to :)

@pnkfelix
Copy link
Member

visiting for triage. PR #53793 is meant to resolve this.

bors added a commit that referenced this issue Sep 12, 2018
@pnkfelix
Copy link
Member

visiting for triage. #53793 landed. closing as fixed.

@nikomatsakis
Copy link
Contributor

Created #54185 to track the 'static question

@nikomatsakis
Copy link
Contributor

#52042 already exists to track an idiom lint

zackmdavis added a commit to zackmdavis/rust that referenced this issue Sep 28, 2018
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.
bors added a commit that referenced this issue 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).

![infer_outlints](https://user-images.githubusercontent.com/1076988/43625740-6bf43dca-96a3-11e8-9dcf-793ac83d424d.png)

r? @alexcrichton
tonytonyjan added a commit to tonytonyjan/rust-book that referenced this issue Feb 24, 2019
reasons:

1. This annotation is no longer needed since it can be inferred from the fields present in the definitions in Rust 2018.
2. `T: 'a` annotation is never mentioned in previous chapters, this would make beinners confused.

references:

rust-lang/rfcs#2093
rust-lang/rust#44493
https://rust-lang-nursery.github.io/edition-guide/rust-2018/ownership-and-lifetimes/inference-in-structs.html
@pnkfelix
Copy link
Member

pnkfelix commented Aug 2, 2022

#54185 (comment) ended the question about whether to special-case 'static ; PR #97875 removed #![feature(infer_static_outlives_requirements)].

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. 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. finished-final-comment-period The final comment period is finished for this PR / Issue. P-high High priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests