properly short circuit const eval of && and || #29879

shahn · 2015-11-17T01:49:56Z

As described in #29608, the rhs of a logical expression is evaluated
even if evaluation of the lhs means no such evaluation should take
place if that evaluation happens in a const context.

As described in rust-lang#29608, the rhs of a logical expression is evaluated even if evaluation of the lhs means no such evaluation should take place.

rust-highfive · 2015-11-17T01:50:11Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

oli-obk · 2015-11-17T09:12:53Z

that's very verbose. I first had the same Idea, but chose to move in a different direction: oli-obk@e427500

I was waiting on #29797 before doing this.

Does the following test work for you?

const N: usize = 0;
const ARR: [i32; N] = [42; N];
const X: bool = (N > 0) && (ARR[0] == 99);

fn main() {
    let _ = X;
}

and the following one?

const N: usize = 2;
const ARR: [i32; N] = [42; N];
const X: bool = (ARR[0] == 99) && (N == 2);

fn main() {
    let _ = X;
}

oli-obk · 2015-11-17T09:19:08Z

src/test/run-pass/issue-29608.rs

@@ -0,0 +1,17 @@
+// Copyright 2014 The Rust Project Developers. See the COPYRIGHT


shahn · 2015-11-17T09:43:42Z

The first one doesn't work indeed, the second does. Thanks for catching it. Happy to defer to your plans if you like them more :)

oli-obk · 2015-11-17T10:39:16Z

