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.

Sign up for GitHub

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

Jump to bottom

Add a forever unstable opt-out of const qualification checks #56123

Merged
merged 1 commit into from Feb 6, 2019
Merged

Add a forever unstable opt-out of const qualification checks #56123

merged 1 commit into from Feb 6, 2019

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Nov 21, 2018

r? @eddyb

cc @RalfJung @Centril

basically a forever unstable way to screw with const things in horribly unsafe, unsound and incoherent ways.

Note that this does not affect miri except by maybe violating assumptions that miri makes. But there's no change in how miri evaluates things.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 21, 2018
@rust-highfive

This comment has been minimized.

Sign in to view
@eddyb eddyb added I-nominated T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 21, 2018
@eddyb
Copy link
Member

eddyb commented Nov 21, 2018

cc @rust-lang/compiler

RalfJung
RalfJung reviewed Nov 21, 2018
View reviewed changes
src/test/ui/consts/miri_unleashed/assoc_const_2.stderr
--> $DIR/assoc_const_2.rs:27:13
|
LL | let y = <String as Bar<String>>::F; //~ ERROR erroneous constant
| ^^^^^^^^^^^^^^^^^^^^^^^^^^ referenced constant has errors
Copy link
Member

Choose a reason for hiding this comment

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

"Has errors" is not very precise, why does it not show an actual miri error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because of allow(const_err) silencing the lint that reports the error on a constant

RalfJung
RalfJung reviewed Nov 21, 2018
View reviewed changes
src/libsyntax/feature_gate.rs Outdated
@@ -230,6 +230,11 @@ declare_features! (
// Allows panicking during const eval (produces compile-time errors)
(active, const_panic, "1.30.0", Some(51999), None),

// Don't run any static analyses on constants and `const fn` and just "let miri handle it"
// NOT meant to ever be stabilized, this is solely for being able to demonstrate problems with
// features of alternative RFCs when writing RFCs that would allow more const features.
Copy link
Member

Choose a reason for hiding this comment

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

Also sounds useful for test cases.

@varkor
Copy link
Member

varkor commented Nov 21, 2018

Would it be reasonable to add this as a -Zunleash_the_miri_inside_of_you (or similar) instead of #[feature(unleash_the_miri_inside_of_you)]? The idea of people deciding to include the feature simply because it's convenient to do so isn't a nice one.

@oli-obk
Copy link
Contributor Author

oli-obk commented Nov 21, 2018

I'm not particularly bound to using features. Do we have any other -Z flags that change language rules?

@RalfJung
Copy link
Member

I'd count these:

    -Z                  two-phase-borrows -- use two-phase reserved/active distinction for `&mut` borrows in MIR borrowck
    -Z           two-phase-beyond-autoref -- when using two-phase-borrows, allow two phases even for non-autoref `&mut` borrows
    -Z       disable-nll-user-type-assert -- disable user provided type assertion in NLL
    -Z       nll-dont-emit-read-for-match -- in match codegen, do not include FakeRead statements (used by mir-borrowck)

@Centril
Copy link
Contributor

Centril commented Nov 22, 2018

@varkor The main nice thing about features is that it becomes easy to test stuff out in the playground and such so if it's not too much of a problem using features would be nice.

Centril
Centril reviewed Nov 22, 2018
View reviewed changes
src/librustc_mir/transform/qualify_consts.rs Outdated Show resolved Hide resolved
src/libsyntax/feature_gate.rs Outdated Show resolved Hide resolved
@RalfJung
Copy link
Member

TBH I also feel like a -Z flag is more suited. Maybe we can get @shepmaster to support arbitrary additional flags on playground, similar to godbolt? :D

@nagisa
Copy link
Member

nagisa commented Nov 22, 2018

NB: nll had both a flag and a "feature". Either one of them would enable the feature for testing.

@oli-obk
Copy link
Contributor Author

oli-obk commented Nov 22, 2018

Discussed in compiler meeting. A flag is definitely better because

making this feature a flag would make it impossible to misuse within libstd
or most of the code, really

@RalfJung RalfJung mentioned this pull request Nov 24, 2018
Open
6 tasks
@pnkfelix
Copy link
Member

This was discussed a week ago in T-compiler. Reviewing the discussion now, it seems like there was consensus that the feature should be a -Z flag. I don't think there's anything more to discuss, and the nomination tag was just accidentally left on.)

Un-nominating.

@bors

This comment has been minimized.

Sign in to view
@rust-highfive

This comment has been minimized.

Sign in to view
@bors

This comment has been minimized.

Sign in to view
@rust-highfive

This comment has been minimized.

Sign in to view
@oli-obk oli-obk mentioned this pull request Dec 11, 2018
Open
@bors
Copy link
Contributor

bors commented Jan 12, 2019

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

@TimNN
Copy link
Contributor

TimNN commented Jan 29, 2019

Ping from triage! What is the status of this PR?

@oli-obk
Copy link
Contributor Author

oli-obk commented Feb 5, 2019

cc @therealprof @jamesmunns @japaric

@eddyb
Copy link
Member

eddyb commented Feb 5, 2019

@bors r+

@bors
Copy link
Contributor

bors commented Feb 5, 2019

📌 Commit d0129a6 has been approved by eddyb

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 5, 2019
@oli-obk
Copy link
Contributor Author

oli-obk commented Feb 6, 2019

@bors r=eddyb

the bors queue did not pick this up as "approved"

@bors
Copy link
Contributor

bors commented Feb 6, 2019

📌 Commit d0129a6 has been approved by eddyb

@bors
Copy link
Contributor

bors commented Feb 6, 2019

⌛ Testing commit d0129a6 with merge b139669...

bors added a commit that referenced this pull request Feb 6, 2019
@bors
Auto merge of #56123 - oli-obk:import_miri_from_future, r=eddyb
b139669 
Add a forever unstable opt-out of const qualification checks

r? @eddyb

cc @RalfJung @Centril

basically a forever unstable way to screw with const things in horribly unsafe, unsound and incoherent ways.

Note that this does *not* affect miri except by maybe violating assumptions that miri makes. But there's no change in how miri evaluates things.
@bors
Copy link
Contributor

bors commented Feb 6, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: eddyb
Pushing b139669 to master...

@bors bors merged commit d0129a6 into rust-lang:master Feb 6, 2019
@nagisa
Copy link
Member

nagisa commented Feb 6, 2019

the sun is upon us

@oli-obk oli-obk deleted the import_miri_from_future branch February 7, 2019 08:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RalfJung RalfJung RalfJung left review comments

@Centril Centril Centril left review comments

Assignees

@eddyb eddyb

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@oli-obk @rust-highfive @eddyb @varkor @RalfJung @Centril @nagisa @pnkfelix @bors @TimNN