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

Infallible promotion #3027

Merged
merged 8 commits into from
Jan 13, 2021
Merged

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Nov 30, 2020

This is the RFC-follow-up to rust-lang/lang-team#58.

Rendered

Copy link

@matu3ba matu3ba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall good read and strategy, but especially >> and << and bit-operators (^,&,|,!) are missing. They could require some more reasoning.

text/0000-infallible-promotion.md Outdated Show resolved Hide resolved
If too much code is broken, various ways to weaken this proposal (at the expense of more technical debt, sometimes across several parts of the compiler) are [described blow][rationale-and-alternatives].

The long-term plan is that such code can switch to [inline `const` expressions](2920-inline-const.md) instead.
However, inline `const` expressions are still in the process of being implemented, and for now are specified to not support code that depends on generic parameters in the context, which is a loss of expressivity when compared with implicit promotion.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean, that this RFC is planned to be blocked on inline const expressions?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pacing of the individual breaking changes will be up for the lang team to decide. Having a migration path may or may not make a difference when incurring a breaking change.

text/0000-infallible-promotion.md Outdated Show resolved Hide resolved
text/0000-infallible-promotion.md Outdated Show resolved Hide resolved
@RalfJung
Copy link
Member Author

RalfJung commented Dec 2, 2020

but especially >> and << and bit-operators (^,&,|,!) are missing. They could require some more reasoning.

They are trivial, I just added them. What kinds of problems did you foresee here?

@matu3ba
Copy link

matu3ba commented Dec 3, 2020

but especially >> and << and bit-operators (^,&,|,!) are missing. They could require some more reasoning.

They are trivial, I just added them. What kinds of problems did you foresee here?

I was assuming overflow just means overflow and not underflow, but this RFC specifies the meaning.
"shift value is unconditionally masked to be modulo N to ensure that the argument is always in range"

Thus should be fine.

@RalfJung
Copy link
Member Author

RalfJung commented Dec 3, 2020

Yeah, "overflow" includes all kinds of values that cannot be represented, no matter "in which direction they overflow".

@bill-myers
Copy link

bill-myers commented Dec 4, 2020

How about changing constants so that if they have a #[panic] attribute they can have a "panic" value that results in a panic upon const use? (and no compiler error)

Then promotion would generate consts with the #[panic] attribute and everything should work fine without needing the complicated, inflexible and limited list of special cases in this RFC.

@RalfJung
Copy link
Member Author

RalfJung commented Dec 4, 2020

How about changing constants so that if they have a #[panic] attribute they can have a "panic" value that results in a panic upon const use?

That would need a lot of complicated machinery all over the compiler. Note that we need a "complicated, inflexible and limited list of special cases" anyway for promotion, e.g. to exclude interior mutability or types that drop. All this RFC is doing is restricting that list just a bit more to avoid having to deal with consts that can fail to evaluate without making compilation fail.

In other words, this RFC is a lot simpler than what you proposed. The patch to implement this RFC will be less than 20 lines, followed by a cleanup PR that can remove way more code (and way more subtle code) than what the RFC implementation added.

Also note that panics are not the only way consts can fail to evaluate. They can also cause Undefined Behavior or perform an operation that us not support by CTFE.

@scottmcm scottmcm added the T-lang Relevant to the language team, which will review and decide on the RFC. label Dec 4, 2020
@scottmcm
Copy link
Member

scottmcm commented Dec 5, 2020

Thanks for the write-up, Ralf! I knew some of the various pieces here, but between this and its earlier MCP version still learned a bunch -- the details of how the checked operators are promoted was an interesting nuance, for example, and not something I'd seen documented elsewhere. It was great having it all in order to bring everything together.

Since it LGTM (and we asked you to write it),
@rfcbot fcp merge

@rfcbot
Copy link
Collaborator

rfcbot commented Dec 5, 2020

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

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), 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.

@rfcbot rfcbot added proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. labels Dec 5, 2020
@rfcbot rfcbot added final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. and removed proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. labels Dec 18, 2020
@rfcbot
Copy link
Collaborator

rfcbot commented Dec 18, 2020

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

@RalfJung
Copy link
Member Author

A recent comment by @oli-obk reminded me of the subtle interaction of promotion and const-validity checks. I added some text explaining that to the RFC; please let me know if that makes sense.

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this RFC. to-announce and removed final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. labels Dec 28, 2020
@rfcbot
Copy link
Collaborator

rfcbot commented Dec 28, 2020

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

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

@RalfJung
Copy link
Member Author

RalfJung commented Jan 2, 2021

I created a tracking issue at rust-lang/rust#80619.
Someone else will have to do the actual merging of the PR in this repo though. :)

@RalfJung
Copy link
Member Author

RalfJung commented Jan 9, 2021

@nikomatsakis @rust-lang/lang who's in charge of merging accepted RFCs? :)

@KodrAus
Copy link
Contributor

KodrAus commented Jan 13, 2021

I tried 😄

cc @Mark-Simulacrum

EDIT: un-cc @Mark-Simulacrum

@KodrAus KodrAus merged commit 7711133 into rust-lang:master Jan 13, 2021
@KodrAus
Copy link
Contributor

KodrAus commented Jan 13, 2021

Huzzah! The @rust-lang/lang team has decided to accept this RFC.

To track further discussion, subscribe to the tracking issue here:
rust-lang/rust#80619

@pickfire

This comment has been minimized.

bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 23, 2021
…-obk

avoid promoting division, modulo and indexing operations that could fail

For division, `x / y` will still be promoted if `y` is a non-zero integer literal; however, `1/(1+1)` will not be promoted any more.

While at it, also see if we can reject promoting floating-point arithmetic (which are [complicated](rust-lang/unsafe-code-guidelines#237) so maybe we should not promote them).

This will need a crater run to see if there's code out there that relies on these things being promoted.

If we can land this, promoteds in `fn`/`const fn` cannot fail to evaluate any more, which should let us do some simplifications in codegen/Miri!

Cc rust-lang/rfcs#3027
Fixes rust-lang#61821
r? `@oli-obk`
m-ou-se added a commit to m-ou-se/rust that referenced this pull request Feb 3, 2021
make const_err a future incompat lint

This is the first step for rust-lang#71800: make const_err a future-incompat lint. I also rewrote the const_err lint description as the old one seemed wrong.

This has the unfortunate side-effect of making const-eval error even more verbose by making the const_err message longer without fixing the redundancy caused by additionally emitting an error on each use site of the constant. We cannot fix that redundancy until const_err is a *hard* error (at that point the error-on-use-site can be turned into a `delay_span_bug!` for uses of monomorphic consts, and into a nicely rendered error for [lazily / post-monomorhization evaluated] associated consts).

~~The one annoying effect of this PR is that `let _x = &(1/(1-1));` now also shows the future-incompat warning, even though of course we will *not* make this a hard error. We'll instead (hopefully) stop promoting it -- see rust-lang/rfcs#3027. The only way I see to avoid the future-incompat warning is to use a different lint for "failure to evaluate promoted".~~

Cc `@rust-lang/wg-const-eval`
@RalfJung RalfJung deleted the infallible-promotion branch September 30, 2022 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this RFC. T-lang Relevant to the language team, which will review and decide on the RFC. to-announce
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants