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

Allow let bindings and destructuring in constants and const fn #49172

Merged
merged 6 commits into from
May 22, 2018

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Mar 19, 2018

r? @eddyb

cc #48821

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 19, 2018
@@ -120,7 +120,7 @@ struct Qualifier<'a, 'gcx: 'a+'tcx, 'tcx: 'a> {
rpo: ReversePostorder<'a, 'tcx>,
tcx: TyCtxt<'a, 'gcx, 'tcx>,
param_env: ty::ParamEnv<'tcx>,
temp_qualif: IndexVec<Local, Option<Qualif>>,
local_qualif: IndexVec<Local, Option<Qualif>>,
return_qualif: Option<Qualif>,
Copy link
Member

Choose a reason for hiding this comment

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

Can you move this into local_qualif?

@@ -189,8 +189,16 @@ impl<'a, 'tcx> Qualifier<'a, 'tcx, 'tcx> {

/// Error about extra statements in a constant.
fn statement_like(&mut self) {
if self.tcx.sess.features_untracked().const_let {
return;
Copy link
Member

Choose a reason for hiding this comment

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

This seems bad, because statement_like is used for a lot of errors - it should maybe be split into several functions?

self.tcx.sess.features_untracked().const_let => {
debug!("store to var {:?}", index);
match self.local_qualif[index] {
Some(ref mut qualif) => *qualif &= self.qualif,
Copy link
Member

Choose a reason for hiding this comment

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

I think you want to replace the qualif always (e.g. let mut x = None; x = Some(TypeThatNeedsDrop);).
Also, if you have this special case, why do you need to do anything about statement_like?

@@ -678,6 +700,10 @@ impl<'a, 'tcx> Visitor<'tcx> for Qualifier<'a, 'tcx, 'tcx> {
self.add(Qualif::STATIC_REF);
}

if self.mode == Mode::ConstFn && self.tcx.sess.features_untracked().const_let {
return;
Copy link
Member

Choose a reason for hiding this comment

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

This is bad because it breaks promotion in const fn. Also, you want mutable borrows to locals outside of const fn too, right? I think we should orthogonally allow &mut foo in regular static, so only const needs a special-case at the outermost level (and promotion also needs to keep disallowing &mut NotZST).

@eddyb
Copy link
Member

eddyb commented Mar 24, 2018

@oli-obk If you want to get this merged quickly, I'd suggest dropping the borrowing changes.

cc @nikomatsakis A bit of a conundrum: static X: &'static mut Foo = &mut Foo {...}; would be fine to allow because static providing immutable access means it's effectively only &Foo.
However, const Y: &'static mut Foo = &mut Foo {...}; can't be allowed (unless Foo is ZST) because the const semantics would result in copies of a &mut Foo (only valid for ZSTs).

But we do want to allow const Z: Bar = Bar::new(&mut Foo {...}); with a temporary lifetime for the &mut Foo reference, so the "top-level escaping mutable reference" is relevant here.
So I'm wondering if, for const specifically, we want to adjust the temporary lifetime rules, such that &mut expr expressions aren't special-cased (same as if they were written id(&mut expr)).
That means const ARRAY: &'static mut [T; 0] = &mut []; would still work, but only through promotion (which has the same rules, IIRC, as what we allow today at the-top level, except around drops, which shouldn't be an issue for [T; 0] types, the only ones we ever allowed).

@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 30, 2018

I removed the borrowing changes

@shepmaster
Copy link
Member

Ping from triage, @eddyb !

@oli-obk
Copy link
Contributor Author

oli-obk commented Apr 8, 2018

This is very low priority atm. We could close it until after the next beta cut.

@pietroalbini
Copy link
Member

Ping from triage @eddyb! This PR needs your review.

@pietroalbini
Copy link
Member

Ping from triage! Can @eddyb or someone else from @rust-lang/compiler review this?

@@ -1058,6 +1078,11 @@ This does not pose a problem by itself because they can't be accessed directly."
// Avoid a generic error for other uses of arguments.
if self.qualif.intersects(Qualif::FN_ARGUMENT) {
let decl = &self.mir.local_decls[index];
if decl.source_info.span.allows_unstable() {
Copy link
Member

@eddyb eddyb Apr 24, 2018

Choose a reason for hiding this comment

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

Isn't this condition flipped? EDIT: this appears to be the same for all allows_unstable.

@@ -355,10 +365,13 @@ impl<'a, 'tcx> Qualifier<'a, 'tcx, 'tcx> {
TerminatorKind::FalseUnwind { .. } => None,

TerminatorKind::Return => {
if self.tcx.sess.features_untracked().const_let {
break;
Copy link
Member

Choose a reason for hiding this comment

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

I would instead put an if around all the code below, just to make sure it's explicit.

@oli-obk
Copy link
Contributor Author

oli-obk commented Apr 24, 2018

Note to self. more tests:

// ok
const X: FakeNeedsDrop = { let x = FakeNeedsDrop; x };
// error
const Y: FakeNeedsDrop = { let mut x = FakeNeedsDrop; x = FakeNeedsDrop; x };
// error
const Z: () = { let mut x = None; x = Some(FakeNeedsDrop); }

@pietroalbini pietroalbini added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 30, 2018
@shepmaster
Copy link
Member

Ping from triage @oli-obk ! Any time to work on this in the near future?

@rust-highfive

This comment has been minimized.

@pietroalbini pietroalbini added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 14, 2018
@pietroalbini
Copy link
Member

Ping from triage @eddyb! This PR needs your review.

@oli-obk
Copy link
Contributor Author

oli-obk commented May 14, 2018

#50603 takes priority over this PR, because it fixes a soundness hole (there will be conflicts, so we're waiting for #50603 to get merged first)

@pietroalbini pietroalbini added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 14, 2018
@kennytm
Copy link
Member

kennytm commented May 19, 2018

#50603 has been merged.

@kennytm kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels May 19, 2018
@oli-obk
Copy link
Contributor Author

oli-obk commented May 19, 2018

amazingly there are no conflicts whatsoever. @eddyb this is ready for another review

}
Place::Local(index) if self.mir.local_kind(index) == LocalKind::ReturnPointer => {
debug!("store to return place {:?}", index);
store(&mut self.return_qualif)
store(&mut self.local_qualif[RETURN_PLACE])
Copy link
Member

Choose a reason for hiding this comment

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

Can you combine these two match arms?

@eddyb
Copy link
Member

eddyb commented May 21, 2018

r=me with the nit fixed

@oli-obk
Copy link
Contributor Author

oli-obk commented May 22, 2018

@bors r=eddyb

@bors
Copy link
Contributor

bors commented May 22, 2018

📌 Commit 2483c81 has been approved by eddyb

@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 May 22, 2018
@bors
Copy link
Contributor

bors commented May 22, 2018

⌛ Testing commit 2483c81 with merge 9f80ea3...

bors added a commit that referenced this pull request May 22, 2018
Allow let bindings and destructuring in constants and const fn

r? @eddyb

cc #48821
@bors
Copy link
Contributor

bors commented May 22, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: eddyb
Pushing 9f80ea3 to master...

@bors bors merged commit 2483c81 into rust-lang:master May 22, 2018
@pietroalbini pietroalbini added the beta-nominated Nominated for backporting to the compiler in the beta channel. label May 24, 2018
@pietroalbini
Copy link
Member

This PR needs to be backported to beta in order to backport #50909.
Cause: #51037 (comment)

cc @rust-lang/compiler

@oli-obk oli-obk deleted the const_let branch May 24, 2018 18:38
@oli-obk
Copy link
Contributor Author

oli-obk commented May 24, 2018

i can create a version of #50909 that doesn't depend on this, it's just a debug statement that conflicts

@pietroalbini
Copy link
Member

@oli-obk that would be awesome!

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.

7 participants