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

Prettyprinter #554

Merged
merged 23 commits into from
Jun 20, 2024
Merged

Prettyprinter #554

merged 23 commits into from
Jun 20, 2024

Conversation

clyben
Copy link
Collaborator

@clyben clyben commented May 8, 2024

Add a pretty printer which either output to rust macro or let-binded egglog.
Pretty printer could take a string of egglog output and parse it under current schema to RcExpr, or an RcExpr of program, output the rust macro or let-binded egglog.
Add runmode PrettyPrint and OptimizedPrettyPrint, which output the rust macro form of original program or optimized program.
To make rust macro look better copy it to rust file and "cargo fmt". Though you need to add ".with_arg_types(..)" and "add_ctx()" to make it runnable.

@clyben clyben requested a review from oflatt May 8, 2024 23:24
Copy link
Member

@oflatt oflatt left a comment

Choose a reason for hiding this comment

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

This PR is waiting on a refactor, leveraging the existing pretty printer for s-expressions.

@clyben clyben force-pushed the prettyprint branch 2 times, most recently from 214c313 to 53a35fd Compare May 17, 2024 07:39
@clyben clyben requested a review from oflatt May 17, 2024 08:23
Copy link
Member

@oflatt oflatt left a comment

Choose a reason for hiding this comment

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

This is promising, but we should simplify it before we merge

dag_in_context/src/pretty_print.rs Outdated Show resolved Hide resolved
dag_in_context/src/pretty_print.rs Outdated Show resolved Hide resolved
dag_in_context/src/pretty_print.rs Outdated Show resolved Hide resolved
dag_in_context/src/pretty_print.rs Outdated Show resolved Hide resolved
dag_in_context/src/pretty_print.rs Outdated Show resolved Hide resolved
dag_in_context/src/pretty_print.rs Outdated Show resolved Hide resolved
dag_in_context/src/pretty_print.rs Outdated Show resolved Hide resolved
dag_in_context/src/pretty_print.rs Outdated Show resolved Hide resolved
dag_in_context/src/pretty_print.rs Show resolved Hide resolved
dag_in_context/src/pretty_print.rs Outdated Show resolved Hide resolved
oflatt
oflatt previously requested changes May 21, 2024
Copy link
Member

@oflatt oflatt left a comment

Choose a reason for hiding this comment

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

Very close now!

dag_in_context/src/pretty_print.rs Outdated Show resolved Hide resolved
dag_in_context/src/pretty_print.rs Outdated Show resolved Hide resolved
dag_in_context/src/pretty_print.rs Show resolved Hide resolved
dag_in_context/src/pretty_print.rs Show resolved Hide resolved
dag_in_context/src/pretty_print.rs Outdated Show resolved Hide resolved
dag_in_context/src/pretty_print.rs Show resolved Hide resolved
}
}

pub fn map_expr_children<F>(self: &RcExpr, mut map_child: F) -> RcExpr
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe leaving a note saying that these functions may violate the invariant that RcExpr is interned (as mentioned in RcExpr's doc.

let single_v7 = single(getat(2));
let single_v8 = single(getat(3));
let sub_v9 = sub(getat(2),
getat(1));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you check if this getat(1) has a different context as single_v6. Also, could you document what is the current behavior of sharing for the pretty printer? My understanding is that it's not perfect (although pretty usable).

@yihozhang
Copy link
Collaborator

action items for this PR after a discussion:

  • add caveats that map_* may violate RcExpr's invariant.
  • document behaviors/limitations of the current pretty printer (inlining parallel!, sharing failure when having different contexts)
  • Ignore all assumptions when pretty printing to Rust
    • user will add their own top-level types and assumptions

@clyben clyben requested a review from yihozhang June 18, 2024 22:28
@yihozhang yihozhang dismissed oflatt’s stale review June 20, 2024 04:41

all the review comments should have been addressed now

@yihozhang yihozhang merged commit b919186 into egraphs-good:main Jun 20, 2024
2 checks passed
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.

None yet

3 participants