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

Rewrite polonius-engine #183

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

ecstatic-morse
Copy link

@ecstatic-morse ecstatic-morse commented Jan 16, 2022

Requires rust-lang/datafrog#50.

Each section ("naive", "initialization", etc.) is now expressed as a Computation, with defined inputs and outputs. Computations are organized into Pipelines, which execute each computation in turn, storing the results in a Db where they are accessible to later computations in the pipeline.

Computations can also dump intermediate results using the (creatively named) Dump struct. A series of Dumpers can be specified for a pipeline, each which will be given access to a type-erased version of the dumped data.

The purpose of this is to support arbitrary configurations of analysis passes, such as skipping move/init analysis by providing an additional "dropped_while_init" input, or the use of alternate solvers (e.g. Soufflé).

For now, I've preserved the existing interface of polonius-engine (Ouput, AllFacts and Algorithm). I expect this to be temporary, but I wanted to take advantage of the existing test machinery and keep my changes localized (relatively speaking).

This adds some additional dependencies (most of which could be removed) and some nightly features (which cannot be, at least not easily). I'm not sure if compiling under the beta toolchain without RUSTC_BOOTSTRAP is a hard requirement, or if it could be relaxed.

@ecstatic-morse ecstatic-morse force-pushed the new-engine branch 2 times, most recently from 495c96f to bc94adf Compare January 16, 2022 08:02
@ecstatic-morse
Copy link
Author

I'm not sure what the path is to upstream this. It's too big to review on its own, obviously. I might have to work in my fork for a while.

Each section ("naive", "initialization", etc.) is now expressed as a
`Computation`, with defined inputs and outputs. Computations are
organized into `Pipeline`s, which execute each computation in turn,
storing the results in a `Db` where they are accessible to later
computations in the pipeline.

`Computation`s can also dump intermediate results using the
creatively-named `Dump` struct. A series of `Dumper`s can be specified
for a pipeline, each which will be given access to a type-erased
version of the dumped data.
Only the location-insensitive variant computes this relation
Comment on lines +202 to +209
let input = <C::Input<'_>>::load_from_db(db);
dump.unit_start(name);
let start_time = Instant::now();
let output = computation.compute(input, dump);
let end_time = Instant::now();
dump.unit_end(name);

output.store_to_db(db, dump);
Copy link
Author

@ecstatic-morse ecstatic-morse Jan 17, 2022

Choose a reason for hiding this comment

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

Here's the part that requires GATs. Moving the lifetime parameter for Input to Computation itself and changing the signature to the following gives an error:

fn compute_<C, T>(computation: &C, db: &mut Db<T>, dump: &mut Dump<'_>)
where
    for<'db> C: Computation<'db, T>,
    for<'db> <C as Computation<'db, T>>::Input: LoadFrom<'db, T>,
    for<'db> <C as Computation<'db, T>>::Output: StoreTo<T>,
    T: FactTypes,
error[E0502]: cannot borrow `*db` as mutable because it is also borrowed as immutable
   --> polonius-engine/src/pipeline.rs:213:5
    |
206 |     let input = <C::Input>::load_from_db(db);
    |                                          -- immutable borrow occurs here
...
213 |     output.store_to_db(db, dump);
    |     ^^^^^^^-----------^^^^^^^^^^
    |     |      |
    |     |      immutable borrow later used by call
    |     mutable borrow occurs here

This is expected since data from db flows from input into computation and then output. It's perhaps more surprising that this works with a GAT on Computation::Input?

Copy link
Author

@ecstatic-morse ecstatic-morse Jan 17, 2022

Choose a reason for hiding this comment

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

My understanding is that this works with GATs because neither the implementer of Computation nor Computation::Output can name the lifetime used in Computation::Input<'db>. I'm not very confident in this assessment, however. Ironically, this is the kind of complex HRTB case that Polonius cannot handle at the moment.

@dullbananas
Copy link

@ecstatic-morse are you still working on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants