allow constant evaluation of function calls #26848

oli-obk · Jul 7, 2015

this has the funky side-effect of also allowing constant evaluation of function calls to functions that are not const fn as long as check_const didn't mark that function NOT_CONST

It's still not possible to call a normal function from a const fn, but let statements' initialization value can get const evaluated (this caused the fallout in the overflowing tests)

we can now do this:

const fn add(x: usize, y: usize) -> usize { x + y }
const ARR: [i32; add(1, 2)] = [5, 6, 7];

also added a test for destructuring in const fn args

const fn i((a, b): (u32, u32)) -> u32 { a + b } //~ ERROR: E0022

This is a [breaking change], since it turns some runtime panics into compile-time errors. This statement is true for ANY improvement to the const evaluator.

rust-highfive · Jul 7, 2015

r? @pnkfelix

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

eddyb · Jul 7, 2015

eddyb reviewed Jul 7, 2015

View changes

eddyb · Jul 7, 2015

src/librustc/middle/const_eval.rs

@@ -271,6 +271,7 @@ pub enum ConstVal {
    Bool(bool),
    Struct(ast::NodeId),
    Tuple(ast::NodeId),
+    Function(ast::DefId),


Why not evaluate const fn calls eagerly?

I was just following the Struct/Tuple definition. And it made things easier with Expr::Call

Oh, this is a path to a function, not a call. Don't mind me!

jdm · Jul 7, 2015

🗽 This is pretty neat!

eddyb · Jul 7, 2015

eddyb reviewed Jul 7, 2015

View changes

eddyb · Jul 7, 2015

src/librustc/middle/const_eval.rs

+                  }
+              },
+              Some(def::DefFn(id, _)) => return Ok(Function(id)),
+              // FIXME: implement const methods?


This should at least check that the function is const fn - check_const runs long after type collection - and type collection is what triggers the evaluation of array length expressions.

that is unfortunate... but for all practical purposes irrelevant, since the function is simply interpreted as const fn and if it hits a non-const value anywhere const_eval bails out anyway. I'd rather keep const eval for all functions, so regular statements calling non-const-fn-but-could-be-const-fn functions with const arguments can also be const evaluated

what does your PR do if it attempts to eval a call to a non const fn whose body is more complex than a single expression?

From my initial reading of the PR, it seems like it is assuming that it is always given a const fn and thus can assume that the block.expr.as_ref().unwrap() is all it needs to look at in the body, in particular it is not (I think) double-checking that the block has no statements...

Update: oh wait, let me double check, but now I'm thinking that all such functions should have already been marked as NOT_CONST from the check_const run.

Update 2: But wait, as eddyb already said, check_const runs long after type collection ... so it still seems like you could mistakenly think you are evaluating a non-const fn correctly but in fact should have flagged the fn as non-const on-the-fly...

yea, I misunderstood. I will force const-fn here. This should also un-break all the unit tests :D

eddyb · Jul 7, 2015

Maybe it might be a good idea to introduce a const evaluation context that includes substitutions for type parameters and for function arguments, instead of passing these around manually.

eefriedman · Jul 7, 2015

Is there any plan to fix rustc's usage of const_eval so it doesn't try to evaluate expressions that haven't gone through type-checking first? Fixing that would massively simplify const_eval, and make changes like this a lot easier to reason about. (See also #26683.)

For the tests, probably should just implement "id" as a store+load from a static mut variable, which is basically guaranteed to be impossible to const-evaluate.

oli-obk · Jul 7, 2015

Maybe it might be a good idea to introduce a const evaluation context that includes substitutions for type parameters and for function arguments, instead of passing these around manually.

yea, I had similar thoughts, but then got too lazy to introduce a PR before this one, but if preferred I can do that once @eefriedman gets his hint PR through

bors · Jul 21, 2015

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

bors · Jul 22, 2015

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

Gankro · Jul 27, 2015

What's the status on this? Just needs review? Blocked on @eefriedman's PR?

oli-obk · Jul 27, 2015

oli-obk reviewed Jul 27, 2015

View changes

oli-obk · Jul 27, 2015

src/librustc/middle/const_eval.rs

