turn statically known erroneous code into a warning and continue normal code-generation

[oli-obk]

This RFC has changed course. The original intent is still noted at the bottom of this post.

If the constant evaluator encounters erronous code during the evaluation of
an expression that is not part of a true constant evaluation context a warning
must be emitted and the expression needs to be translated normally.

Currently the compiler errors in some situations like let x = 5 << 42; but not in others like let x = 5 << some_function_returning_42();

There are three levels of breaking changes (in the order of my preference and severity) that could be applied here:

1. always error when an error is detected at compile-time
2. always warn when an error is detected at compile-time and replace the code by an unconditional panic
3. always warn when an error is detected at compile-time and continue code-generation without the constant evaluator.

Variant 3 is the one now suggested by this RFC.

---

Old RFC: To prevent breaking changes, the compiler should turn statically known erroneous code (that previously wasn't a compile-time error) into an unconditional runtime-panic and issue a warning.

Rendered (dead link)

Rendered (official repo)

[pnkfelix]

Well, one easy way to implement it is: If you detect the error situation, then emit the warning, and then abandon the constant folding for that expression (that is, continue to emit the code that will run the expression at runtime, thus hitting the error condition when/if it runs).

Right?

Update: Oh, I didn't read carefully enough -- my strategy above (which I think is already included in the Alteratives section) differs from that of the RFC in that the above strategy will only work for -C debug-assertions mode builds, not release builds

[oli-obk]

you also need to detect that you are not in a true const environment, where you definitely want this to happen and there can't be breakage since the feature wasn't there before

[pnkfelix]

cc @rust-lang/compiler

[nikomatsakis]

I certainly agree with the sentiment of this RFC, but I think I prefer the "constant evaluation does not influence code generation" alternative. That is, I think the fact that we sometimes constant fold expressions is not something which should be observable to an end user (except via a warning) -- it's just a runtime optimization for better performance.

[nikomatsakis]

It's probably worth adding to the RFC a definition of "constant evaluation" vs "runtime evaluation" contexts. I think that the language has a pretty clear distinction, though:

• the initializer of a constant const foo: ty = EXPR or static foo: ty = EXPR
• the size of an array [T; EXPR]

The body of a const fn is sort of interesting. I think my preference is that the body is treated as a constant evaluation context, and hence an error is reported, even if the const fn is never called from a constant initializer -- so long as the overflow can be observed without knowing the values for these arguments. Implementing this, of course, will require a more sophisticated handling of constants (which we need anyhow). Since const fn is feature-gated we have time to play with this. Similar concerns will apply to generic and associated constants of course. I think from the POV of this RFC we can leave this as an "unresolved question", or at least as an open bug that will not be impl'd immediately.

[oli-obk]

I think the fact that we sometimes constant fold expressions is not something which should be observable to an end user (except via a warning) -- it's just a runtime optimization for better performance.

I mostly agree. Rust has debug_assert and compiler-inserted debug assertions in debug builds or with the -C debug-assertions flags enabled. These are disabled by default in release mode because they have a runtime-penalty. In case it is statically known that such an assertion would be triggered if it were enabled, then it should also be there in release mode, since the code already compiles to implementation defined machine code (As far as I know Rust makes no guarantees about code containing overflow or overbitshift except that it still is memory safe).

[nikomatsakis]

@oli-obk

The overflow RFC states that, when overflow checking is not enabled (i.e., in release builds), "erroneous operations will produce a value". I do not believe there is any provision in the RFC for us to cause a panic to occur unless overflow checks have been enabled. (Earlier drafts did explicitly permit this, however.)

My main concern here is causing code to break that used to work (because it relied on the integer overflow fallback semantics). I realize that such code is "officially" buggy, but it costs us nothing (in terms of maintenance burden, etc) to simply keep producing the same code we would have produced had the expression not been constant, and it will probably prevent some code from randomly panic'ing after a rustup. (Issuing a warning is nice, though.)

[oli-obk]

Implementations are permitted to check for overflow at any time (statically or dynamically)

It is the programmers responsibility to avoid these error conditions: any failure to do so can be considered a bug, and hence can be flagged by a static/dynamic analysis tools as an error.

These two statements sound to me as if it were legal to actually cause a compiler-error when such a bug is encountered. While this would be my preferred handling of such situations, I can see (and accept without argument) how such breaking changes should not happen suddenly after a minor compiler-upgrade.

My issue is that a library user needs to opt-in to all of the overflow panics or none when compiling the library, while the default could be "all statically proven overflow panics".

[oli-obk]

It's probably worth adding to the RFC a definition of "constant evaluation" vs "runtime evaluation" contexts.

done

I think from the POV of this RFC we can leave this as an "unresolved question", or at least as an open bug that will not be impl'd immediately.

added an unresolved question about const fn.

[eefriedman]

This isn't a complete list... off the top of my head, patterns and enum discriminants are also required to be constant.

[oli-obk]

fixed

[pnkfelix]

(brief discussion leads me to think that the context of a body of a const fn is inherited from the context of the call-site. (This is mostly a note to myself; I'll try to write something more coherent later.)

[arielb1]

This is also relevant for our erroring on transmute.

[arielb1]

Pre-execution constants (array lengths, enum variant discriminants, const, static, and probably some places I've missed) must have a result after compilation is over. Therefore, panics and UB here must occur during compilation (as controlled compiler errors, not rustc panicking/UB-ing, of course). There should not be any question about that.

The more interesting case is panics/UB in ordinary code. If the code isn't actually reached at runtime, this can be completely fine, and may occur as a result of macros or potentially generics, e.g.

fn elts_per_page<T>() -> usize {
    if mem::size_of::<T>() == 0 {
        usize::MAX
    } else {
        4096 / mem::size_of::<T>()
    }
}

or a similar version of using macros. It would be very annoying if overly-eager evaluation makes that a compile-time error. Note that this is what currently happens with transmute: transmuting between types of different sizes is obviously UB, but it currently causes a trans-time error. The attempts to go down this path with SIMD (#1199) weren't particularly well-received. Polymorphic recursion is somewhat similar too, but recursion limit issues are always non-compositional.

[oli-obk]

It would be very annoying if overly-eager evaluation makes that a compile-time error.

it wouldn't occur, If we enable if/else in the constant evaluator, only the true branch will be evaluated.

if you wrapped the else branch in a const-fn that would be a different issue though, as that function should also be evaluated on its own.

[nikomatsakis]

@oli-obk

it wouldn't occur, If we enable if/else in the constant evaluator, only the true branch will be evaluated.

This is of course specific to @arielb1's example. There can easily be conditions which are always false, though we cannot know it statically. But regardless the intent of this RFC is that such code would be accepted, though it would get a warning, right?

UPDATE: adding the ", right?" at the end :)

[nikomatsakis]

@oli-obk can you adjust the RFC to the alternative of "do not influence codegen"? I still think that is the preferred alternative. (After some discussion with others on the @rust-lang/lang team, I believe they are largely in agreement, though I'm not 100% sure -- if others disagree, please say so.)

[oli-obk]

But regardless the intent of this RFC is that such code would be accepted, though it would get a warning, right?

The code would get a warning currently (if there were a const fn sizeof), but an improved const evaluator (that can evaluate if) would again remove the warning.

can you adjust the RFC to the alternative of "do not influence codegen"?

sure thing, if the preference ever changes, it'll be as trivial as turning the lint into forbid

[pnkfelix]

Update: ugh so sorry, I was looking at a cached page.

Update 2: wait, I don't seem to be seeing an update that actually adopts the alternative... Raar github!

@oli-obk I think the lang team is more inclined to accept the "constant evaluation does not influence code generation" Alternative.

Would you be willing to revise the RFC text so that that alternative becomes the plan of record?

(If you are not willing, please let us know, so that we can figure out next steps in that scenario.)

[oli-obk]

updated title and message at the top

[nikomatsakis]

Hear ye, hear ye. This RFC is entering final comment period.

[oli-obk]

These warnings should always be displayed. I don't see a reason to ever allow them. Is there any equivalent to forbid that enforces that a warning can't be changed to allow, but only to deny/forbid?

[ssokolow]

@oli-obk: I'd be careful going down that road. When I was younger and dumber, my response to a warning I couldn't silence wasn't "Hmm. Why shouldn't this be silenced?" but, instead, a burst of annoyance and, in a burst of semi-spiteful productivity, a wrapper which added 2>&1 | grep -v 'warning text' (or equivalent) and possibly # Silence warning made unsilenceable by incompetent devs.

At the very least, I'd make it very clear that it's some kind of deprecation warning and, thus, it can't be silenced because it's really an error that's just cutting you some slack by giving you some catch-up time.

[oli-obk]

At the very least, I'd make it very clear that it's some kind of deprecation warning and, thus, it can't be silenced because it's really an error that's just cutting you some slack by giving you some catch-up time.

I'll make sure to add to the warning message that this is in fact an erroneous but well-defined situation (see #1237 for details on the well-defined-ness).

[oli-obk]

I just tried to implement this RFC... And hit a curious issue. There's actually no const evaluation happening until the lint-passes run: https://github.com/rust-lang/rust/blob/master/src/librustc_lint/builtin.rs#L174-L178 Is it really a breaking change to improve a lint? IIRC #1105 says that if you could have prevented the situation by changing a lint-level or using UFCs, the breaking change is fine.

The more we improve the const evaluator, the more time this lint will end up using, especially in the non-error case. It would be better to actually remove the lint and fold those const evaluable expressions so llvm has to do less work.

A few things could be done here, but the only true way forward is to wait for MIR, port const_eval to MIR, fold all constant expressions in MIR and report errors/warnings during the folding, and finally remove the lint as it is now duplication.

[arielb1]

@oli-obk

The thing here is that various places in rustc evaluate const-exprs, and the first place that fails reports the error.

The first place that evaluates const-exprs is typeck in type-level expressions (I am using -Z treat-err-as-bug to pinpoint the errors):

fn main() {
    let x: Option<[u8; 1/0]> = None;
}

Error:

$ RUST_BACKTRACE=1 rustc -Z treat-err-as-bug foo.rs
foo.rs:2:24: 3:27 error: internal compiler error: array length constant evaluation error: attempted to divide by zero
foo.rs:2     let x: Option<[u8; 1/0]> = None;
                                ^~~
<snip>
   7:     0x7fa1094ad367 - session::Session::span_err_with_code::h9bb47c81beb44685kGr
   8:     0x7fa10ae56eff - astconv::ast_ty_to_ty::he0a6db80093cc897Z8v
   9:     0x7fa10aea5eee - astconv::ast_ty_arg_to_ty::h4b63c64f7c8feadc7Zv
  10:     0x7fa10aea5b56 - vec::Vec<T>.FromIterator<T>::from_iter::h7691401690500070843
  11:     0x7fa10aea2af7 - astconv::convert_angle_bracketed_parameters::hd78cf94c1c4a6735K2u
  12:     0x7fa10ae55757 - astconv::ast_path_substs_for_ty::h5630483a91fb604bXLu
  13:     0x7fa10aeaaa3f - astconv::ast_path_to_ty::he16254360eff8a4aJrv
  14:     0x7fa10ae97b5f - astconv::finish_resolving_def_to_ty::h548b2bf1a94ff87fb7v
  15:     0x7fa10ae575b4 - astconv::ast_ty_to_ty::he0a6db80093cc897Z8v
  16:     0x7fa10ae494ea - check::GatherLocalsVisitor<'a, 'tcx>.Visitor<'tcx>::visit_local::h639702fd67237370ESn
  17:     0x7fa10ae2cf45 - check::check_fn::h3fcdd81a07ce16bbrZn
  18:     0x7fa10ae44e98 - check::check_bare_fn::h12de780dac587f1d1On
  19:     0x7fa10ae42dc2 - check::check_item_body::h481313b4e0932aa5Ufo
  20:     0x7fa10ae44ab4 - check::check_item_types::h4ee0d91dc68ee41byMn
  21:     0x7fa10af006f9 - check_crate::hed55c46ad8cec60dw8C
  22:     0x7fa10bbc2c9c - driver::phase_3_run_analysis_passes::he94da8ae9035992aGGa

The second place is const_check, which evaluates divisions:

fn main() {
    let x = (0u8-1u8)/1;
}

Error:

$ RUST_BACKTRACE=1 rustc -Z treat-err-as-bug foo.rs
foo.rs:2:14: 2:21 error: internal compiler error: attempted to sub with overflow in a constant expression
foo.rs:2     let x = (0u8-1u8)/1;
                      ^~~~~~~
<snip>
   7:     0x7f0e14929367 - session::Session::span_err_with_code::h9bb47c81beb44685kGr
   8:     0x7f0e14a6dd4c - middle::check_const::CheckCrateVisitor<'a, 'tcx>.Visitor<'v>::visit_expr::h6d2359b0311a538foke
   9:     0x7f0e14a6f893 - middle::check_const::CheckCrateVisitor<'a, 'tcx>.Visitor<'v>::visit_block::h7c5fa31839a19fd6bie
  10:     0x7f0e14a6e8e0 - middle::check_const::CheckCrateVisitor<'a, 'tcx>::fn_like::closure.81125
  11:     0x7f0e14a6e6d1 - middle::check_const::CheckCrateVisitor<'a, 'tcx>::fn_like::he96168b2665b6768B1d
  12:     0x7f0e14a7412f - visit::walk_item::h13051231923695214486
  13:     0x7f0e14a73d5e - middle::check_const::CheckCrateVisitor<'a, 'tcx>.Visitor<'v>::visit_item::heaafb7200b0245caUbe
  14:     0x7f0e14a77a0a - middle::check_const::check_crate::hd10c1c21df338568sPe
  15:     0x7f0e1703ecdb - driver::phase_3_run_analysis_passes::he94da8ae9035992aGGa

Third in line is the type_limits lint, which only cares about the value of shifts:

fn main() {
    let _x = 1<<(0u8-1u8);
    let _y = 1<<64;
}

Error:

$ RUST_BACKTRACE=1 rustc -Z treat-err-as-bug foo.rs
foo.rs:3:14: 3:19 error: internal compiler error: bitshift exceeds the type's number of bits, #[deny(exceeding_bitshifts)] on by default
foo.rs:3     let _y = 1<<64;
                      ^~~~~
<snip>
  10:     0x7febf9dfc424 - lint::context::Context<'a, 'tcx>::span_lint::h2ffdb047ae2ae95bNht
  11:     0x7febfadfd0cd - builtin::TypeLimits.LintPass::check_expr::h4be59f8968b266922ea
  12:     0x7febf9e01ed6 - lint::context::Context<'a, 'tcx>.Visitor<'v>::visit_expr::h9e77c18ecd8829fd2tt
  13:     0x7febf9e05d9e - lint::context::Context<'a, 'tcx>.Visitor<'v>::visit_block::h0d243a925ef41f59oHt
  14:     0x7febf9e0278a - lint::context::Context<'a, 'tcx>.Visitor<'v>::visit_fn::hd9ec7ea9d9ac70cc8vt
  15:     0x7febf9dfde24 - lint::context::Context<'a, 'tcx>::with_lint_attrs::h2026432032816296521
  16:     0x7febf9e02a2c - lint::context::Context<'a, 'tcx>.Visitor<'v>::visit_mod::hcb8a89207f3e5847aFt
  17:     0x7febf9e10944 - lint::context::check_crate::h206de34c06abf3109Yt
  18:     0x7febfc1427cc - driver::phase_3_run_analysis_passes::he94da8ae9035992aGGa

The last place that does constant evaluation is trans:

fn main() {
    let _ = 127u8+129u8;
}

Error:

$ RUST_BACKTRACE=1 rustc -Z treat-err-as-bug foo.rs
foo.rs:2:14: 2:25 error: internal compiler error: attempted to add with overflow
foo.rs:2     let _x = 127u8+129u8;
                      ^~~~~~~~~~~
<snip>
   7:     0x7f547b7800f6 - session::Session::span_err::h7e3fb41450de8302CFr
   8:     0x7f547c623575 - trans::consts::const_expr_unadjusted::hdf9dedc40195dad4O4s
   9:     0x7f547c5cb0dc - trans::consts::const_expr::h0fc914963c16e320tOs
  10:     0x7f547c61fa7a - trans::consts::get_const_expr_as_global::h57188bb77ebc5316zLs
  11:     0x7f547c6365d3 - trans::expr::trans_into::h415bacc50d92798cpwA
  12:     0x7f547c6b74da - trans::_match::mk_binding_alloca::h17286627990888781114
  13:     0x7f547c5ad80e - trans::base::init_local::hfaa046e6df58977cVdh
  14:     0x7f547c5bd816 - trans::controlflow::trans_block::h32c6387759bf9c1aRov
  15:     0x7f547c5bc421 - trans::base::trans_closure::h4f0176769f365e43yUh
  16:     0x7f547c5be12a - trans::base::trans_fn::h3ac38359cc215db9g5h
  17:     0x7f547c5c1018 - trans::base::trans_item::h3fe1d8c9b2f38607Kti
  18:     0x7f547c5ced44 - trans::base::trans_crate::hb837a89597acef06Phj
  19:     0x7f547dd57708 - driver::phase_4_translate_to_llvm::hc295663c813bca94zOa

I think we should consolidate the const_check & type_limits passes. The trans pass will be harder when we get generic constants through.

[eefriedman]

I just tried to implement this RFC... And hit a curious issue. There's actually no const evaluation happening until the lint-passes run: https://github.com/rust-lang/rust/blob/master/src/librustc_lint/builtin.rs#L174-L178 Is it really a breaking change to improve a lint? IIRC #1105 says that if you could have prevented the situation by changing a lint-level or using UFCs, the breaking change is fine.

Improving a lint is fine; see #1193 etc. For now, it would probably be good enough to just discard any constant evaluation warnings which would be triggered by a lint. Don't worry too much about the eventual model; a lot of our constant-handling code is probably going to have to be rewritten to work on MIR anyway.

[nikomatsakis]

Huzzah! The language design subteam has decided to accept this RFC. Tracking issue is rust-lang/rust#28238