Skip to content

Factor out CTermShow class for simpler node printing #4808

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

Merged
merged 34 commits into from
May 20, 2025

Conversation

ehildenb
Copy link
Member

@ehildenb ehildenb commented May 6, 2025

Blocked on: #4819

This is a refactoring to how our display of nodes/KCFGs works, ideally to make it easier to thread options through to them. We're specifically trying to capture all the common options for how CTerm should be displayed in one place, and remove those options from other places. This came about as I was trying to implement some asks from the KMIR team on changing the display in the TUI viewer, and realized that currently all the options are threaded through in different ways and controlled at different points.

This is a breaking change in pyk API, and so corresponding changes into downstream repos will need to be added to the update-deps PRs.

Another big change that happens here: the kcfg directory no longer needs a KPrint because we use a kast.pretty.PrettyPrinter directly instead. Before, it was using KPrint.pretty_print, which was just constructing a PrettyPrinter then using it. So we skip the intermediate class. That means that several classes which took in a KPrint now just take in a KDefinition.

Changes in particular:

  • A new class CTermShow is introduced, which enables printing out CTerm using a PrettyPrinter, via CTermShow.show_config, CTermShow.show_constraints and CTermShow.show. It takes several options controlling how printing can occur, including:
    • extra_unparsing_modules, patch_symbol_table, unalias, and sort_ac_collections: all the same as PrettyPrinter.
    • minimize: collapse unused cell-variables into ..., and the recursively collapse the ... as much as possible in the configuration.
    • break_cell_collections: enable line-breaking collections (List, Set, Map) which are stored directly in cells, as usually you want one item printer per line in this case.
    • omit_labels: a set of labels to always replace occurrences of with ..., so they do not get printed.
  • NodePrinter (and APRProofNodePrinter) now take a CTermShow instead of a KPrint, which also means they don't take the options controlling printing directly (like minimize). These options must be set when building teh CTermShow that is passed in for the NodePrinter to use.
  • Similarly, KCFGShow (and APRProofShow) now take a KDefinition instead of a KPrint and build a PrettyPrinter directly. This means that the NodePrinter (and thus CTermShow) is responsible for setting options related to node printing like sort_collections ahead of time.
  • The KCFGViewer (and APRProofViewer) are now taking directly a KDefinition instead of a KPrint, and similar to the *Show counterparts offloading decisions about how to print items to how the CTermShow is setup in the client application.
  • Tests are updated with the new pattern (of constructing a CTermShow ahead of time and passing it in), which provides an example of what's needed downstream. In particular, before we were mocking an entire KPrint in the unit tests, but now we only need to mock a CTermShow, which is much more compact. We rename the file from mock_kprint.py to mock.py, so it can be for all mocks.

@ehildenb ehildenb self-assigned this May 6, 2025
@rv-jenkins rv-jenkins changed the base branch from master to develop May 6, 2025 15:32
@ehildenb

This comment was marked as outdated.

automergerpr-permission-manager bot pushed a commit that referenced this pull request May 7, 2025
Related: #4808

This is part of an effort to refactor the `[APRProof|KCFG]Show` (and
`Viewer`) heirarchy to allow more control in how states are rendered.
This refactoring makse future refactorings simpler, and can be merged
standalone as being feature-preserving.
@ehildenb

This comment was marked as outdated.

@ehildenb
Copy link
Member Author

ehildenb commented May 13, 2025

Adjustments to import graph.

Before:

k-before

After:

k

In particular, kcfg is no longer importing ktool.

@ehildenb
Copy link
Member Author

I have one more refactoring in mind, which should eliminate the need for eth NodePrinter class heirarchy (in favor of customizing CTermShow class and passing it around), but that should wait for a follow-up and is a bit more invasive.

Once that refactoring s in, our display heirarchy should be significantly simplified.

@ehildenb ehildenb requested a review from gtrepta May 15, 2025 18:05
@tothtamas28
Copy link
Contributor

tothtamas28 commented May 16, 2025

