Better graphviz output for SCCs and NLL constraints #123797
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
S-waiting-on-review
|
@bors r+ rollup |
bors
added
S-waiting-on-bors
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Apr 16, 2024
…llaumeGomez Rollup of 14 pull requests Successful merges: - rust-lang#120781 (Correct usage note on OpenOptions::append()) - rust-lang#121694 (sess: stabilize `-Zrelro-level` as `-Crelro-level`) - rust-lang#122521 (doc(bootstrap): add top-level doc-comment to utils/tarball.rs) - rust-lang#123491 (Fix ICE in `eval_body_using_ecx`) - rust-lang#123574 (rustdoc: rename `issue-\d+.rs` tests to have meaningful names (part 6)) - rust-lang#123687 (Update ar_archive_writer to 0.2.0) - rust-lang#123721 (Various visionOS fixes) - rust-lang#123797 (Better graphviz output for SCCs and NLL constraints) - rust-lang#123990 (Make `suggest_deref_closure_return` more idiomatic/easier to understand) - rust-lang#123995 (Make `thir_tree` and `thir_flat` into hooks) - rust-lang#123998 (Opaque types have no namespace) - rust-lang#124001 (Fix docs for unstable_features lint.) - rust-lang#124006 (Move size assertions for `mir::syntax` types into the same file) - rust-lang#124011 (rustdoc: update the module-level docs of `rustdoc::clean`) r? `@ghost` `@rustbot` modify labels: rollup
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Apr 16, 2024
Rollup merge of rust-lang#123797 - amandasystems:better-graphviz, r=oli-obk Better graphviz output for SCCs and NLL constraints This PR modifies the output for `-Z dump-mir-graphviz=yes`. Specifically, it changes the output of the files `.-------.nll.0.regioncx.all.dot` and `nll.0.regioncx.scc.dot` to be easier to read and contain some information that helped me during debugging. In particular: - SCC indices are contracted to `SCC(n)` instead of `ConstraintSccIndex(n)` to compress the nodes - SCC regions are in `{}` rather than `[]` (controversial since they are technically ordered by index, but I figured they're more sets than arrays conceptually since they're equivalence classes). - For regions in other universes than the root, also show the region universe (as ?8/U1) - For regions with external names, show the external name in parenthesis - For the region graph where edges are locations, render the All variant of the enum without the file since it's extremely long and often destroys the rendering - For region graph edge annotations for single locations, remove the wrapping around the Location variant and just add its contents since this can be unambiguously done Example output (from the function `foo()` of `tests/ui/error-codes/E0582.rs`) for an SCC graph:  ...and for the constraints:  This PR also gives `UniverseIndex`es the `is_root()` method since this is now an operation that happens three times in the borrowck crate.
lqd
reviewed
Apr 16, 2024
Comment on lines
+13
to
+26
| fn render_outlives_constraint(constraint: &OutlivesConstraint<'_>) -> String { | ||
| match constraint.locations { | ||
| Locations::All(_) => "All(...)".to_string(), | ||
| Locations::Single(loc) => format!("{loc:?}"), | ||
| } | ||
| } | ||
|
|
||
| fn render_universe(u: UniverseIndex) -> String { | ||
| if u.is_root() { | ||
| return "".to_string(); | ||
| } | ||
|
|
||
| format!("/{:?}", u) | ||
| } |
There was a problem hiding this comment.
These are only used once, so we can inline them in render_region_vid.
There was a problem hiding this comment.
Sure, do you want this enough for me to open a new PR with that change?
|
|
||
| fn render_outlives_constraint(constraint: &OutlivesConstraint<'_>) -> String { | ||
| match constraint.locations { | ||
| Locations::All(_) => "All(...)".to_string(), |
There was a problem hiding this comment.
We might as well remove the ellipsis altogether, wdyt?
There was a problem hiding this comment.
I considered this as my first option, but felt uneasy about leaving something out without showing that’s what I’m doing. Otherwise I don’t think it’s a bad option!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR modifies the output for
-Z dump-mir-graphviz=yes. Specifically, it changes the output of the files.-------.nll.0.regioncx.all.dotandnll.0.regioncx.scc.dotto be easier to read and contain some information that helped me during debugging. In particular:SCC(n)instead ofConstraintSccIndex(n)to compress the nodes{}rather than[](controversial since they are technically ordered by index, but I figured they're more sets than arrays conceptually since they're equivalence classes).Example output (from the function
foo()oftests/ui/error-codes/E0582.rs) for an SCC graph:...and for the constraints:
This PR also gives
UniverseIndexes theis_root()method since this is now an operation that happens three times in the borrowck crate.