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

Implement dataflow-based const validation #64470

Merged
merged 31 commits into from Sep 29, 2019

Conversation

@ecstatic-morse
Copy link
Contributor

@ecstatic-morse ecstatic-morse commented Sep 14, 2019

This PR adds a separate, dataflow-enabled pass that checks the bodies of consts, statics and const fns for const safety. This is based on my work in #63860, which tried to integrate into the existing pass in qualify_consts.rs. However, the resulting pass was even more unwieldy than the original. Unlike its predecessor, this PR is designed to be combined with #63812 to replace the existing pass completely.

The new checker lives in librustc_mir/transform/check_consts.

qualifs.rs contains small modifications to the existing Qualif trait and its implementors, but is mostly unchanged except for the removal of IsNotPromotable and IsNotImplicitlyPromotable, which are only necessary for promotion.

resolver.rs contains the dataflow analysis used to propagate qualifs between locals.

Finally, validation.rs contains a refactored version of the existing Visitor in qualfy_consts.rs. All errors have been associated with a struct to make comparison with the existing pass simple.

The existing validation logic in qualify_consts has been modified to allow it to run in parallel with the new validator. If use_new_validator is not set, the old validation will be responsible for actually generating the errors, but those errors can be compared with the ones from the new validator.

@rust-highfive

This comment has been hidden.

@ecstatic-morse
Copy link
Contributor Author

@ecstatic-morse ecstatic-morse commented Sep 14, 2019

r? @eddyb

@rust-highfive rust-highfive assigned eddyb and unassigned zackmdavis Sep 14, 2019
@ecstatic-morse

This comment has been hidden.

@ecstatic-morse ecstatic-morse force-pushed the split-promotion-and-validation branch from 17b52c5 to 797a2c2 Sep 15, 2019
@rust-highfive

This comment has been hidden.

@ecstatic-morse ecstatic-morse force-pushed the split-promotion-and-validation branch from 797a2c2 to 691ca56 Sep 15, 2019
@ecstatic-morse

This comment has been hidden.

@ecstatic-morse ecstatic-morse force-pushed the split-promotion-and-validation branch from 91ff10a to 64afa84 Sep 16, 2019
@rust-highfive

This comment has been hidden.

@rust-highfive

This comment has been hidden.

@ecstatic-morse ecstatic-morse force-pushed the split-promotion-and-validation branch from 3400462 to abd2034 Sep 16, 2019
@rust-highfive

This comment has been hidden.

@ecstatic-morse ecstatic-morse force-pushed the split-promotion-and-validation branch from abd2034 to 5e93494 Sep 17, 2019
@rust-highfive

This comment has been hidden.

@ecstatic-morse ecstatic-morse force-pushed the split-promotion-and-validation branch from 5e93494 to fe0d220 Sep 17, 2019
@rust-highfive

This comment has been hidden.

@ecstatic-morse ecstatic-morse force-pushed the split-promotion-and-validation branch 3 times, most recently from 3b1e7e5 to 0e431ee Sep 17, 2019
@ecstatic-morse
Copy link
Contributor Author

@ecstatic-morse ecstatic-morse commented Sep 17, 2019

I've cleaned up the history a bit, and I'm happy with the current state of this. I'm removing the draft status, although there's a few decisions that need to be made before this is actually merged.

This PR now allows validation to proceed in two modes, controlled by use_new_validator. If that flag is false, the old validator is responsible for actually emitting the errors, but the new validator still runs so it can compare the errors it would have emitted. Currently, if the errors don't match, the PR panics to trigger test failures.

If use_new_validator is set, we suppress all errors in the old validator and simply run the new one. No comparison is done because I expect this flag to only be set for items where dataflow is actually required, at least until we can remove the old pass entirely. The old pass is still run unconditionally because it does promotability analysis as well.

Also, -Zunleash-the-miri-inside-you turns on the new validator. This not strictly necessary, but the new validator gives better warnings about when a const check is ignored.