I have one more refactoring in mind, which should eliminate the need for eth NodePrinter class heirarchy (in favor of customizing CTermShow class and passing it around), but that should wait for a follow-up and is a bit more invasive.

I see two issues that I think should be addressed when refactoring these classes.

  1. Parallel class hierarchies: KCFGShow, KCFGViewer, NodePrinter vs APRProofShow, APRProofViewer, APRProofNodePrinter. My current understanding is that the only difference between these is that when printing a proof, there's a different set of node labels we want to show.

  2. Coupling: these printing utilities (KCFGShow, KCFGViewer and the APRPRoof variants) depend on layers of concrete implementations of how a KAST term is printed (NodePrinter, which depends on PrettyPrinter, which depends on KDefinition, ...). Instead, they should define their own interface in terms of abstractions (e.g. a function that takes a Node and a KCFGViewer-specific set of configuration parameters, and returns a str for the node content to be printed). This makes configuration of the tools for a given semantics downstreams much more feasible.

@ehildenb
Copy link
Member Author

  1. I want to tackle this one. I will start by first getting rid of print_node in the NodePrinter heirarchy. Then it would be nice to handle the attrs directly at the KCFG level, but this is tricky because the attributes are different based on how you interpret the KCFG (as a raw KCFG or as an APRProof).
  2. Also agreed. One step/improvement at a time though, this is what I have time for to get KMIR printing to look better, and I want to get it merged downstream all the way first, before introducing more potential breakages.

@ehildenb ehildenb requested a review from tothtamas28 May 16, 2025 16:27
@ehildenb ehildenb requested a review from tothtamas28 May 20, 2025 16:23
@automergerpr-permission-manager automergerpr-permission-manager bot merged commit 0734e45 into develop May 20, 2025
18 checks passed
@automergerpr-permission-manager automergerpr-permission-manager bot deleted the node-print-updates branch May 20, 2025 19:41
automergerpr-permission-manager bot pushed a commit to runtimeverification/mir-semantics that referenced this pull request May 21, 2025
- Switches to using `CTermShow` for controlling node printing:
runtimeverification/k#4808

---------

Co-authored-by: devops <[email protected]>
Co-authored-by: Freeman <[email protected]>
Co-authored-by: Everett Hildenbrandt <[email protected]>
automergerpr-permission-manager bot pushed a commit to runtimeverification/riscv-semantics that referenced this pull request May 21, 2025
- Includes update to how to build an `APRProofShow` so that it doesn't
need a `KPrint` anymore, just a `KDefinition`.
runtimeverification/k#4808

---------

Co-authored-by: devops <[email protected]>
Co-authored-by: Everett Hildenbrandt <[email protected]>
jberthold added a commit to runtimeverification/mir-semantics that referenced this pull request May 21, 2025
These are some improvements to printing that we can implement now that
we have merged changes upstream
runtimeverification/k#4808.

- Improves the printing of spans in the TUI viewer to actually print the
file name and line that it's referring to.
- Passes the entire `DisplayOpts` to the `KMIRAPRNodePrinter`, because
why not?
- Omits printing the `<currentBody>` cell by default in TUI and shower,
but enables turning it back on with `--no-omit-current-body` option to
both.
- Saves off the `SMIRInfo` in the `proof_dir` when we have it available
in `prove-rs`, so that it can be used for printing sourcemap information
in the show/view commands (like current function name and span).

---------

Co-authored-by: devops <[email protected]>
Co-authored-by: Jost Berthold <[email protected]>
tothtamas28 added a commit to runtimeverification/imp-semantics that referenced this pull request May 21, 2025
Update `kframework` to `7.1.257`.
* Includes update according to runtimeverification/k#4808

---------

Co-authored-by: devops <[email protected]>
Co-authored-by: Tamás Tóth <[email protected]>
tothtamas28 added a commit to runtimeverification/zkevm-harness that referenced this pull request May 21, 2025
Update `kriscv` to version `0.1.82`.
* Includes update according to runtimeverification/k#4808

---------

Co-authored-by: devops <[email protected]>
Co-authored-by: Tamás Tóth <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants