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

Allow panicking in constants #2345

Merged
merged 3 commits into from Jul 2, 2018

Conversation

Projects
None yet
@oli-obk
Contributor

oli-obk commented Feb 22, 2018

Rendered

Tracking issue

DON'T PANIC! "in large, friendly letters"

cc @pnkfelix (#1383)

@Centril Centril added the T-lang label Feb 22, 2018

@joshtriplett

This comment has been minimized.

Member

joshtriplett commented Feb 23, 2018

This seems like a good idea to me. Supporting panic from compile-time computation as a compile-time error makes perfect sense, and I'd anticipate many excellent error diagnostics on that basis.

I do want to see something along the lines of the last item in the unresolved section. Whether unwrap or something else, I'd like to see a convenient way for a const fn that wants to produce a T produce a Result<T, E> instead and turn the E into a compile-time error. That mechanism should work as straightforwardly as possible, and integrate with convenient use of ? within the const fn.

@SimonSapin

This comment has been minimized.

Contributor

SimonSapin commented Feb 23, 2018

I think that making Result::unwrap and Option::unwrap be const fns is the obvious next step.

? in const fns is less straightforward since its expansion includes a call to From::from, so we’d need to decide how const fn trait methods work in general and what the From trait should do specifically.

@kennytm

This comment has been minimized.

Member

kennytm commented Mar 4, 2018

While I do support panicking in const fn, I'm uneasy with this potential direction mentioned in the unresolved question:

This change becomes really useful if Result::unwrap and Option::unwrap become const fn, doing both in one go might be a good idea

This sounds like eventually the entire libcore will need to marked as const fn as our const-evaluation support improves, which reminds me of D's mess of encouraging pure nothrow @safe @nogc on everything to become maximally compatible.

We need the const specifier because we want to ensure, in terms of API stability, that the function can always be used in a constant context. This makes inferring constness not a feasible solution. AFAIK this question is put aside in #911 since it was a short-term solution to make 1.0 available and the const keyword can be easily deprecated should a better system emerges.

I don't know of a better solution. Maybe we could add a #[const(always/auto/never)] attribute applicable to functions and impl and modules, but library authors are still pressured to attach these attributes everywhere.

@oli-obk

This comment has been minimized.

Contributor

oli-obk commented Mar 4, 2018

Well.. we could infer constness for private functions, which would make a lot of things easier. We could also consider some future notconst modifier that deprecates const in favour of marking things const. Or we turn the entire thing into a lint and always infer it. So if you use a nonconst thing in a const env you get a deny lint that you can turn off and thus opt into accidental breakage.

Anyway, this is a very much orthogonal topic and we should probably be disussing it in the internals forum instead of here

@Centril

This comment has been minimized.

Contributor

Centril commented Mar 4, 2018

which reminds me of D's mess of encouraging pure nothrow @safe @nogc on everything to become maximally compatible.

Sounds like something an effect system and effect polymorphism might solve nicely?

@glaebhoerl

This comment has been minimized.

Contributor

glaebhoerl commented Mar 4, 2018

@oli-obk Having all of these const-related extensions be piecemeal, feature-by-feature independent RFCs and also saying that neither of them are the appropriate place to discuss the overarching issues they raise feels kind of suboptimal to me...

@Centril Effect polymorphism, alas, is only a solution to abstracting over things. That is, if you might want to say things like "this fn is const as long as its argument fn is also const". Specifying what effects this function, itself has (or hasn't) is a separate problem.

@oli-obk

This comment has been minimized.

Contributor

oli-obk commented Mar 4, 2018

@glaebhoerl while all of these RFCs increase the possible code that can be evaluated, this one in particular does not add anything new except nicer errors. The only one that allows real new features is the constant if RFC. Everything else was already possible via const fn. The particular concern about marking everything const fn should be part of the const fn stabilization and has already been heavily discussed in the tracking issue rust-lang/rust#24111

@clarcharr

This comment has been minimized.

Contributor

clarcharr commented Mar 9, 2018

I don't know of a better solution. Maybe we could add a #[const(always/auto/never)] attribute applicable to functions and impl and modules, but library authors are still pressured to attach these attributes everywhere.

This wouldn't be too different from #[inline] today, but I agree.

@clarcharr

This comment has been minimized.

Contributor

clarcharr commented Mar 9, 2018

Also I think that the big thing to point out, whether or not you agree with const fn, is that currently, Rust is not const by default. So, special-casing panic to work in constants now is very reasonable, with the only caveat being making Debug work somehow.

If you're in favour of const inference then this RFC should be an obvious yes imho.

@daboross

This comment has been minimized.

daboross commented Mar 14, 2018

I think that making Result::unwrap and Option::unwrap be const fns is the obvious next step.

Would this change the following from a runtime to a compile-time error, then?

fn main() {
    Err::<(), ()>(()).unwrap();
}

If so, is that a breaking change - if someone does really want to panic at runtime?

I like this change, but this aspect seems concerning. It seems to make adding const to function a breaking change, because things which were previously runtime errors become compile-time errors instead.

@clarcharr

This comment has been minimized.

Contributor

clarcharr commented Mar 14, 2018

@daboross I mean, that example is enough to warrant a crater run, but IMHO if a logic error is caught at compile time instead of runtime, that's just helping the programmer out.

@oli-obk

This comment has been minimized.

Contributor

oli-obk commented Mar 14, 2018

Would this change the following from a runtime to a compile-time error, then?

No. That just becomes a lint.

@daboross

This comment has been minimized.

daboross commented Mar 14, 2018

@clarcharr it's helping the programmer for actively developed applications and libraries, but not everything has a maintainer.

There are situations even today when small backwards-incompatible coherence changes break existing unmaintained libraries (AndyBarron/app-dirs-rs#28 is my latest example).

This would be on a much larger scale, though. If we adopt const fns widely, which I assume is the plan, this would break any library or application which computes something incorrectly using compile-time data.

What if there's some bad regex::Regex::new hidden away in some part of a library that can't ever actually run? If regex makes new a const fn, and doesn't bump up from 1.0 in order to do that, that code will break. Even if the panic would never have been run at all, it could change to fail to compile.


@oli-obk Alright, that sounds reasonable to me.

Does this mean we make all compile-time panics lints which then forward the panic to runtime?

@oli-obk

This comment has been minimized.

Contributor

oli-obk commented Mar 14, 2018

It seems to make adding const to function a breaking change, because things which were previously runtime errors become compile-time errors instead.

It is only a compile time error if you call it in a const context. Since you can't call non const fns in const contexts, you can't get a breaking change this way. Const context being array lengths, static initializers or outright constants

@daboross

This comment has been minimized.

daboross commented Mar 14, 2018

@oli-obk ahh, that makes a lot more sense, thanks!

I guess I was going on the assumption that the compiler would unroll / execute any const fns which were present in non-const code. If that isn't the plan, then my objection is not applicable at all.

@oli-obk

This comment has been minimized.

Contributor

oli-obk commented Mar 14, 2018

You can't execute foo(std::env::args().count()) at compile time, because the value isn't there at compile time. Const fn means that additionally to runtime, you can execute it at compile time. If the arguments are constant, the compiler will try to execute at compile time on a best effort basis, and if that fails, the error is reported as a lint. RFC #1229 discussed this exact issue very extensively if you are interested in more details

@SimonSapin

This comment has been minimized.

Contributor

SimonSapin commented Mar 14, 2018

Err::<(), ()>(()).unwrap();

This is similar to fn main() { [][0] } (array indexing out of bounds), which today causes warning: this expression will panic at run-time but still compiles. That unwrap call might or might not warn in the future, but allowing panics in const expressions doesn’t change the semantics of non-const expression.

@Centril

This comment has been minimized.

Contributor

Centril commented Mar 25, 2018

@glaebhoerl

@Centril Effect polymorphism, alas, is only a solution to abstracting over things. That is, if you might want to say things like "this fn is const as long as its argument fn is also const". Specifying what effects this function, itself has (or hasn't) is a separate problem.

Fair enough; Specifying what effects a particular function has could be done more ergonomically with effect aliases (type aliases but for the effect kind), like so:

effect foo = const + async + total;
foo fn bar(..) { // modulo syntax bikeshed and making it unambiguous.
}
@pnkfelix

This comment has been minimized.

Member

pnkfelix commented May 14, 2018

Summary of comment thread:

As far as I can tell, all commenters seem to be in favor of this RFC as written.

There was some discussion about whether to expand the scope of the RFC to also change Result::unwrap and Option::unwrap to be const fn's

  • Though there was a bit of pushback from kennytm on this matter, I think in terms of setting us down a path of gradually adding const everywhere in libcore, and that led to some discussion of inferring const-ness rather than requiring explicit demarcation.
  • In any case, @oli-obk said, rightly IMO, that this topic would probably be better off on an internals thread rather than continuing here.

I do have one relatively small concern with the text itself, but my concern is pretty slight, so I think I will propose merging this via the rfcbot and then mark my concern formally there (in part to ensure that it does not get completely overlooked).

@pnkfelix

This comment has been minimized.

Member

pnkfelix commented May 14, 2018

@rfcbot fcp merge

@pnkfelix pnkfelix closed this May 14, 2018

@pnkfelix pnkfelix reopened this May 14, 2018

@pnkfelix

This comment has been minimized.

Member

pnkfelix commented May 14, 2018

(argh hit the wrong button.)

@pnkfelix

This comment has been minimized.

Member

pnkfelix commented May 14, 2018

@rfcbot fcp merge

@rfcbot

This comment has been minimized.

rfcbot commented May 14, 2018

Team member @pnkfelix has proposed to merge this. The next step is review by the rest of the tagged teams:

Concerns:

  • feature-scoped-to-macros-or-to-their-expansion resolved by #2345 (comment)

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@eddyb

This comment has been minimized.

Member

eddyb commented May 14, 2018

@pnkfelix I assume the support functions to be made const fn are unstable and/or will receive an const_unstable attribute to make calling them in a const context unstable, and so libstd macros will be able to use them in const contexts only because of #[allow_internal_unstable].

@pnkfelix

This comment has been minimized.

Member

pnkfelix commented May 14, 2018

@eddyb Okay, my assumption was wrong in that I thought ::std::rt::begin_panic was stable, but AFAICT it is not. Good.

Plus, I didn't know about the #[const_unstable] attribute. Also good.

The bits of machinery you list mean that its probably relatively easy to implement something that behaves like the second change I outlined: namely, for all practical purposes, in stable code const-eval will be restricted to invocations of panic! et al, and will not be able to use things like ::std::rt::begin_panic! directly nor macros that expand to those functions.

(I still think the RFC could spell this out, but its not as vital since I am probably wrong about the intention, in that the change to the API that const-eval presents is just to add panic!, assert!, and assert_eq!, as the RFC says.)

@pnkfelix

This comment has been minimized.

Member

pnkfelix commented May 15, 2018

@rfcbot resolved feature-scoped-to-macros-or-to-their-expansion

@rfcbot

This comment has been minimized.

rfcbot commented Jun 22, 2018

🔔 This is now entering its final comment period, as per the review above. 🔔

@tkadur

This comment has been minimized.

tkadur commented Jun 28, 2018

I'm jumping in pretty late here, but I'm not sure how I feel about the compile time versions of panic! and co. having the same names as the normal ones. I feel like having something entirely separate, a la static_assert, would be better for clarity.

As an aside, it would give us the option to allow const_panic! (or whatever it would be called) in non-const code (much like C++ allows) in a future RFC if people decide it's worthwhile.

@Centril

This comment has been minimized.

Contributor

Centril commented Jun 28, 2018

@tkadur note that if you write panic!() inside const fn foo() { .. }, and then call foo() in a runtime context, then that panic will happen at runtime, not at compile time.

I think a goal should be to minimize the difference in experience across fn and const fn personally; so panic! is better imo.

@tkadur

This comment has been minimized.

tkadur commented Jun 29, 2018

a goal should be to minimize the difference in experience across fn and const fn

This is a good point - makes sense to me.

@eddyb

This comment has been minimized.

Member

eddyb commented Jun 29, 2018

Note that you'll almost never use panic directly - but rather, this lets us stabilize things like Option::unwrap.

@rfcbot

This comment has been minimized.

rfcbot commented Jul 2, 2018

The final comment period, with a disposition to merge, as per the review above, is now complete.

@Centril Centril merged commit 5a44ed0 into rust-lang:master Jul 2, 2018

@Centril

This comment has been minimized.

Contributor

Centril commented Jul 2, 2018

Huzzah! This RFC is merged!

Tracking issue: rust-lang/rust#51999

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment