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

Do not discern between statements with and without semicolon after lowering to HIR #61753

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
8 participants
@petrochenkov
Copy link
Contributor

commented Jun 11, 2019

In AST we need to discern between statements with semicolon and statements without semicolon in a block to detect the block's "trailing expression" (which may end up trailing after macro expansion, but start somewhere in the middle of the block, see the in-progress issue #61733).

After the trailing expression is detected during lowering to HIR, the distinction is no longer necessary...

... with exception of one surprising corner case that I discovered today:

fn main() {
    0; // OK, unused expression

    ()
}
fn main() {
    { 0 }; // OK, unused expression

    ()
}
fn main() {
    { 0 } // ERROR expected (), found integer (???)

    ()
}

Apparently semicolon-less expression in the middle of a block must unify with () for some reason.

The reason is unclear to me, so this PR removes this special case and unifies StmtKind::Expr and StmtKind::Semi in HIR.
r? @nikomatsakis

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2019

@bors try

bors added a commit that referenced this pull request Jun 11, 2019

Auto merge of #61753 - petrochenkov:noexpr, r=<try>
Do not discern between statements with and without semicolon after lowering to HIR

In AST we need to discern between statements with semicolon and statements without semicolon in a block to detect the block's "trailing expression" (which may be end up trailing after macro expansion, but start somewhere in the middle of the block).

After the trailing expression is detected during lowering to HIR, the distinction is no longer necessary...

... with exception of one surprising corner case that I discovered today:
```rust
fn main() {
    0; // OK, unused expression

    ()
}
fn main() {
    { 0 }; // OK, unused expression

    ()
}
fn main() {
    { 0 } // ERROR expected (), found integer (???)

    ()
}
```
Apparently semicolon-less expression in the middle of a block must unify with `()` for some reason.

The reason is unclear to me, so this PR removes this special case and unifies `StmtKind::Expr` and `StmtKind::Semi` in HIR.
r? @nikomatsakis
@bors

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2019

⌛️ Trying commit bf0b6aa with merge af22dfc...

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2019

@@ -1205,20 +1205,16 @@ pub enum StmtKind {
/// An item binding.
Item(ItemId),

/// An expression without a trailing semi-colon (must have unit type).
/// An expression statement.

This comment has been minimized.

Copy link
@Centril

Centril Jun 11, 2019

Member
Suggested change
/// An expression statement.
/// An expression statement (may have any type).
hir_vec![next_let, match_stmt, pat_let, body_stmt],
None,
hir_vec![next_let, match_stmt, pat_let],
Some(body_expr),

This comment has been minimized.

Copy link
@Centril

Centril Jun 11, 2019

Member

Can you update the comment that outlines the desugaring at the top of the match arm?

@@ -4856,10 +4856,6 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
// Ignore for now.
hir::StmtKind::Item(_) => {}
hir::StmtKind::Expr(ref expr) => {
// Check with expected type of `()`.
self.check_expr_has_type_or_error(&expr, self.tcx.mk_unit());

This comment has been minimized.

Copy link
@Centril

Centril Jun 11, 2019

Member

(T-Lang: N.B. this removes the expectation that { expr } : () and so therefore:

{ core::default::Default::default() }
1;

may stop compiling (crater will find out).

We could add inference defaulting to () if that is desirable.

@cramertj

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

Apparently semicolon-less expression in the middle of a block must unify with () for some reason.

Huh, that's the behavior I would've expected-- my intuition was that intermediate statements should always be of type (), and that using ; turns a T-returning-expression into a ()-returning expression.

@eddyb

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

Yeah, the intention is that ; throws away the value but otherwise every statement must evaluate to (). I suspect this PR might fail on crater due removing information from inference .

@bors

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2019

☀️ Try build successful - checks-travis
Build commit: af22dfc

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2019

@craterbot run mode=check-only

@craterbot

This comment has been minimized.

Copy link
Collaborator

commented Jun 12, 2019

👌 Experiment pr-61753 created and queued.
🤖 Automatically detected try build af22dfc
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2019

@cramertj @eddyb
My intuition was that

  • statements don't have types or values at all
  • results of expressions in trailing position are returned, so their type is restricted by the return type
  • results of expressions in the middle of a block are thrown away, so their type is not restricted
@craterbot

This comment has been minimized.

Copy link
Collaborator

commented Jun 12, 2019

🚧 Experiment pr-61753 is now running on agent aws-1.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot

This comment has been minimized.

Copy link
Collaborator

commented Jun 14, 2019

🎉 Experiment pr-61753 is completed!
📊 57 regressed and 0 fixed (60951 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

commented Jun 15, 2019

Hmm, all the errors are in the "does not live long enough" category, rather than regressed type inference.

This is due to changes in for loop desugaring.
Previously for loops implicitly added a semicolon to the end of their body.

for x in xs {
    doit()
}

=>

for x in xs {
    doit();
}

(No other block or loop constructions do that.)

UPDATE: It's even worse - for desugaring creates a semicolon-less statement that is in the last position, but not considered trailing - something that can never be obtained from actual source code.

I'll revert this part and you'll see what effect it has on tests.

@petrochenkov petrochenkov force-pushed the petrochenkov:noexpr branch from bf0b6aa to 0e716aa Jun 16, 2019

fn main() {
for x in 0..3 {
x //~ ERROR mismatched types
x

This comment has been minimized.

Copy link
@petrochenkov

petrochenkov Jun 16, 2019

Author Contributor

x no longer needs to unify with (), so it's no longer an error.

fn main() {
// Check that the tail statement in the body unifies with something
for _ in 0..3 {
unsafe { std::mem::uninitialized() }
unsafe { std::mem::uninitialized() } //~ ERROR type annotations needed

This comment has been minimized.

Copy link
@petrochenkov

petrochenkov Jun 16, 2019

Author Contributor

unsafe { std::mem::uninitialized() } no longer needs to unify with (), so it cannot infer its type.

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

commented Jun 16, 2019

Updated with for loop desugaring changes reverted.

@eddyb

This comment has been minimized.

Copy link
Member

commented Jun 16, 2019

@petrochenkov Can you open an issue about the for inconsistency?

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

commented Jun 16, 2019

@eddyb

Can you open an issue about the for inconsistency?

#61902

@cramertj

This comment has been minimized.

Copy link
Member

commented Jun 20, 2019

We discussed this in the @rust-lang/lang meeting and we agreed that the current behavior is roughly as-intended (implementation inconsistencies aside), and that this change and similar ones should go through the RFC process.

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

commented Jun 20, 2019

Ok, this is not something important enough to go through an RFC.
A documentation issue for this quirk - rust-lang-nursery/reference#625.

The for loop inconsistency (#61902) still needs some attention.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.