-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
[Relay] Add ADTs to text format #3863
Conversation
Thank you for adding tests for the additional features. Higher-level point of discussion: What syntax do we want to settle on for text format type definitions? I like the style @weberlo used here (similar to OCaml's), but it may contrast a bit with other syntax we have (e.g., it doesn't use curly braces). Also on the subject of syntax, I think it may be a little redundant to include the line separators One reason I am raising these questions is so we can update our documentation to be consistent with the syntax we actually implement. |
@slyubomirsky I agree on the vertical bars in If we remove the bars from enum WebEvent {
// An `enum` may either be `unit-like`,
PageLoad,
PageUnload,
// like tuple structs,
KeyPress(char),
Paste(String),
} and we could just replace We could also allow optional braces in match arms, which would be useful for sequencing.
The last minor point is that if we're going with Rust syntax, should we also require commas after match arms? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please extend graph_equal for this.
also:maybe change the name of graph_equal to something better (freevars_eq e.x.) |
@MarisaKirisame Is there not a more standard PL name for the type of equality that |
@weberlo alpha_equal_up_to_free_vars. unifiable is also good. |
* NOTE: The `USE_ANTLR` option in `config.cmake` must be enabled in order for | ||
* changes in this file to be reflected by the parser. | ||
* NOTE: All upper-case rules are *lexer* rules and all camel-case rules are *parser* rules. | ||
*/ | ||
|
||
grammar Relay; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update the version number to 0.0.4.
On syntax, I like the idea of optional braces but we should be certain that this will not cause problems for round-tripping. I think commas in type definitions and match definitions in the Rust style might be good in that they more closely resemble C-like syntax |
@slyubomirsky @MarisaKirisame I took a crack at implementing Rust-style syntax in the most recent commits. Take a look when you get chance. |
|
||
Doc adt_body; | ||
adt_body << PrintSep(constructor_docs, separator); | ||
// add trailing comma if there are any constructors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why trailing comma?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
idk. it's not necessary. just a matter of style.
I've just added support for external type declarations (example syntax is |
src/relay/ir/pretty_printer.cc
Outdated
} | ||
|
||
// TODO(weberlo): Consolidate this method and `IsAtomic` in `pass_util.h`? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be a good idea if we can avoid repeating ourselves, though are we certain that these definitions (something we will always inline and atomic expressions) will always coincide?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. I was originally thinking it might be a good idea, but I'm not convinced they should be merged anymore.
|
||
|
||
def test_extern_adt_defn(): | ||
# TODO(weberlo): update this test once extern is implemented |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we shouldn't include the syntax while we don't have the feature implemented (e.g., leave it for a different PR)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jroesch is planning on updating it very soon for the VM. He just wanted me to add all of the necessary boilerplate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need this change ASAP, I just want the parsing changes to be shipped in a unit to keep things clean.
now cleanup?
8f63f9d
to
0ea6870
Compare
* Getting closer to having ADT defs * ADT defs working probly * Match parsing basipally done * came to earth in a silver chrome UFO * match finished? * All tests but newest are passing * ADT constructors work now cleanup? * Cleanup round 1 * Cleanup round 2 * Cleanup round 3 * Cleanup round 4 * Cleanup round 6 * Cleanup round 7 * Lil grammar fix * Remove ANTLR Java files * Lint roller * Lint roller * Address feedback * Test completeness in match test * Remove unused imports * Lint roller * Switch to Rust-style ADT syntax * Lil fix * Add dummy `extern type` handler * Add type arg to test * Update prelude semantic version * Repair test * Fix graph var handling in match * Revert 's/graph_equal/is_unifiable' change
* Getting closer to having ADT defs * ADT defs working probly * Match parsing basipally done * came to earth in a silver chrome UFO * match finished? * All tests but newest are passing * ADT constructors work now cleanup? * Cleanup round 1 * Cleanup round 2 * Cleanup round 3 * Cleanup round 4 * Cleanup round 6 * Cleanup round 7 * Lil grammar fix * Remove ANTLR Java files * Lint roller * Lint roller * Address feedback * Test completeness in match test * Remove unused imports * Lint roller * Switch to Rust-style ADT syntax * Lil fix * Add dummy `extern type` handler * Add type arg to test * Update prelude semantic version * Repair test * Fix graph var handling in match * Revert 's/graph_equal/is_unifiable' change
* Getting closer to having ADT defs * ADT defs working probly * Match parsing basipally done * came to earth in a silver chrome UFO * match finished? * All tests but newest are passing * ADT constructors work now cleanup? * Cleanup round 1 * Cleanup round 2 * Cleanup round 3 * Cleanup round 4 * Cleanup round 6 * Cleanup round 7 * Lil grammar fix * Remove ANTLR Java files * Lint roller * Lint roller * Address feedback * Test completeness in match test * Remove unused imports * Lint roller * Switch to Rust-style ADT syntax * Lil fix * Add dummy `extern type` handler * Add type arg to test * Update prelude semantic version * Repair test * Fix graph var handling in match * Revert 's/graph_equal/is_unifiable' change
* Getting closer to having ADT defs * ADT defs working probly * Match parsing basipally done * came to earth in a silver chrome UFO * match finished? * All tests but newest are passing * ADT constructors work now cleanup? * Cleanup round 1 * Cleanup round 2 * Cleanup round 3 * Cleanup round 4 * Cleanup round 6 * Cleanup round 7 * Lil grammar fix * Remove ANTLR Java files * Lint roller * Lint roller * Address feedback * Test completeness in match test * Remove unused imports * Lint roller * Switch to Rust-style ADT syntax * Lil fix * Add dummy `extern type` handler * Add type arg to test * Update prelude semantic version * Repair test * Fix graph var handling in match * Revert 's/graph_equal/is_unifiable' change
This PR tackles some of item 0 from this issue, using this RFC as a reference.
One notable deviation is that
match
expressions can be specified as incomplete (i.e., not requiring all cases to be covered) by writingmatch?
.The tests in
test_ir_parser.py
currently do not pass if I don't inline every expression in the pretty printer. This is because when graph vars are created, scoping becomes more complicated. For example, the following programwould be pretty-printed like so
. While this can still be parsed (by delaying evaluation of the graph expr until the context of its usage is available), it seems to add additional complexity without any benefits that I'm aware of. It also makes testing equality with the original program more difficult.
Hopefully, this brings us closer, if not all the way there, to writing Relay's prelude in the text format.
CC @jroesch @MarisaKirisame @joshpoll @slyubomirsky @junrushao1994