Flip "region lattice" in RegionKind doc comment#152968
Flip "region lattice" in RegionKind doc comment#152968rust-bors[bot] merged 1 commit intorust-lang:mainfrom
Conversation
|
r? @jieyouxu rustbot has assigned @jieyouxu. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
@rustbot reroll |
|
r? me |
| /// function declaration. They have relationships to one another | ||
| /// determined based on the declared relationships from the | ||
| /// function. | ||
| /// Early-bound/free regions ([`RegionKind::ReEarlyParam`]) are the |
There was a problem hiding this comment.
I think this comment is out of date in that "free regions" here refers to a previously existing RegionKind::Free which got renamed to ReLateParam. This should probably say something like "Early/LateParam regions are the..."
There was a problem hiding this comment.
alternatively we did actually mean free regions in which case this includes 'static but talking explicitly about early bound regions is confusing...
There was a problem hiding this comment.
maybe this should actually be "Free regions (ReEarlyParam ReLateParam ReStatic) are the named lifetimes in scope from.." i'm not sure 😅
There was a problem hiding this comment.
"Early/LateParam regions are the named lifetimes in scope from the function declaration. They have relationships to one another and 'static based on .." is probably the most useful thing to document 🤔
There was a problem hiding this comment.
Early/LateParam regions are the named lifetimes in scope from the function declaration
late params can be anonymous, right? just gonna remove the word "named".
Also hm, ReLateParam can be constructed from HKT ReBound stuff (liberate_late_bound_regions), so they're not always "from the function declaration". Maybe flipping the sentence:
Lifetimes in scope from a function declaration are represented via ReEarlyParam/ReLateParam. They have relationships to one another and
'staticbased on the declared relationships from the function.
because all lifetimes from a function declaration are ReEarlyParam/ReLateParam, but not all ReLateParam are from a function declaration.
bbbbc92 to
8cca904
Compare
| /// | ||
| /// ## Bound Regions | ||
| /// | ||
| /// These are regions that are stored behind a binder and must be instantiated |
There was a problem hiding this comment.
I think the pre-existing comment here is wrong/wildly misleading. Early bound parameters are not ReBound. This comment implies that a ReBound can correspond to either an early or a late bound parameter but actually it just always corresponds to a lifetime from a for<'a, ...> binder
I would probably drop everything about early/late here and only talk about them in relation to the Binder type
There was a problem hiding this comment.
just pushed an edit, how's this?
d0ac5b1 to
40374b4
Compare
| /// The distinction between early and late exists because higher-ranked | ||
| /// lifetimes aren't supported in all places. See [1] [2]. |
There was a problem hiding this comment.
This is a fairly complex topic. I think we should just link to the dev guide chapter on early/late parameters and go "read here for why there are two kinds of parameters"
There was a problem hiding this comment.
Agreed, I just copied this from the original text to preserve it somewhere.
Do you think it's worth it to preserve the links to the blog posts linked here, or should we just link to the dev guide and delete the links to these posts?
There was a problem hiding this comment.
hmm, can you drop them but link to them from the dev guide? you should be able to edit it at src/doc/rustc-dev-guide/src/early-late-parameters.md or something like that
There was a problem hiding this comment.
probably like a note at the very start of that chapter that goes "see also these blog posts from when this distinction was introduced"
40374b4 to
3eb071e
Compare
|
The rustc-dev-guide subtree was changed. If this PR only touches the dev guide consider submitting a PR directly to rust-lang/rustc-dev-guide otherwise thank you for updating the dev guide with your changes. |
|
wonderful! @bors r+ rollup |
…yUwU Flip "region lattice" in RegionKind doc comment If we assume that references take their region covariantly, and use that to define the subtyping relationship of regions, `'empty` is ⊤ (top) and `'static` is ⊥ (bottom), and that `'a <: 'b` is defined as `'a >= 'b`. However, the RegionKind doc comment's "region lattice" had `'static` at Top. While this is perhaps technically correct (a so-called "region lattice" is not a "type lattice"), it confused me a lot, and took me half an hour to be like "no, ok, I *do* understand things correctly, this region lattice is just flipped from the subtype lattice". Seems better to just have them be the same! I also took the opportunity to rewrite some parts of the comment with notes that would have helped me when starting out. Bikesheds welcome (as long as they don't go on forever)! For context: the comment "`'static` is Top" was [written in 2013](rust-lang@a896440#diff-8780054cdf09361c4ac2540c7f544ecfdc52a9e610822aabb8bcd2964affcb1cR430), before there was a [discussion in 2014](rust-lang#15699) that clarified that references take their regions covariantly, and `'a <: 'b` is defined as `'a >= 'b`. See also Zulip thread: https://rust-lang.zulipchat.com/#narrow/channel/131828-t-compiler/topic/RegionKind.20rustc.20doc.20comment
Rollup of 7 pull requests Successful merges: - #153801 (Add the option to run UI tests with the parallel frontend) - #153967 (Tweak wording of failed predicate in inference error) - #152968 (Flip "region lattice" in RegionKind doc comment) - #153531 (Fix LegacyKeyValueFormat report from docker build: various) - #153709 (Fix hypothetical ICE in `variances_of`) - #153884 (test `classify-runtime-const` for `f16`) - #153946 (dissolve `tests/ui/cross`)
…uwer Rollup of 14 pull requests Successful merges: - #153972 (stdarch subtree update) - #153801 (Add the option to run UI tests with the parallel frontend) - #153959 (Fix non-module `parent_module` in stripped cfg diagnostics) - #153967 (Tweak wording of failed predicate in inference error) - #152968 (Flip "region lattice" in RegionKind doc comment) - #153531 (Fix LegacyKeyValueFormat report from docker build: various) - #153622 (remove concept of soft-unstable features) - #153709 (Fix hypothetical ICE in `variances_of`) - #153884 (test `classify-runtime-const` for `f16`) - #153894 (Point at unit structs on foreign crates in type errors when they are the pattern of a binding) - #153920 (improve `#[track_caller]` invalid ABI error) - #153946 (dissolve `tests/ui/cross`) - #153965 (Fix minor kasan bugs) - #153991 (Small report_cycle refactor)
…uwer Rollup of 14 pull requests Successful merges: - rust-lang/rust#153972 (stdarch subtree update) - rust-lang/rust#153801 (Add the option to run UI tests with the parallel frontend) - rust-lang/rust#153959 (Fix non-module `parent_module` in stripped cfg diagnostics) - rust-lang/rust#153967 (Tweak wording of failed predicate in inference error) - rust-lang/rust#152968 (Flip "region lattice" in RegionKind doc comment) - rust-lang/rust#153531 (Fix LegacyKeyValueFormat report from docker build: various) - rust-lang/rust#153622 (remove concept of soft-unstable features) - rust-lang/rust#153709 (Fix hypothetical ICE in `variances_of`) - rust-lang/rust#153884 (test `classify-runtime-const` for `f16`) - rust-lang/rust#153894 (Point at unit structs on foreign crates in type errors when they are the pattern of a binding) - rust-lang/rust#153920 (improve `#[track_caller]` invalid ABI error) - rust-lang/rust#153946 (dissolve `tests/ui/cross`) - rust-lang/rust#153965 (Fix minor kasan bugs) - rust-lang/rust#153991 (Small report_cycle refactor)
…uwer Rollup of 14 pull requests Successful merges: - rust-lang/rust#153972 (stdarch subtree update) - rust-lang/rust#153801 (Add the option to run UI tests with the parallel frontend) - rust-lang/rust#153959 (Fix non-module `parent_module` in stripped cfg diagnostics) - rust-lang/rust#153967 (Tweak wording of failed predicate in inference error) - rust-lang/rust#152968 (Flip "region lattice" in RegionKind doc comment) - rust-lang/rust#153531 (Fix LegacyKeyValueFormat report from docker build: various) - rust-lang/rust#153622 (remove concept of soft-unstable features) - rust-lang/rust#153709 (Fix hypothetical ICE in `variances_of`) - rust-lang/rust#153884 (test `classify-runtime-const` for `f16`) - rust-lang/rust#153894 (Point at unit structs on foreign crates in type errors when they are the pattern of a binding) - rust-lang/rust#153920 (improve `#[track_caller]` invalid ABI error) - rust-lang/rust#153946 (dissolve `tests/ui/cross`) - rust-lang/rust#153965 (Fix minor kasan bugs) - rust-lang/rust#153991 (Small report_cycle refactor)
If we assume that references take their region covariantly, and use that to define the subtyping relationship of regions,
'emptyis ⊤ (top) and'staticis ⊥ (bottom), and that'a <: 'bis defined as'a >= 'b. However, the RegionKind doc comment's "region lattice" had'staticat Top. While this is perhaps technically correct (a so-called "region lattice" is not a "type lattice"), it confused me a lot, and took me half an hour to be like "no, ok, I do understand things correctly, this region lattice is just flipped from the subtype lattice". Seems better to just have them be the same!I also took the opportunity to rewrite some parts of the comment with notes that would have helped me when starting out.
Bikesheds welcome (as long as they don't go on forever)!
For context: the comment "
'staticis Top" was written in 2013, before there was a discussion in 2014 that clarified that references take their regions covariantly, and'a <: 'bis defined as'a >= 'b.See also Zulip thread: https://rust-lang.zulipchat.com/#narrow/channel/131828-t-compiler/topic/RegionKind.20rustc.20doc.20comment