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

[7/n] rustc: desugar UFCS in HIR and don't use DefMap for associated resolutions. #37676

Merged
merged 12 commits into from
Nov 28, 2016

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Nov 9, 2016

This is part of a series (prev | next) of patches designed to rework rustc into an out-of-order on-demand pipeline model for both better feature support (e.g. MIR-based early constant evaluation) and incremental execution of compiler passes (e.g. type-checking), with beneficial consequences to IDE support as well.
If any motivation is unclear, please ask for additional PR description clarifications or code comments.


Previously, a path like T::Assoc::method, while equivalent to <<T>::Assoc>::method, wasn't desugared in any way at the HIR level and everything inspecting it had to either deal with knowing only T (before typeck) or knowing only the definition of method (after typeck).
Such a path also had only one NodeId and associated resolution during typeck modified DefMap, in a way that would be hard for incremental recompilation to track, and inconvenient for partial type conversions from HIR to Ty, which are required to break faux-cycles in on-demand type collection.

The desugarings performed by this PR are as follows:

  • use a::{b,c}; is flattened to use a as _; use a::b; use a::c;
    • as resolution is complete, use a as _; doesn't do anything, except get checked for stability
  • Vec::new (an expression) becomes Vec<..>::new<..>, to distinguish it from <Vec>::new<..>
    • the "infer all parameters" <..> form is internal and not even pretty-printed
    • used when there are no type parameters at all, in an expression or pattern path segment
  • T::A::B becomes <<T>::A>::B in a type, and <<T<..>>::A<..>>::B<..> in an expression/pattern
    • one additional hir::Ty node is created for each prefix, starting with the fully-resolved type (T) and extending it with each segment (e.g. <T>::A)
  • fully-resolved paths contain their Def in HIR, getting rid of the DefMap and absolving incremental recompilation of needing to manually look up nodes to handle that side information

Not keeping the DefMap around meant that associated resolutions had to be stored somewhere else:

  • expressions and patterns use a new NodeId -> Def map in ty::Tables
    • compatible with the future per-body (constant / fn / closure) Tables
  • types are accessible via Ty and the usual per-item generics / predicates / type
    • rustdoc and save-analysis are the only situations which insist on mapping syntactical types to semantical ones, or at least understand the resolution of associated types, therefore the type conversion cache, i.e. a NodeId -> Ty map, is exposed by typeck for this purpose
    • stability had to be split into a pass that runs on HIR and checks the results of name resolution, and impromptu checks triggered by typeck for associated paths, methods, fields, etc.
    • privacy using semantic types results in accurate reachability for impl Trait, which fixes Using impl trait across crates results in linker error #35870, and thorough introspection of associated types, which may allow relaxing private-in-public checking on bounds, while keeping the intended ban on projections with private type parameters

cc @petrochenkov

@rust-highfive
Copy link
Collaborator

r? @Aatch

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

@bors
Copy link
Contributor

bors commented Nov 9, 2016

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

@eddyb
Copy link
Member Author

eddyb commented Nov 9, 2016

r? @nikomatsakis

@rust-highfive rust-highfive assigned nikomatsakis and unassigned Aatch Nov 9, 2016
@eddyb eddyb mentioned this pull request Nov 9, 2016
@eddyb eddyb force-pushed the lazy-7 branch 2 times, most recently from 869c0b3 to 0bf9c99 Compare November 10, 2016 01:14
eddyb added a commit to eddyb/rust that referenced this pull request Nov 10, 2016
[6/n] rustc: transition HIR function bodies from Block to Expr.

