Skip to content

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Oct 19, 2019

This is based on discussion at https://github.com/rust-lang/rust/pull/64890/files#r334555787.

That said, why are function arguments the only unsized locals that could remain uninitialized? Couldn't we also fail to initialize some local but still go on with const_prop, and then hit a line that takes a reference to that? Cc @wesleywiser @oli-obk ; I don't know enough about const-prop to understand why this can happen only for function arguments.

The PR includes #64890; the only new commit is 05e4e6ba0d5.

@rust-highfive
Copy link
Contributor

r? @davidtwco

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 19, 2019
@rust-highfive

This comment has been minimized.

@RalfJung
Copy link
Member Author

Hm, looks like changing the <= to a < makes a test fail.... oh it's because local 0 is the return place. Yeah that conditional needed a lot of comments that were never written.^^

@RalfJung
Copy link
Member Author

Here's an example of taking a ref to an unsized local that's not a function argument... why does that not ICE in const-prop?

@wesleywiser
Copy link
Member

r? @wesleywiser

@wesleywiser
Copy link
Member

why does that not ICE in const-prop?

Currently, we only propagate temporaries, never variables:

// cannot use locals because if x < y { y - x } else { x - y } would
// lint for x != y
// FIXME(oli-obk): lint variables until they are used in a condition
// FIXME(oli-obk): lint if return value is constant
*val = body.local_kind(local) == LocalKind::Temp;

Assigning the argument to a variable thwarts ConstProp and bypasses the ICE.

@RalfJung
Copy link
Member Author

I see. Still, it strikes me as a better test to check for "uninitialized + usized", which matches the actual error condition. Or even just "uninitialized", as that's already impossible code (though maybe there's something useful const-prop can do there?).

@wesleywiser
Copy link
Member

That seems reasonable to me! Off-hand, I can't think of anything useful const prop can do with uninitialized values.

@RalfJung
Copy link
Member Author

So, something like this? (Local build is sill ongoing.)

@RalfJung RalfJung changed the title [WIP] clarify const_prop ICE protection comment clarify const_prop ICE protection comment Oct 20, 2019
@RalfJung
Copy link
Member Author

Okay, tests are passing locally. The PR is ready for review.

@wesleywiser
Copy link
Member

Thanks @RalfJung!

@bors r+

@bors
Copy link
Collaborator

bors commented Oct 20, 2019

📌 Commit f907fbe has been approved by wesleywiser

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 20, 2019
Centril added a commit to Centril/rust that referenced this pull request Oct 20, 2019
…eywiser

clarify const_prop ICE protection comment

This is based on discussion at https://github.com/rust-lang/rust/pull/64890/files#r334555787.

That said, why are function arguments the only unsized locals that could remain uninitialized? Couldn't we also fail to initialize some local but still go on with const_prop, and then hit a line that takes a reference to that? Cc @wesleywiser @oli-obk ; I don't know enough about const-prop to understand why this can happen only for function arguments.

~~The PR includes rust-lang#64890; the only new commit is 05e4e6ba0d5.~~
bors added a commit that referenced this pull request Oct 20, 2019
Rollup of 8 pull requests

Successful merges:

 - #65314 (rustdoc: forward -Z options to rustc)
 - #65592 (clarify const_prop ICE protection comment)
 - #65603 (Avoid ICE when include! is used by stdin crate)
 - #65614 (Improve error message for APIT with explicit generic arguments)
 - #65629 (Remove `borrowck_graphviz_postflow` from test)
 - #65633 (Remove leading :: from paths in doc examples)
 - #65638 (Rename the default argument 'def' to 'default')
 - #65639 (Fix parameter name in documentation)

Failed merges:

r? @ghost
@bors bors merged commit f907fbe into rust-lang:master Oct 21, 2019
@RalfJung RalfJung deleted the const-prop-comment branch October 21, 2019 08:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants