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

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

Merged
merged 6 commits into from Sep 4, 2015

Conversation

@oli-obk
Copy link
Contributor

commented Jul 30, 2015

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)


# Unresolved questions

How to implement this?

This comment has been minimized.

Copy link
@pnkfelix

pnkfelix Jul 30, 2015

Member

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

This comment has been minimized.

Copy link
@oli-obk

oli-obk Jul 30, 2015

Author Contributor

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

This comment has been minimized.

Copy link
Member

commented Jul 30, 2015

@pnkfelix pnkfelix added T-compiler T-lang and removed T-lang labels Jul 30, 2015

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2015

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

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2015

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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2015

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

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2015

@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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2015

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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2015

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.


- the initializer of a constant `const foo: ty = EXPR` or `static foo: ty = EXPR`
- the size of an array `[T; EXPR]`
- the length of a repeat expression `[VAL; LEN_EXPR]`

This comment has been minimized.

Copy link
@eefriedman

eefriedman Jul 31, 2015

Contributor

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

This comment has been minimized.

Copy link
@oli-obk

oli-obk Jul 31, 2015

Author Contributor

fixed


## Const-eval the body of `const fn` that are never used in a constant environment

Currently a `const fn` that is called in non-const code is treated just like a normal function.

This comment has been minimized.

Copy link
@pnkfelix

pnkfelix Jul 31, 2015

Member

(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 comment has been minimized.

Copy link
Contributor

commented Aug 1, 2015

This is also relevant for our erroring on transmute.

@arielb1

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2015

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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2015

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.

@arielb1 arielb1 referenced this pull request Aug 4, 2015
@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2015

@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

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2015

@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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2015

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

@oli-obk

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2015

updated PR, should I squash?

@pnkfelix

This comment has been minimized.

Copy link
Member

commented Aug 7, 2015

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 oli-obk changed the title turn statically known erroneous code into a warning and an unconditional panic turn statically known erroneous code into a warning and continue normal code-generation Aug 7, 2015

@oli-obk

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2015

updated title and message at the top

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2015

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

@oli-obk

This comment has been minimized.

Copy link
Contributor Author

commented Aug 17, 2015

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

This comment has been minimized.

Copy link

commented Aug 17, 2015

@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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 18, 2015

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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 18, 2015

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

This comment has been minimized.

Copy link
Contributor

commented Aug 18, 2015

@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

This comment has been minimized.

Copy link
Contributor

commented Aug 19, 2015

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

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2015

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

tari added a commit to tari/rfcs that referenced this pull request Sep 8, 2015

@oli-obk oli-obk referenced this pull request Oct 5, 2015

bors added a commit to rust-lang/rust that referenced this pull request Oct 18, 2015

Auto merge of #28845 - oli-obk:rfc1229, r=pnkfelix
This PR turns statically known erroneous code (e.g. numeric overflow) into a warning and continues normal code-generation to emit the same code that would have been generated without `check_const` detecting that the result can be computed at compile-time.

<del>It's not done yet, as I don't know how to properly emit a lint from trans. I can't seem to extract the real lint level of the item the erroneous expression is in.</del> It's an unconditional warning now.

r? @pnkfelix 

cc @nikomatsakis 

* [RFC 1229 text](https://github.com/rust-lang/rfcs/blob/master/text/1229-compile-time-asserts.md)
* RFC PR: rust-lang/rfcs#1229
* tracking issue: #28238
@eddyb eddyb referenced this pull request May 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.