Allow a MIR analysis to perform the state join directly

[Jarcho]

AnalysisJoin is a separate trait in order for the domain type to provide the join via JoinSemiLattice. This also prevents a custom join implementation when using a JoinSemiLattice type as the domain, but that's probably not useful to do in the first place.

---

This is needed to make clippy::redundant_clone an analysis pass. Given the following:

let x = String::new();
let y = &x;
let z = if true {
    y.clone();
} else {
    String::from("foo")
};
println!("{y} {z}");

To avoid linting on the clone, the join between the two branches needs to be able to see that x is borrowed at that point and cannot be moved. Since borrowing is a different analysis pass this needs to be passed into the join somehow.

[rustbot]

r? @davidtwco

(rustbot has picked a reviewer for you, use r? to override)

[rustbot]

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

[cjgillot]

The dataflow framework relies on the semi-lattice traits to model the mathematical concept. This is required for correctness and termination.

What does it mean for a lattice to depend on a context parameter?

Such context must be part of the dataflow analysis State, instead of the join impl.

[Jarcho]

The context here would be any supplemental data that doesn't change during the analysis. For the specific case it would be the borrow state calculated by a previous analysis.

I'm now just realizing that this needs to be a mutable reference anyways for clippy. Specifically this function is needed. Note that &mut self is used as a perf enhancement to avoid allocating memory on every call.

---

To more precisely describe the join, a state such containing the following

x = `1
y = `1
x was cloned into y

is merged with

x = `1
y = `2

at which point y is considered to be a clone of x which has been modified. If x is never read afterwards, then the clone is not needed. In order to determine this we have to know if x is borrowed when the join occurs. The analysis of what is borrowed where is already performed we just need to look up the result at the point the two states are merged.

[bors]

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

[davidtwco]

r? @cjgillot

[Jarcho]

Upon further work even taking the analysis by mutable reference isn't enough as the location of the join is also needed. The new implementation adds a join function to the analysis with a default implementation using JoinSemiLattice when implemented by the domain type.

[cjgillot]

This looks like you want a custom version of Direction::join_state_into_successors_of, don't you?

[Jarcho]

join_state_into_successors_of doesn't have access to the destination state. The closure created in iterate_to_fixpoint is where that's accessed.

[apiraino]

@Jarcho do you need more input about the design, here? AFAICS the discussion is progressing for now in rust-clippy#11364.

Anyway, reassigning to you as a nudge to resolve conflicts. Feel free to request a review with @rustbot ready when you feel this is unblocked, thanks!

@rustbot author

[cjgillot]

TBH, I'm quite unconvinced by the need for this PR.
As it works around the abstraction that the dataflow engine relies on, I'm leaning towards rejecting it.
I could be convinced with a clear definition of the proposed abstraction, and how it incorporates with the dataflow algorithm.

[Jarcho]

The fundamental abstraction it relies on is: there exists some function f(x,y) -> z such that z >= x and z >= y. This is the case both with and without this PR.

I don't know if you looked at this after the last change I made. Current implementation is for the analysis to implement the join. There is a default implementation for the case where the domain implements JoinSemilattice to simplify the common case. The AnalysisJoin trait exists to allow this default implementation.

Some change here is still seems needed. Don't know if you saw this comment: rust-lang/rust-clippy#11364 (comment).

[Dylan-DPC]

@Jarcho any updates on this?

[Jarcho]

@cjgillot I guess you aren't seeing notifications from the thread in clippy's repo. The last suggestion you had has a small downgrade to the lint's capabilities.

[Jarcho]

@rustbot ready

This is still waiting on a response.

[cjgillot]

Your answer is a PR on clippy repo. I made several comments on that pass, mostly asking to document the design and suggesting to decouple the analysis in several steps. Before we modify the dataflow API, I'd like to understand why it needs to be modified.
All the discussion is "how do we materialize the lattice data structure ?" At the very least I need a description of the lattice you use in your other PR.

[Jarcho]

Your answer is a PR on clippy repo.

My answer to what question? The only question that could answer is 'What am I working on that needs these changes?".

I made several comments on that pass, mostly asking to document the design and suggesting to decouple the analysis in several steps.

I have also been responding to all of those.

All the discussion is "how do we materialize the lattice data structure ?" At the very least I need a description of the lattice you use in your other PR.

I'll have to get back to you on that since it's now been several months since I've last worked on this.

[bors]

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

[Dylan-DPC]

@Jarcho @cjgillot what's the status of this? thanks

[apiraino]

by reading the latest comments, I get the feeling that this PR is waiting on more design work

@rustbot author

[Jarcho]

Coming back to this again. Since it's been a while I'm going to just repeat some things here.

---

This adds a way to pass additional parameters when performing the join between two states. As a non-exhaustive list this includes things like the MIR body, which block this state is for, precalculated values/tables, and preallocated scratch memory. Basically this is for any additional data needed that's the same for all the values in the graph, and as such, should not be stored per node; or to amortize the cost of doing something as is the case for allocating scratch memory.

For the implementation this adds a single new trait, AnalysisJoin, which replaces JoinSemilattice as the implementation of the join. There is a default implementation of AnalysisJoin when the domain type implements JoinSemilattice. This way AnalysisJoin won't need to be implemented most of the time. This default implementation is also the reason it isn't just another method on Analysis itself.

---

Moving to the clippy analysis, it needs to keep track of three things:

1. For each local, which other locals it is a clone of or cloned from (a bidirectional link is needed). For each of these we also need to track which clone call created this link.
2. Which locals are part of a clone pair, but their counterpart has been mutated/consumed. This would also track which clone call created the links tracked here.
3. A way to track when a value has changed after it has been cloned.

Joining two states consists of merging the two lists (1 & 2) and then merging the set of tracked values. If, when merging, there is a local with two different values then a new value will have to be created and the appropriate entries in the second list need to be added.

To get the results we start from a set of all clone calls. While visiting the results Any read from a local in the second list removes the associated clone calls from the set. Anything remaining didn't need to be a clone.

Ideally the first list would have a representation like Vec<SmallVec<Location>> where each index in the Vec corresponds to a pair of locals. The SmallVec is there just because it's possible (though unlikely) that the same link can be created by different clone calls. If this can be compressed to have indices just for locals which are actually linked together at some point in the analysis then this will be cut down to a reasonable size (it's $n^2$ normally). This set, however, is not known until after the analysis has finished meaning the indices have to be allocated as the analysis is run. The join will need to have access to the analysis in order to map the index to the pair of locals involved.

At some point in the future the lint will also be extended to support projections. Field and array index projections don't change things too much other than increasing the number of values being tracked. Deref projections would be a whole other issue though.