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

Remove ResultsVisitable #132134

Merged
merged 2 commits into from
Nov 17, 2024
Merged

Remove ResultsVisitable #132134

merged 2 commits into from
Nov 17, 2024

Commits on Nov 4, 2024

  1. Replace BorrowckResults with Borrowck. 

    The results of most analyses end up in a `Results<'tcx, A>`, where `A`
is the analysis. It's then possible to traverse the results via a
`ResultsVisitor`, which relies on the `ResultsVisitable` trait. (That
trait ends up using the same `apply_*` methods that were used when
computing the analysis, albeit indirectly.)

This pattern of "compute analysis results, then visit them" is common.
But there is one exception. For borrow checking we compute three
separate analyses (`Borrows`, `MaybeUninitializedPlaces`, and
`EverInitializedPlaces`), combine them into a single `BorrowckResults`,
and then do a single visit of that `BorrowckResults` with
`MirBorrowckResults`. `BorrowckResults` is just different enough from
`Results` that it requires the existence of `ResultsVisitable`, which
abstracts over the traversal differences between `Results` and
`BorrowckResults`.

This commit changes things by introducing `Borrowck` and bundling the
three borrowck analysis results into a standard `Results<Borrowck>`
instead of the exceptional `BorrowckResults`. Once that's done, the
results can be visited like any other analysis results.
`BorrowckResults` is removed, as is `impl ResultsVisitable for
BorrowckResults`. (It's instructive to see how similar the added `impl
Analysis for Borrowck` is to the removed `impl ResultsVisitable for
BorrowckResults`. They're both doing exactly the same things.)

Overall this increases the number of lines of code and might not seem
like a win. But it enables the removal of `ResultsVisitable` in the next
commit, which results in many simplifications.
    @nnethercote
    nnethercote committed Nov 4, 2024
    Configuration menu
    Copy the full SHA
    3350edf View commit details
    Browse the repository at this point in the history

  2. Remove ResultsVisitable. 

    Now that `Results` is the only impl of `ResultsVisitable`, the trait can
be removed. This simplifies things by removining unnecessary layers of
indirection and abstraction.

- `ResultsVisitor` is simpler.
  - Its type parameter changes from `R` (an analysis result) to the
    simpler `A` (an analysis).
  - It no longer needs the `Domain` associated type, because it can use
    `A::Domain`.
  - Occurrences of `R` become `Results<'tcx, A>`, because there is now
    only one kind of analysis results.

- `save_as_intervals` also changes type parameter from `R` to `A`.

- The `results.reconstruct_*` method calls are replaced with
  `results.analysis.apply_*` method calls, which are equivalent.

- `Direction::visit_results_in_block` is simpler, with a single generic
  param (`A`) instead of two (`D` and `R`/`F`, with a bound connecting
  them). Likewise for `visit_results`.

- The `ResultsVisitor` impls for `MirBorrowCtxt` and
  `StorageConflictVisitor` are now specific about the type of the
  analysis results they work with. They both used to have a type param
  `R` but they weren't genuinely generic. In both cases there was only a
  single results type that made sense to instantiate them with.
    @nnethercote
    nnethercote committed Nov 4, 2024
    Configuration menu
    Copy the full SHA
    c904c6a View commit details
    Browse the repository at this point in the history