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

[6/n] rustc: transition HIR function bodies from Block to Expr. #37412

Merged
merged 5 commits into from
Nov 10, 2016

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Oct 26, 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.


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 #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:

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:

// 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

@rust-highfive
Copy link
Collaborator

r? @nrc

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

hi: original_span.hi,
expn_id: original_span.expn_id
};
err.span_help(span_semi, "consider removing this semicolon:");
Copy link
Member Author

Choose a reason for hiding this comment

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

cc @GuillaumeGomez I wanted to show this on IRC but you left. This and the report_and_explain_type_error call above is how you can emit type errors with additional information (note/help) attached to them.

Copy link
Member

Choose a reason for hiding this comment

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

Ah nice, thanks!

@nrc
Copy link
Member

nrc commented Oct 26, 2016

r? @nikomatsakis

@rust-highfive rust-highfive assigned nikomatsakis and unassigned nrc Oct 26, 2016
@eddyb eddyb force-pushed the lazy-6 branch 2 times, most recently from 73f8338 to 065e10a Compare October 26, 2016 07:43
@bors
Copy link
Contributor

bors commented Oct 26, 2016

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

@Manishearth
Copy link
Member

Manishearth commented Oct 26, 2016

Feel free to merge this early (without a batch), just ping me when you do.

@bors
Copy link
Contributor

bors commented Oct 27, 2016

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

@bors
Copy link
Contributor

bors commented Oct 30, 2016

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

Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

r=me, presuming that there aren't any backcompat concerns here.

@@ -476,11 +484,12 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
};
}

// We won't diverge unless the discriminant or all arms diverge.
self.diverges.set(discrim_diverges | all_pats_diverge | all_bodies_diverge);
Copy link
Contributor

Choose a reason for hiding this comment

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

Not that I care too much here, but I guess plausibly you could have 50% of the patterns diverging and the other 50% of the bodies diverging, right?

i.e., the proper test is discrim_diverges | all_arms_diverge, where all_arms_diverge = pat_diverges | body_diverges

@@ -53,7 +53,7 @@ pub unsafe extern fn __rust_maybe_catch_panic(f: fn(*mut u8),
// now hopefully.
#[no_mangle]
pub unsafe extern fn __rust_start_panic(_data: usize, _vtable: usize) -> u32 {
return abort();
abort();
Copy link
Contributor

Choose a reason for hiding this comment

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

This is to silence a warning? Do we need to be concerned about back-compat? I guess this just causes new warnings, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

New lint warnings, which only cause problems because we use #![deny(warnings)].

@nikomatsakis
Copy link
Contributor

hmm, so I'm not sure how precise the old code was around matches -- is it worth doing a crater run or anything? Probably -- if you are with me -- you should just fix the match imprecision anyhow.

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Nov 4, 2016
[5/n] rustc: record the target type of every adjustment.

_This is part of a series ([prev](rust-lang#37404) | [next](rust-lang#37412)) 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:/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 first commit rearranges `tcx.tables` so that all users go through `tcx.tables()`. This in preparation for per-body `Tables` where they will be requested for a specific `DefId`. Included to minimize churn.

The rest of the changes focus on adjustments, there are some renamings, but the main addition is the target type, always available in all cases (as opposed to just for unsizing where it was previously needed).

Possibly the most significant effect of this change is that figuring out the final type of an expression is now _always_ just one successful `HashMap` lookup (either the adjustment or, if that doesn't exist, the node type).
@bors
Copy link
Contributor

bors commented Nov 6, 2016

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

@bors
Copy link
Contributor

bors commented Nov 9, 2016

🔒 Merge conflict

@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 10, 2016

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented Nov 10, 2016

📌 Commit 8e9106c has been approved by nikomatsakis

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:/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
```
bors added a commit that referenced this pull request Nov 10, 2016
Rollup of 5 pull requests

- Successful merges: #37402, #37412, #37661, #37664, #37667
- Failed merges:
@bors bors merged commit 8e9106c into rust-lang:master Nov 10, 2016
@eddyb eddyb deleted the lazy-6 branch November 10, 2016 08:54
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:/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 added a commit that referenced this pull request Jan 4, 2017
Don't leak the compiler's internal representation of scopes in error messages.

Fixes #37884 (actually fixes #27942, which was made worse by #37412) by handling more node types.
Ideally we'd turn the unknown node type situations into ICEs and fix them as they show up in errors.
But we might want to backport this patch so I was less aggressive.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants