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

clarify const_prop ICE protection comment #65592

Merged
merged 2 commits into from
Oct 21, 2019

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
Collaborator

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
Contributor

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