_This is part of a series ([prev](rust-lang#37408) | [next](rust-lang#37676)) of patches designed to rework rustc into an out-of-order on-demand pipeline model for both better feature support (e.g. [MIR-based](https://github.com/solson/miri) early constant evaluation) and incremental execution of compiler passes (e.g. type-checking), with beneficial consequences to IDE support as well.
If any motivation is unclear, please ask for additional PR description clarifications or code comments._

<hr>

The main change here is that functions and closures both use `Expr` instead of `Block` for their bodies.
For closures this actually allows a honest representation of brace-less closure bodies, e.g. `|x| x + 1` is now distinguishable from `|x| { x + 1 }`, therefore this PR is `[syntax-breaking]` (cc @Manishearth).

Using `Expr` allows more logic to be shared between constant bodies and function bodies, with some small such changes already part of this PR, and eventually easing rust-lang#35078 and per-body type tables.

Incidentally, there used to be some corners cut here and there and as such I had to (re)write divergence tracking for type-checking so that it is capable of understanding basic structured control-flow:

``` rust
fn a(x: bool) -> i32 {
    // match also works (as long as all arms diverge)
    if x { panic!("true") } else { return 1; }
    0 // "unreachable expression" after this PR
}
```

And since liveness' "not all control paths return a value" moved to type-checking we can have nice things:

``` rust
// before & after:
fn b() -> i32 { 0; } // help: consider removing this semicolon

// only after this PR
fn c() -> i32 { { 0; } } // help: consider removing this semicolon
fn d() { let x: i32 = { 0; }; } // help: consider removing this semicolon
fn e() { f({ 0; }); } // help: consider removing this semicolon
```
@eddyb eddyb force-pushed the lazy-7 branch 2 times, most recently from 16ee826 to 006c75d Compare November 10, 2016 03:01
@bors
Copy link
Contributor

bors commented Nov 10, 2016

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

@eddyb
Copy link
Member Author

eddyb commented Nov 10, 2016

Crater report shows no user-facing regressions.

@bors
Copy link
Contributor

bors commented Nov 10, 2016

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

@eddyb eddyb force-pushed the lazy-7 branch 4 times, most recently from b2cde2a to 7449ef9 Compare November 10, 2016 17:08
@petrochenkov
Copy link
Contributor

(I'll read this on Friday or Saturday.)

@alexcrichton alexcrichton added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Nov 10, 2016
eddyb added a commit to eddyb/rust that referenced this pull request Nov 11, 2016
[8/n] rustc: clean up lookup_item_type and remove TypeScheme.

_This is part of a series ([prev](rust-lang#37676) | [next]()) of patches designed to rework rustc into an out-of-order on-demand pipeline model for both better feature support (e.g. [MIR-based](https://github.com/solson/miri) early constant evaluation) and incremental execution of compiler passes (e.g. type-checking), with beneficial consequences to IDE support as well.
If any motivation is unclear, please ask for additional PR description clarifications or code comments._

<hr>

* `tcx.tcache` -> `tcx.item_types`
* `TypeScheme` (grouping `Ty` and `ty::Generics`) is removed
* `tcx.item_types` entries no longer duplicated in `tcx.tables.node_types`
* `tcx.lookup_item_type(def_id).ty` -> `tcx.item_type(def_id)`
* `tcx.lookup_item_type(def_id).generics` -> `tcx.item_generics(def_id)`
* `tcx.lookup_generics(def_id)` -> `tcx.item_generics(def_id)`
* `tcx.lookup_{super_,}predicates(def_id)` -> `tcx.item_{super_,}predicates(def_id)`
eddyb added a commit to eddyb/rust that referenced this pull request Nov 12, 2016
[8/n] rustc: clean up lookup_item_type and remove TypeScheme.

_This is part of a series ([prev](rust-lang#37676) | [next]()) of patches designed to rework rustc into an out-of-order on-demand pipeline model for both better feature support (e.g. [MIR-based](https://github.com/solson/miri) early constant evaluation) and incremental execution of compiler passes (e.g. type-checking), with beneficial consequences to IDE support as well.
If any motivation is unclear, please ask for additional PR description clarifications or code comments._

<hr>

* `tcx.tcache` -> `tcx.item_types`
* `TypeScheme` (grouping `Ty` and `ty::Generics`) is removed
* `tcx.item_types` entries no longer duplicated in `tcx.tables.node_types`
* `tcx.lookup_item_type(def_id).ty` -> `tcx.item_type(def_id)`
* `tcx.lookup_item_type(def_id).generics` -> `tcx.item_generics(def_id)`
* `tcx.lookup_generics(def_id)` -> `tcx.item_generics(def_id)`
* `tcx.lookup_{super_,}predicates(def_id)` -> `tcx.item_{super_,}predicates(def_id)`
@eddyb eddyb force-pushed the lazy-7 branch 2 times, most recently from a01d993 to 74e72f3 Compare November 12, 2016 11:16
@eddyb
Copy link
Member Author

eddyb commented Nov 28, 2016

@arielb1 Turns out your changes to the handling of closures means impl Fn just works now, heh.

@eddyb
Copy link
Member Author

eddyb commented Nov 28, 2016

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented Nov 28, 2016

📌 Commit 372c6df has been approved by nikomatsakis

@bors
Copy link
Contributor

bors commented Nov 28, 2016

⌛ Testing commit 372c6df with merge 0b399e5...

bors added a commit that referenced this pull request Nov 28, 2016
[7/n] rustc: desugar UFCS in HIR and don't use DefMap for associated resolutions.

_This is part of a series ([prev](#37412) | [next](#37688)) of patches designed to rework rustc into an out-of-order on-demand pipeline model for both better feature support (e.g. [MIR-based](https://github.com/solson/miri) early constant evaluation) and incremental execution of compiler passes (e.g. type-checking), with beneficial consequences to IDE support as well.
If any motivation is unclear, please ask for additional PR description clarifications or code comments._

<hr>

Previously, a path like `T::Assoc::method`, while equivalent to `<<T>::Assoc>::method`, wasn't desugared in any way at the HIR level and everything inspecting it had to either deal with knowing only `T` (before typeck) or knowing only the definition of `method` (after typeck).
Such a path also had only one `NodeId` and associated resolution during typeck modified `DefMap`, in a way that would be hard for incremental recompilation to track, and inconvenient for partial type conversions from HIR to `Ty`, which are required to break faux-cycles in on-demand type collection.

The desugarings performed by this PR are as follows:
* `use a::{b,c};` is flattened to `use a as _; use a::b; use a::c;`
  * as resolution is complete, `use a as _;` doesn't do anything, except get checked for stability
* `Vec::new` (an expression) becomes `Vec<..>::new<..>`, to distinguish it from `<Vec>::new<..>`
  * the "infer all parameters" `<..>` form is internal and not even pretty-printed
  * used when there are no type parameters at all, in an expression or pattern path segment
* `T::A::B` becomes `<<T>::A>::B` in a type, and `<<T<..>>::A<..>>::B<..>` in an expression/pattern
  * one additional `hir::Ty` node is created for each prefix, starting with the fully-resolved type (`T`) and extending it with each segment (e.g. `<T>::A`)
* fully-resolved paths contain their `Def` in HIR, getting rid of the `DefMap` and absolving incremental recompilation of needing to manually look up nodes to handle that side information

Not keeping the `DefMap` around meant that associated resolutions had to be stored somewhere else:
* expressions and patterns use a new `NodeId -> Def` map in `ty::Tables`
  * compatible with the future per-body (constant / `fn` / closure) `Tables`
* types are accessible via `Ty` and the usual per-item generics / predicates / type
  * `rustdoc` and `save-analysis` are the only situations which insist on mapping syntactical types to semantical ones, or at least understand the resolution of associated types, therefore the type conversion cache, i.e. a `NodeId -> Ty` map, is exposed by typeck for this purpose
  * stability had to be split into a pass that runs on HIR and checks the results of name resolution, and impromptu checks triggered by `typeck` for associated paths, methods, fields, etc.
  * privacy using semantic types results in accurate reachability for `impl Trait`, which fixes #35870, and thorough introspection of associated types, which may allow relaxing private-in-public checking on bounds, while keeping the intended ban on projections with private type parameters

cc @petrochenkov
@bors bors merged commit 372c6df into rust-lang:master Nov 28, 2016
@eddyb eddyb deleted the lazy-7 branch November 28, 2016 13:09
sanxiyn added a commit to sanxiyn/rust that referenced this pull request Dec 15, 2016
This code was introduced in rust-lang#27565 to mark types in paths alive. It is now unnecessary since rust-lang#37676.
critiqjo pushed a commit to critiqjo/rustdoc that referenced this pull request Dec 16, 2016
[8/n] rustc: clean up lookup_item_type and remove TypeScheme.

_This is part of a series ([prev](rust-lang/rust#37676) | [next]()) of patches designed to rework rustc into an out-of-order on-demand pipeline model for both better feature support (e.g. [MIR-based](https://github.com/solson/miri) early constant evaluation) and incremental execution of compiler passes (e.g. type-checking), with beneficial consequences to IDE support as well.
If any motivation is unclear, please ask for additional PR description clarifications or code comments._

<hr>

* `tcx.tcache` -> `tcx.item_types`
* `TypeScheme` (grouping `Ty` and `ty::Generics`) is removed
* `tcx.item_types` entries no longer duplicated in `tcx.tables.node_types`
* `tcx.lookup_item_type(def_id).ty` -> `tcx.item_type(def_id)`
* `tcx.lookup_item_type(def_id).generics` -> `tcx.item_generics(def_id)`
* `tcx.lookup_generics(def_id)` -> `tcx.item_generics(def_id)`
* `tcx.lookup_{super_,}predicates(def_id)` -> `tcx.item_{super_,}predicates(def_id)`
bors added a commit that referenced this pull request Jul 20, 2017
Fix checking for missing stability annotations

This was a regression from #37676 causing "unmarked API" ICEs like #43027.

r? @alexcrichton
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using impl trait across crates results in linker error
8 participants