cc @oli-obk @eddyb This is designed to be reviewed commit-by-commit which should make things a bit more manageable.

@ecstatic-morse ecstatic-morse marked this pull request as ready for review Sep 17, 2019
@ecstatic-morse ecstatic-morse changed the title Split const validation from promotion Implement dataflow-based const validation Sep 17, 2019
@andjo403
Copy link
Contributor

@andjo403 andjo403 commented Sep 29, 2019

this PR cause large perf regression see perf
the query check_match seems to have the biggest impact
but also mir_validated

@oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Sep 29, 2019

This is expected since we're running old and new and comparing the result. When we remove the old check we should get perf back

@ecstatic-morse
Copy link
Contributor Author

@ecstatic-morse ecstatic-morse commented Sep 29, 2019

Why would this PR affect check_match (the HIR check that validates patterns in match arms)?

Centril added a commit to Centril/rust that referenced this issue Oct 2, 2019
…=oli-obk

Enable support for `IndirectlyMutableLocals` in `rustc_peek`

This PR allows `rustc_peek` tests to be written for the `IndirectlyMutableLocals` analysis implemented in rust-lang#64470. See any of the tests in [`test/ui/mir-dataflow`](https://github.com/rust-lang/rust/blob/master/src/test/ui/mir-dataflow/inits-1.rs) for an example.

Included in this PR is a major rewrite of the `rustc_peek` module. This was motivated by the differences between the `IndirectlyMutableLocals` analysis and the initialized places ones.

To properly test `IndirectlyMutableLocals`, we must pass locals by-value to `rustc_peek`, since any local that is not `Freeze` will be marked as indirectly mutable as soon as a reference to it is taken. Unfortunately, `UnsafeCell` is not `Copy`, so we can only do one `rustc_peek` on each value with interior mutability inside a test. I'm not sure how to deal with this restriction; perhaps I need to special case borrows preceding a call to `rustc_peek` in the analysis itself?

`rustc_peek` also assumed that the analysis was done on move paths and that its transfer function only needed to be applied at assignment statements. This PR removes both of those restrictions by adding a trait, `RustcPeekAt`, that controls how the peeked at `Place` maps to the current dataflow state and using a dataflow cursor to retrieve the state itself.

Finally, this PR adds a test which demonstrates some unsoundness in the `IndirectlyMutableLocals` analysis by converting a reference to a `Freeze` field to a reference to a `!Freeze` field by offsetting a pointer (or in this case transmuting a pointer to a ZST field with the same address as a `!Freeze` field). This does not represent a hole in the language proper, since this analysis is only used to validate `const` bodies, in which the unsound code will only compile with `-Zunleash-the-miri-inside-of-you`. Nevertheless, this should get fixed.

r? @oli-obk
@joshlf
Copy link
Contributor

@joshlf joshlf commented Oct 4, 2019

I assume somebody has checked, but just in case not: does this affect the static_assertions crate, which relies on const evaluation to fail in some cases?

(cc @nvzqz )

@nvzqz
Copy link
Contributor

@nvzqz nvzqz commented Oct 4, 2019

It currently expects const eval for array size overflow to fail. But even if that changes to allow an overflow, there’d still be a type mismatch.

@oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Oct 4, 2019

Any stable behaviour won't be changed by anything we implement. The only thing that happens is that unstable behaviour or unimplemented behaviour gets stabilized. If a macro were to expand to if foo {} inside a constant, that would indeed fail right now and work later, but that's just like expanding to garlbladadf and hoping it fails to compile because that name doesn't exist.

The behaviour that the static_assertions crate depends on is either type resolution failures or things that would panic if they were runtime code. If your code panics at runtime and you move it to compile time, you get a compile time error, guaranteed.

@nvzqz
Copy link
Contributor

@nvzqz nvzqz commented Oct 4, 2019