+            unsafety,
+            abi,
+            block,
+          ) = match try!(eval_const_expr_partial(tcx, callee, UncheckedExprNoHint, fn_args)) {


@eefriedman this eval_const_expr_partial gets me the DefId of the function I'm calling. I'm not sure if anything but UncheckedExprNoHint makes sense here.

oli-obk · Jul 27, 2015

oli-obk reviewed Jul 27, 2015

View changes

oli-obk · Jul 27, 2015

src/librustc/middle/const_eval.rs

+          }
+          let result = block.expr.as_ref().unwrap();
+          debug!("const call({:?})", call_args);
+          try!(eval_const_expr_partial(tcx, &**result, ty_hint, Some(&call_args)))


this ty_hint is just forwarded, as the type of the call expression is the same as the type of the functions body-expression

oli-obk · Jul 27, 2015

oli-obk reviewed Jul 27, 2015

View changes

oli-obk · Jul 27, 2015

src/librustc/middle/const_eval.rs

+              let arg_val = try!(eval_const_expr_partial(
+                  tcx,
+                  arg_expr,
+                  UncheckedExprNoHint,


I only know about the ast-type, not the actual type-checked type. I could of course check the appropriate tables if the type is known.

eefriedman · Jul 27, 2015

I'll look at this more carefully tomorrow, but the way hints work inside of eval_const_expr_partial is that if the argument is ExprTypeChecked, all recursive calls should generally also be ExprTypeChecked. This limits the use of weird hinting semantics to places where it's absolutely necessary.

glaebhoerl · Jul 27, 2015

this has the funky side-effect of also allowing constant evaluation of function calls to functions that are not const fn as long as check_const didn't mark that function NOT_CONST

Am I understanding correctly that this doesn't magically turn every fn that accidentally happens to meet the requirements of a const fn into an actual const fn, i.e. a normal fn still can't be used where a compile-time constant is required, rather this is basically for early-evaluation as an optimization in cases where evaluation would normally happen at runtime? If that's right, it's not as scary as it might've been, though it still seems preferable to bail out with a warning if doing this runs into any problems (overflow, divergence, ...)?

eefriedman · Jul 27, 2015

Looking over this more carefully, I'm pretty sure this approach is going to exacerbate the existing problems with overflow in constant evaluation in type-checking. You simply can't correctly evaluate a const fn correctly in general without knowing the declared argument and result types. For example:

const fn bmax() -> u8 { !0 }
const fn smax() -> u16 { !0 }
let c: [u8; bmax() as usize] = [0; 255];
let c: [u8; smax() as usize] = [0; 65535];

oli-obk · Jul 28, 2015

I will add that and similar functions with function arguments as test cases. For now I will raise an error in case the type is not yet in the ast_ty_to_ty map. I wonder how this plays with generics...

bors · Jul 28, 2015

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

pnkfelix · Jul 29, 2015

(I'm looking through this now, but this seems like this may require a bit more review and/or revision compared to e.g. #25570 which seemed pretty clean to me.)

cc @jroesch

nikomatsakis · Jul 29, 2015

@oli-obk

This is a [breaking change], since it turns some runtime panics into compile-time errors. This statement is true for ANY improvement to the const evaluator.

can you elaborate on what you mean here? Is this specifically because you were treating arbitrary fns as const fns?

pnkfelix · Jul 29, 2015

@oli-obk wrote:

This is a [breaking change], since it turns some runtime panics into compile-time errors. This statement is true for ANY improvement to the const evaluator.

@nikomatsakis asked:

can you elaborate on what you mean here? Is this specifically because you were treating arbitrary fns as const fns?

Niko and I then reviewed the situation further, and determined the following:

The hypothesized property (that any improvement to the const evaluator is a breaking change) is not, in general, about the fact that this PR treated arbitrary fns as const fns.
In particular, while the particular test cases where this PR observed the described phenomenon were cases of arbitrary fns, those test cases could (as originally written) very well have been written using const fn instead of fn, and this PR as written would have the same effect (of turning a runtime-panic into a compile-time error).
To be concrete, we're talking about something like this (which is a small modification of an existing test).
```
// error-pattern:thread '<main>' panicked at 'shift operation overflowed'
// compile-flags: -C debug-assertions

// (Work around constant-evaluation)
const fn id<T>(x: T) -> T { x }

fn main() {
    let _x = -1_i32 >> id(32);
}
```
before this PR, the above test would panic at runtime; after this PR, it would be a compile-time failure.
Nonetheless, @nikomatsakis has convinced me that that this treatment of the above case is wrong. Instead, when encountering this scenario under -C debug-assertions, we should generate the code to panic at runtime. Also emitting a compile-time warning (i.e. a lint) in that scenario would be acceptable, but not an outright compile-time error.

If you follow that policy (of emitting code to panic at runtime -- which I guess you could emulate by just not const-evaluating the overflowing cases), then the hypothesis no longer holds: This would no longer be a breaking-change, and it would re-enforce a precedent that improvements to the const-evaluator should not be breaking changes.

oli-obk · Jul 30, 2015

Is this specifically because you were treating arbitrary fns as const fns?

yes, but it applies also to my other branch about constant evaluation of indexing operations or any further operations or constant values that are added (e.g. Ranges).

we should generate the code to panic at runtime. Also emitting a compile-time warning (i.e. a lint) in that scenario would be acceptable, but not an outright compile-time error.

I'll create an RFC to make this the standard procedure for compiler-improvements that detect an error at compile-time, but insert an unconditional runtime-panic to not be a breaking change.

emitting code to panic at runtime -- which I guess you could emulate by just not const-evaluating the overflowing cases

This would require 2 things

let the const evaluator know whether it is an a true const environment (Array Length or a constant's value)
All new operations (const call, const index, ...) need to poison the const-evaluator to turn a compiler-error into some const-eval-error that is caught wherever normal code was fed to the const-evaluator, emits a warning "unconditional panic inserted because {}" and inserts the panic. I would not actually call regular code-gen, but simply insert the panic. But this should be discussed in the RFC.

oli-obk · Jul 30, 2015

RFC: rust-lang/rfcs#1229

steveklabnik · Sep 27, 2015

This RFC was merged, any chance the PR can get rebased?

oli-obk · Oct 2, 2015

I implemented the RFC, there'll be a PR for it soon, after that, I'll rebase this PR over the RFC-PR and add the appropriate regression tests.

nikomatsakis · Oct 2, 2015

Great!

On Fri, Oct 2, 2015 at 5:08 AM, Oliver Schneider notifications@github.com
wrote:

I implemented the RFC, there'll be a PR for it soon, after that, I'll
rebase this PR over the RFC-PR and add the appropriate regression tests.

—
Reply to this email directly or view it on GitHub
#26848 (comment).

oli-obk · Oct 14, 2015

done and rebased

depends on PR #28845 (rfc 1229)

bors · Oct 18, 2015

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

oli-obk · Oct 19, 2015

now that #28845 (RFC 1229) has landed, we can merge this without causing breaking changes

oli-obk · Oct 27, 2015

oli-obk reviewed Oct 27, 2015

View changes

oli-obk · Oct 27, 2015

src/librustc/middle/const_eval.rs

+              },
+              _ => signal!(e, NonConstPath),
+          };
+          if let ExprTypeChecked = ty_hint {


@eddyb's comment is addressed here

thank you for pointing this out.

pnkfelix · Oct 27, 2015

pnkfelix reviewed Oct 27, 2015

View changes

pnkfelix · Oct 27, 2015

src/test/run-pass/shift-near-oflo.rs

@@ -13,9 +13,6 @@
 // Check that we do *not* overflow on a number of edge cases.
 // (compare with test/run-fail/overflowing-{lsh,rsh}*.rs)

-// (Work around constant-evaluation)
-fn id<T>(x: T) -> T { x }


(I'm not sure if this was a good idea. The point of this test was to exercise the code generated for execution at runtime, but with this change, you are heavily encouraging it to be evaluated at compile time. I would probably have preferred to just leave this as it was, perhaps even modifying the fn id further to try to ensure that it would not be flagged as a const fn by future aggressive analyses...)

Yes, I was overzealous here.

The problem is, that this test gets silently turned into a compile-time test whenever we improve the const evaluator. I think if we made the id function in a way that it modifies a static on every call, there's no way this can ever be evaluated at compile-time in rustc. LLVM will probably still end up optimizing everything out. So this test also needs to guarantee that optimizations are off (are they even on in tests? if they are not, ever activating them will again silently turn this test into a compile-time test).

An alternative is to add a text file from which the values are read. This way optimizations and the const evaluator cannot ever influence the meaning of this test.

I don't care as much about LLVM optimizing everything out (my main intent is to test the rust codegen paths) -- but having said that, you are probably right that it would be good to prevent LLVM as well.

Still, would marking the fn as '#[inline(never)]` not suffice here?

oh right, we have that. I'll create a new workaround function and submit a PR.

pnkfelix · Oct 27, 2015

Despite my misgivings about the change to the run-pass/shift-near-oflo.rs test, I will approve this to get it into the bors queue (and out of my review queue).

I would nonetheless be interested in hearing @oli-obk 's opinion on whether that change to the test should stand, or if a followup PR should perhaps return it to its former state (or something similar to that).

pnkfelix · Oct 27, 2015

@bors r+

bors · Oct 27, 2015

📌 Commit 2b000fe has been approved by pnkfelix

bors · Oct 27, 2015

⌛️ Testing commit 2b000fe with merge 540fd3a...

bors · Oct 27, 2015

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt

dtwood · Nov 2, 2015

dtwood reviewed Nov 2, 2015

View changes

dtwood · Nov 2, 2015

src/librustc/middle/const_eval.rs

+          } else {
+              // we don't know much about the function, so we force it to be a const fn
+              // so compilation will fail later in case the const fn's body is not const
+              assert_eq!(constness, hir::Constness::Const)


I'm getting a ICE here with the current nightly on playpen:

thread 'rustc' panicked at 'assertion failed: `(left == right)` (left: `NotConst`, right: `Const`)', ../src/librustc/middle/const_eval.rs:106

in this simple test program:

fn f(x: usize) -> usize { x } const Y: usize = f(2); fn main() { let z = [0; X]; }

Curiously if you move the const Y... line to the top, then it at least doesn't ICE, although I'd expect to get E0015 as an error, rather than E0307 (you can then get the E0015 by removing the let z = ... line)

This isn't the right place to file a bug report; try https://github.com/rust-lang/rust/issues/new . (You can link to the PR from there if you think it's useful.)

I've just been playing on my phone on playpen on the bus, and won't have a laptop for at least a week, so was just hoping to put this somewhere that someone would spot it, and they can triage it (otherwise I'll just forget)

Filed #29587

Great, thanks

On Wed, 4 Nov 2015 at 20:46 eefriedman notifications@github.com wrote:

In src/librustc/middle/const_eval.rs
#26848 (comment):

},

_ => signal!(e, NonConstPath),

}

} else {

signal!(e, NonConstPath)

},

_ => signal!(e, NonConstPath),

};

if let ExprTypeChecked = ty_hint {

// no need to check for constness... either check_const

// already forbids this or we const eval over whatever

// we want

} else {

// we don't know much about the function, so we force it to be a const fn

// so compilation will fail later in case the const fn's body is not const

assert_eq!(constness, hir::Constness::Const)

Filed #29587 #29587

—
Reply to this email directly or view it on GitHub
https://github.com/rust-lang/rust/pull/26848/files#r43936838.

Manishearth · Nov 5, 2015

Won't this introduce a backcompat hazard? Functions which are const but not marked as such may become non-const in the future, breaking libs which depend on this.

oli-obk · Nov 5, 2015

@Manishearth it's impossible to use non-const functions in a const environment

Manishearth · Nov 5, 2015

I'm talking about

this has the funky side-effect of also allowing constant evaluation of function calls to functions that are not const fn as long as check_const didn't mark that function NOT_CONST

oli-obk · Nov 5, 2015

yes, normal code can be const evaluated. But in case the const evaluation fails, it falls back to normal code generation anyway. I had an RFC for that and implemented it a while back.

oli-obk · Nov 5, 2015

as you can see in #29587, there's actually an assertion preventing the use of non-const functions in a true const environment.

rust-highfive assigned pnkfelix Jul 7, 2015

oli-obk force-pushed the oli-obk:const_fn_const_eval branch from ef3593b to 6d1e92a Jul 7, 2015

oli-obk force-pushed the oli-obk:const_fn_const_eval branch from 6d1e92a to 0e2388d Jul 22, 2015

oli-obk force-pushed the oli-obk:const_fn_const_eval branch 3 times, most recently from e41da4c to 2e750e6 Jul 27, 2015

oli-obk referenced this pull request Jul 30, 2015
Merged
Const indexing and friends #25570

oli-obk force-pushed the oli-obk:const_fn_const_eval branch 2 times, most recently from 0c978cb to 084c171 Oct 14, 2015

oli-obk added some commits Oct 14, 2015

allow constant evaluation of function calls

b4e30bd

copy paste error of stable attribute

Loading status checks…

72f42f1

oli-obk force-pushed the oli-obk:const_fn_const_eval branch from 084c171 to 72f42f1 Oct 19, 2015

the const evaluator might run before check_const
So we cannot assume that the function call was marked NOT_CONST by check_const.

Loading status checks…

2b000fe

bors merged commit 2b000fe into rust-lang:master Oct 27, 2015
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

oli-obk deleted the oli-obk:const_fn_const_eval branch Oct 28, 2015

rust-lang/rust

Join GitHub today

allow constant evaluation of function calls #26848

Conversation

oli-obk commented Jul 7, 2015

rust-highfive assigned pnkfelix Jul 7, 2015

This comment has been minimized.

rust-highfive commented Jul 7, 2015

oli-obk force-pushed the oli-obk:const_fn_const_eval branch from ef3593b to 6d1e92a Jul 7, 2015

eddyb reviewed Jul 7, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jdm commented Jul 7, 2015

eddyb reviewed Jul 7, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

eddyb commented Jul 7, 2015

This comment has been minimized.

eefriedman commented Jul 7, 2015

This comment has been minimized.

oli-obk commented Jul 7, 2015

This comment has been minimized.

bors commented Jul 21, 2015

oli-obk force-pushed the oli-obk:const_fn_const_eval branch from 6d1e92a to 0e2388d Jul 22, 2015

This comment has been minimized.

bors commented Jul 22, 2015

This comment has been minimized.

Gankro commented Jul 27, 2015

oli-obk force-pushed the oli-obk:const_fn_const_eval branch 3 times, most recently from e41da4c to 2e750e6 Jul 27, 2015

oli-obk reviewed Jul 27, 2015 View changes

This comment has been minimized.

oli-obk reviewed Jul 27, 2015 View changes

This comment has been minimized.

oli-obk reviewed Jul 27, 2015 View changes

This comment has been minimized.

This comment has been minimized.

eefriedman commented Jul 27, 2015

This comment has been minimized.

glaebhoerl commented Jul 27, 2015

This comment has been minimized.

eefriedman commented Jul 27, 2015

This comment has been minimized.

oli-obk commented Jul 28, 2015

This comment has been minimized.

bors commented Jul 28, 2015

This comment has been minimized.

pnkfelix commented Jul 29, 2015

This comment has been minimized.

nikomatsakis commented Jul 29, 2015

This comment has been minimized.

pnkfelix commented Jul 29, 2015

oli-obk referenced this pull request Jul 30, 2015

This comment has been minimized.

oli-obk commented Jul 30, 2015

This comment has been minimized.

oli-obk commented Jul 30, 2015

This comment has been minimized.

steveklabnik commented Sep 27, 2015

This comment has been minimized.

oli-obk commented Oct 2, 2015

This comment has been minimized.

nikomatsakis commented Oct 2, 2015

oli-obk force-pushed the oli-obk:const_fn_const_eval branch 2 times, most recently from 0c978cb to 084c171 Oct 14, 2015

This comment has been minimized.

oli-obk commented Oct 14, 2015

This comment has been minimized.

bors commented Oct 18, 2015

oli-obk added some commits Oct 14, 2015

oli-obk force-pushed the oli-obk:const_fn_const_eval branch from 084c171 to 72f42f1 Oct 19, 2015

This comment has been minimized.

oli-obk commented Oct 19, 2015

oli-obk reviewed Oct 27, 2015 View changes

This comment has been minimized.

This comment has been minimized.

pnkfelix reviewed Oct 27, 2015 View changes

oli-obk force-pushed the oli-obk:const_fn_const_eval branch from `ef3593b` to `6d1e92a` Jul 7, 2015

eddyb reviewed Jul 7, 2015

View changes

eddyb reviewed Jul 7, 2015

View changes

oli-obk force-pushed the oli-obk:const_fn_const_eval branch from `6d1e92a` to `0e2388d` Jul 22, 2015

oli-obk force-pushed the oli-obk:const_fn_const_eval branch 3 times, most recently from `e41da4c` to `2e750e6` Jul 27, 2015

oli-obk reviewed Jul 27, 2015

View changes

oli-obk reviewed Jul 27, 2015

View changes

oli-obk reviewed Jul 27, 2015

View changes

oli-obk force-pushed the oli-obk:const_fn_const_eval branch 2 times, most recently from `0c978cb` to `084c171` Oct 14, 2015

oli-obk force-pushed the oli-obk:const_fn_const_eval branch from `084c171` to `72f42f1` Oct 19, 2015

oli-obk reviewed Oct 27, 2015

View changes

pnkfelix reviewed Oct 27, 2015

View changes

bors merged commit `2b000fe` into rust-lang:master Oct 27, 2015
2 checks passed

dtwood reviewed Nov 2, 2015

View changes