I do find my const_eval more readable, but it always evaluates the rhs (just doesn't report errors if there are any). Not sure how to move forward

For the trans/const issues I'm not sure how to solve them. You basically need to extract the boolean value from the llvm value. Which I couldn't figure out.

The second one works, because it's not short circuiting, since the first expression returns an Err in const_eval because array indexing is not implemented. The following example will not compile, while it should.

const N: usize = 2;
const ARR: [i32; N] = [42; N];
const X: bool = (ARR[0] == 99) && (ARR[99] == 1337);

fn main() {
    let _ = X;
}

alexcrichton · 2015-11-17T20:22:37Z

r? @nikomatsakis

Seems related to #29797

nikomatsakis · 2015-11-17T20:59:42Z

src/librustc_trans/trans/consts.rs

@@ -591,6 +591,21 @@ fn const_expr_unadjusted<'a, 'tcx>(cx: &CrateContext<'a, 'tcx>,
            let is_float = ty.is_fp();
            let signed = ty.is_signed();

+            if ty.is_bool() && (b.node == hir::BiOr || b.node == hir::BiAnd) {
+              match eval_const_expr_partial(cx.tcx(),


Unfortunately, this doesn't seem sufficient. The trans constant evaluator generally handles a wider set of conditions that eval_const_expr_partial, so it seems to me that this will result in short-circuiting sometimes working and sometimes (for more complex constants) not working, which doesn't seem like the ideal situation.

(I guess this would still be an improvement towards the eventual goal.)

yes you're right, see also @oli-obk 's comment. I would need to fix this better somehow, but don't yet know a good way.

It's really hard to extract the proper value from a llvm constant value. There's stackoverflow posts on it for doing it with c++

http://stackoverflow.com/questions/5315176/llvm-get-constant-integer-back-from-value
http://stackoverflow.com/questions/17255699/outputting-inputs-constant-char-array-from-llvm-pass
http://stackoverflow.com/questions/30314417/llvm-retrieving-the-value-of-constant-function-parameters-from-the-c-api

nikomatsakis · 2015-11-17T21:02:46Z

@oli-obk in your code, can't you adjust it pretty easily to avoid evaluating the RHS?

e.g.

        let a = try!(eval_const_expr_partial(tcx, &**a, ty_hint, fn_args));
       match (a, b, op.node) {
           // short circuit bool ops
           (Bool(false), _, BiAnd) => return Bool(false),
           (Bool(true), _, BiOr) => return Bool(true),
           _ => { }
       }
       // FIXME: lazy eval b?
       let b = eval_const_expr_partial(tcx, &**b, b_ty, fn_args);
       match (a, b, op.node) {
           (Bool(true), Ok(Bool(rhs)), BiAnd) => Bool(rhs),
           (Bool(false), Ok(Bool(rhs)), BiOr) => Bool(rhs),
            ... // as before
        }

or am I missing something?

bors · 2015-11-17T23:57:51Z

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

oli-obk · 2015-11-18T08:32:42Z

@nikomatsakis yea, that looks about right. I don't remember if anything spoke against that. I'll modify my code.

nikomatsakis · 2015-11-18T15:06:06Z

@oli-obk @shahn

yes you're right, see also @oli-obk 's comment. I would need to fix this better somehow, but don't yet know a good way.

I am torn about this. Do you think guys think it makes sense to move forward with a partial fix here? Or wait until we refactor the trans-based impl away? I'm not a big fan of trying really hard to make the trans-based code work, though maybe that IS more prudent. (Since I don't know whether anyone has time or bandwidth to spearhead an effort to reshape the constant evaluator, though @oli-obk given all your work in this area lately maybe we should try to touch base on that. I'd be eternally grateful. :))

oli-obk · 2015-11-18T15:25:11Z

I'm perfectly fine with not improving trans/consts.rs. I'd wouldn't invest time in extracting llvm stuff that's just going to get dropped anyway. Since a partial fix will just confuse a lot (see the test cases I provided above) I suggest to not implement this feature for trans/consts. I can also live with ignoring the short circuit issue completely until we get one unified const evaluator. This way there won't be any confusion.

WRT moving to one unified const evaluator, I can't give any promises, but it's likely I'll be posting a new PR whenever I get one merged, so at least there'll be movement forward.

nikomatsakis · 2015-11-18T15:34:28Z

@oli-obk

I can also live with ignoring the short circuit issue completely until we get one unified const evaluator. This way there won't be any confusion.

I could live with that too, though we ought to make some notes on the relevant GH issues.

WRT moving to one unified const evaluator, I can't give any promises, but it's likely I'll be posting a new PR whenever I get one merged, so at least there'll be movement forward.

This is tantalizing but I'm not sure what it means. :) Does this mean you have something in the works? Or you have a long queue of improvements on the way?

oli-obk · 2015-11-18T15:43:07Z

Does this mean you have something in the works? Or you have a long queue of improvements on the way?

I have plans. And a few horribly outdated commits to rebase. I can create a tracking issue if you want.

nikomatsakis · 2015-11-18T18:03:04Z

So it seems to me we ought to close this PR in favor of @oli-obk's branch, which I agree is a somewhat cleaner approach -- @shahn, I think you said you were ok with that? (thanks in any case for the effort)

shahn · 2015-11-18T18:13:48Z

Yes, that's OK with me of course. I agree that implementing partially correct behaviour might be more confusing than leaving it as is for now. Thanks for the review!

properly short circuit const eval of && and ||

fc0f890

As described in rust-lang#29608, the rhs of a logical expression is evaluated even if evaluation of the lhs means no such evaluation should take place.

rust-highfive assigned alexcrichton Nov 17, 2015

oli-obk reviewed Nov 17, 2015
View reviewed changes

src/test/run-pass/issue-29608.rs

@@ -0,0 +1,17 @@

// Copyright 2014 The Rust Project Developers. See the COPYRIGHT

Copy link

Contributor

oli-obk Nov 17, 2015

Choose a reason for hiding this comment

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

2015

rust-highfive assigned nikomatsakis and unassigned alexcrichton Nov 17, 2015

nikomatsakis reviewed Nov 17, 2015
View reviewed changes

shahn closed this Nov 18, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

properly short circuit const eval of && and || #29879

properly short circuit const eval of && and || #29879

shahn commented Nov 17, 2015

rust-highfive commented Nov 17, 2015

oli-obk commented Nov 17, 2015

oli-obk Nov 17, 2015

shahn commented Nov 17, 2015

oli-obk commented Nov 17, 2015

alexcrichton commented Nov 17, 2015

nikomatsakis Nov 17, 2015

nikomatsakis Nov 17, 2015

shahn Nov 17, 2015

oli-obk Nov 18, 2015

nikomatsakis commented Nov 17, 2015

bors commented Nov 17, 2015

oli-obk commented Nov 18, 2015

nikomatsakis commented Nov 18, 2015

oli-obk commented Nov 18, 2015

nikomatsakis commented Nov 18, 2015

oli-obk commented Nov 18, 2015

nikomatsakis commented Nov 18, 2015

shahn commented Nov 18, 2015

		@@ -0,0 +1,17 @@
		// Copyright 2014 The Rust Project Developers. See the COPYRIGHT

properly short circuit const eval of && and || #29879

properly short circuit const eval of && and || #29879

Conversation

shahn commented Nov 17, 2015

rust-highfive commented Nov 17, 2015

oli-obk commented Nov 17, 2015

oli-obk Nov 17, 2015

Choose a reason for hiding this comment

shahn commented Nov 17, 2015

oli-obk commented Nov 17, 2015

alexcrichton commented Nov 17, 2015

nikomatsakis Nov 17, 2015

Choose a reason for hiding this comment

nikomatsakis Nov 17, 2015

Choose a reason for hiding this comment

shahn Nov 17, 2015

Choose a reason for hiding this comment

oli-obk Nov 18, 2015

Choose a reason for hiding this comment

nikomatsakis commented Nov 17, 2015

bors commented Nov 17, 2015

oli-obk commented Nov 18, 2015

nikomatsakis commented Nov 18, 2015

oli-obk commented Nov 18, 2015

nikomatsakis commented Nov 18, 2015

oli-obk commented Nov 18, 2015

nikomatsakis commented Nov 18, 2015

shahn commented Nov 18, 2015