allow constant evaluation of function calls #26848

oli-obk · 2015-07-07T11:12:30Z

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 · 2015-07-07T11:12:42Z

r? @pnkfelix

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

eddyb · 2015-07-07T11:19:33Z

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 · 2015-07-07T14:00:30Z

🗽 This is pretty neat!

eddyb · 2015-07-07T17:22:29Z

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 · 2015-07-07T17:24:39Z

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 · 2015-07-07T18:09:36Z

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 · 2015-07-07T18:17:38Z

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 · 2015-07-21T17:55:49Z

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

bors · 2015-07-22T16:46:28Z

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

Gankra · 2015-07-27T02:09:51Z

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

oli-obk · 2015-07-27T07:54:18Z

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.

eefriedman · 2015-07-27T08:34:25Z

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 · 2015-07-27T11:36:51Z

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 · 2015-07-27T23:22:26Z

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 · 2015-07-28T05:57:24Z

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 · 2015-07-28T22:57:17Z

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

pnkfelix · 2015-07-29T01:04:34Z

(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 · 2015-07-29T23:05:12Z

@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 · 2015-07-29T23:25:49Z

@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 · 2015-07-30T06:21:32Z

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 · 2015-07-30T15:23:48Z

RFC: rust-lang/rfcs#1229

steveklabnik · 2015-09-27T00:11:18Z

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

oli-obk · 2015-10-02T09:07:50Z

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 · 2015-10-02T21:43:55Z

Great!

On Fri, Oct 2, 2015 at 5:08 AM, Oliver Schneider [email protected]
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 · 2015-10-14T10:36:59Z

done and rebased

depends on PR #28845 (rfc 1229)

bors · 2015-10-18T13:04:20Z

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

oli-obk · 2015-10-19T11:16:12Z

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

So we cannot assume that the function call was marked NOT_CONST by check_const.

oli-obk · 2015-10-27T08:40:02Z

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 · 2015-10-27T17:04:49Z

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 · 2015-10-27T17:05:12Z

@bors r+

bors · 2015-10-27T17:05:13Z

📌 Commit 2b000fe has been approved by pnkfelix

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: ```rust 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 ```rust 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.

bors · 2015-10-27T17:11:15Z

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

bors · 2015-10-27T19:15:27Z

☀️ 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 · 2015-11-02T23:42:25Z

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 [email protected] 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 · 2015-11-05T06:21:40Z

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 · 2015-11-05T07:12:10Z

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

Manishearth · 2015-11-05T07:13:44Z

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 · 2015-11-05T07:15:08Z

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 · 2015-11-05T07:16:20Z

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 const_fn_const_eval branch from ef3593b to 6d1e92a Compare July 7, 2015 11:16

eddyb reviewed Jul 7, 2015
View reviewed changes

oli-obk force-pushed the const_fn_const_eval branch from 6d1e92a to 0e2388d Compare July 22, 2015 07:12

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

oli-obk reviewed Jul 27, 2015
View reviewed changes

oli-obk mentioned this pull request Jul 30, 2015

Const indexing and friends #25570

Merged

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

oli-obk added 2 commits October 19, 2015 13:13

allow constant evaluation of function calls

b4e30bd

copy paste error of stable attribute

72f42f1

oli-obk force-pushed the const_fn_const_eval branch from 084c171 to 72f42f1 Compare October 19, 2015 11:13

the const evaluator might run before check_const

2b000fe

So we cannot assume that the function call was marked NOT_CONST by check_const.

oli-obk reviewed Oct 27, 2015
View reviewed changes

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

oli-obk deleted the const_fn_const_eval branch October 28, 2015 08:19

dtwood reviewed Nov 2, 2015
View reviewed changes

allow constant evaluation of function calls #26848

allow constant evaluation of function calls #26848

Conversation

oli-obk commented Jul 7, 2015

rust-highfive commented Jul 7, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdm commented Jul 7, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddyb commented Jul 7, 2015

eefriedman commented Jul 7, 2015

oli-obk commented Jul 7, 2015

bors commented Jul 21, 2015

bors commented Jul 22, 2015

Gankra commented Jul 27, 2015

Choose a reason for hiding this comment

eefriedman commented Jul 27, 2015

glaebhoerl commented Jul 27, 2015

eefriedman commented Jul 27, 2015

oli-obk commented Jul 28, 2015

bors commented Jul 28, 2015

pnkfelix commented Jul 29, 2015

nikomatsakis commented Jul 29, 2015

pnkfelix commented Jul 29, 2015

oli-obk commented Jul 30, 2015

oli-obk commented Jul 30, 2015

steveklabnik commented Sep 27, 2015

oli-obk commented Oct 2, 2015

nikomatsakis commented Oct 2, 2015

oli-obk commented Oct 14, 2015

bors commented Oct 18, 2015

oli-obk commented Oct 19, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pnkfelix commented Oct 27, 2015

pnkfelix commented Oct 27, 2015

bors commented Oct 27, 2015

bors commented Oct 27, 2015

bors commented Oct 27, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Manishearth commented Nov 5, 2015

oli-obk commented Nov 5, 2015

Manishearth commented Nov 5, 2015

oli-obk commented Nov 5, 2015

oli-obk commented Nov 5, 2015