Once const in if is stabilized, I think the initial approach would be to emit (via build.rs) a const_assert! that has a different body depending on whether the used Rust compiler version supports the feature. This would allow for having a meaningful error message via panic!. This would not be a breaking change since it doesn't bump the minimum required Rust version.

However, once const_assert! starts allowing for custom formatted error messages, then that would be a breaking change and require a major version bump. Reason being that you wouldn't want one of your dependencies to start using const_assert!(CONDITION, "Assumption X failed") and that breaking your builds. If not by then, down the line once const string formatting works, you'd also be able to do const_assert!(X > Y, "{} > {} failed", X, Y). If that isn't available in the same version in which const in if is stabilized, then that's yet another breaking change.

I'm totally fine with breaking changes in static_assertions because it doesn't expose any types and thus won't cause incompatibility issues in the ecosystem. I think this crate is part of a small subset of the ecosystem for which that is the case.

nikomatsakis added a commit to nikomatsakis/team that referenced this issue Oct 17, 2019
They're working to make compile-time evaluation more expressive by
enabling `if`, `match` and other control flow in constants. As one of
their first major contributions, they implemented a dataflow analysis
to validate the bodies of `const`s and `const
fn`s (rust-lang/rust#64470).
ecstatic-morse added a commit to ecstatic-morse/rust that referenced this issue Oct 25, 2019
bors added a commit that referenced this issue Oct 25, 2019
bors added a commit that referenced this issue Nov 23, 2019
Enable `if` and `match` in constants behind a feature flag

This PR is an initial implementation of #49146. It introduces a `const_if_match` feature flag and does the following if it is enabled:
- Allows `Downcast` projections, `SwitchInt` terminators and `FakeRead`s for matched places through the MIR const-checker.
- Allows `if` and `match` expressions through the HIR const-checker.
- Stops converting `&&` to `&` and `||` to `|` in `const` and `static` items.

As a result, the following operations are now allowed in a const context behind the feature flag:
- `if` and `match`
- short circuiting logic operators (`&&` and `||`)
- the `assert` and `debug_assert` macros (if the `const_panic` feature flag is also enabled)

However, the following operations remain forbidden:
- `while`, `loop` and `for` (see #52000)
- the `?` operator (calls `From::from` on its error variant)
- the `assert_eq` and `assert_ne` macros, along with their `debug` variants (calls `fmt::Debug`)

This PR is possible now that we use dataflow for const qualification (see #64470 and #66385).

r? @oli-obk
cc @rust-lang/wg-const-eval @eddyb
RalfJung added a commit to RalfJung/rust that referenced this issue Dec 2, 2019
…eddyb,matthewjasper

Handle const-checks for `&mut` outside of `HasMutInterior`

Addresses [this comment](rust-lang#64470 (comment)).

Const-checking relied on `HasMutInterior` to forbid `&mut` in a const context. This was strange because all we needed to do was look for an `Rvalue::Ref` with a certain `BorrowKind`, whereas the `Qualif` traits are specifically meant to get the qualifs for a *value*. This PR removes that logic from `HasMutInterior` and moves it into `check_consts::Validator`.

As a result, we can now properly handle qualifications for `static`s, which had to be ignored previously since you can e.g. borrow a static `Cell` from another `static`. We also remove the `derived_from_illegal_borrow` logic, since it is no longer necessary; we give good errors for subsequent reborrows/borrows of illegal borrows.
/// A function call where the callee is not a function definition or function pointer, e.g. a
/// closure.
Copy link
Member

@RalfJung RalfJung Apr 14, 2020

Choose a reason for hiding this comment

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

I am very confused by this comment. Syntactically, in MIR only functions and function pointers can be called -- so "where the callee is not a function definition or function pointer" seems just impossible?

Copy link
Member

@RalfJung RalfJung Apr 14, 2020

Choose a reason for hiding this comment

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

@bjorn3's (deleted? it's rather confusing to get a notification and then not see the comment, please edit it but don't delete) comment made me realize that maybe I got the parentheses here wrong -- I thought this was "or (function pointer, e.g. closure)", but I suppose closure is meant as an example for other callees here? But closures are not called directly, they are called via the Fn* traits. Or does this run on a variant of MIR where things have not been lowered yet?

Copy link
Contributor

@bjorn3 bjorn3 Apr 15, 2020

Choose a reason for hiding this comment

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

But closures are not called directly, they are called via the Fn* traits.

Realized that immediately after I commented.

return;
}
_ => {
self.check_op(ops::FnCallOther);
Copy link
Member

@RalfJung RalfJung Apr 14, 2020

Choose a reason for hiding this comment

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

Callees must always be FnPtr or FnDef (Miri will ICE when it sees anything else), how is this here even reachable?

Copy link
Contributor Author

@ecstatic-morse ecstatic-morse Apr 14, 2020

Choose a reason for hiding this comment

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

This const-checking pass has structured errors everywhere the old pass had a call to span_err. If you would like to change this, it would be nice to encode the set of types that can appear as the func operand of a Call terminator in an enum somewhere so we could use exhaustive matching in more places.

Copy link
Member

@RalfJung RalfJung Apr 14, 2020

Choose a reason for hiding this comment

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

it would be nice to encode the set of types that can appear as the func operand of a Call terminator in an enum somewhere so we could use exhaustive matching in more places.

This is basically suggesting "intrinsically-typed MIR", where invalid MIR cannot even be constructed. I am not sure what the best way would be to do that in Rust (I only know how to do it with dependent types). Calls are not the only operations this affects, tons of MIR operations have invariants for which types can occur -- arithmetic primitives, casts, pointer dereferences, ...

Copy link
Contributor Author

@ecstatic-morse ecstatic-morse Apr 14, 2020

Choose a reason for hiding this comment

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

I just meant an as_func_ty combinator for Ty whose return value is restricted to the types that are valid as a func. It's still up to the user to call this in the correct places.

Copy link
Member

@RalfJung RalfJung Apr 14, 2020

Choose a reason for hiding this comment

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

I just meant that there are more "subclasses" of types than "callable types", and some of them overlap. I don't think we should adopt something ad-hoc that only works for callable types.

Copy link
Contributor Author

@ecstatic-morse ecstatic-morse Apr 14, 2020

Choose a reason for hiding this comment

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

Gotta start somewhere. What do we do for closures btw? Do these get lowered to FnDefs by this point?

Copy link
Member

@RalfJung RalfJung Apr 14, 2020

Choose a reason for hiding this comment

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

Gotta start somewhere.

I'd rather just remove this dead code for now, and leave improving MIR itself for later. Do you want to track this in an issue?

Copy link
Member

@RalfJung RalfJung Apr 14, 2020

Choose a reason for hiding this comment

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

Closures do not get called directly. Instead FnOnce::call_once (or the other Fn* traits) gets called, which is a FnDef. The ty::Closure is then passed as self argument.

See for example the MIR of this function.

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Apr 24, 2020
…=oli-obk

Only run dataflow for const qualification if type-based check would fail

This is the optimization discussed in rust-lang#49146 (comment). We wait for `Qualif::in_any_value_of_ty` to return `true` before running dataflow. For bodies that deal mostly with primitive types, this will avoid running dataflow at all during const qualification.

This also removes the `BitSet` used to cache `in_any_value_of_ty` for each local, which was only necessary for an old version of rust-lang#64470 that also handled promotability.
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Apr 24, 2020
…=oli-obk

Only run dataflow for const qualification if type-based check would fail

This is the optimization discussed in rust-lang#49146 (comment). We wait for `Qualif::in_any_value_of_ty` to return `true` before running dataflow. For bodies that deal mostly with primitive types, this will avoid running dataflow at all during const qualification.

This also removes the `BitSet` used to cache `in_any_value_of_ty` for each local, which was only necessary for an old version of rust-lang#64470 that also handled promotability.
@ecstatic-morse ecstatic-morse deleted the split-promotion-and-validation branch Oct 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet