Algebraic types extension #107
Conversation
There was a problem hiding this comment.
Super cool; thanks for the PR! I do think it would make sense to reorganize things so they get integrated with the rest of the tools… for most of this stuff, there's not much ADT-specific about them!
There are two design things that I am currently worried about, noted within:
- The abstract syntax for
constructoverloadsargsto contain not just variable names but also string-encoded integers. This is a one-off exception to a rule that otherwise serves us pretty well. TBH I am not sure what to do about this, because you do want to have a static integer as a parameter there. This requires some creativity. 😕 Maybe we want instructions to optionally have a separate list of static parameters for cases like this. - The concrete syntax for parameterized types is a little sad because there is a special case for one-argument types vs. everything else. This isn't too bad though because it's just about the text format, not the AST.
One other thing that will need to happen before merging is adding to the documentation. We will need book pages for the language extension and each of the new tools.
|
|
||
| Extra tools and samples for the Bril algebraic types extension. | ||
|
|
||
| - `briltc`: Bril type-checker with support for algebraic types |
There was a problem hiding this comment.
Seems like this can just be at the top level? We do have type-infer there, and this is similar to that but different. The fact that it supports the ADT extension is not terribly important; someday, people might extend other stuff with similar support (or extend this with support for other extensions).
| - `briltc`: Bril type-checker with support for algebraic types | ||
| - `rs2bril`: compiler from Rust syntax to Bril | ||
| - `samples`: sample Bril programs using algebraic types | ||
| - `samples-rs`: sample Rust programs |
There was a problem hiding this comment.
Those can be thought of as tests for rs2bril, right?
|
|
||
| - `briltc`: Bril type-checker with support for algebraic types | ||
| - `rs2bril`: compiler from Rust syntax to Bril | ||
| - `samples`: sample Bril programs using algebraic types |
There was a problem hiding this comment.
Seems like maybe these should be included in the test suite? Or are they different?
| Extra tools and samples for the Bril algebraic types extension. | ||
|
|
||
| - `briltc`: Bril type-checker with support for algebraic types | ||
| - `rs2bril`: compiler from Rust syntax to Bril |
There was a problem hiding this comment.
Seems like this deserves to be at the top level too. It's a sibling to ts2bril.
| @@ -0,0 +1,14 @@ | |||
|
|
|||
There was a problem hiding this comment.
I know Makefiles are cool, but these are kinda the standard commands one would run for any Cargo-based Rust project. So maybe not necessary?
| @@ -0,0 +1,7 @@ | |||
|
|
|||
There was a problem hiding this comment.
I would love to integrate all these tools in with the rest of the tools here, rather than keeping them in a separate ADT-specific sandbox. Justifications below.
| throw error("construct destination must be a sum type"); | ||
| } | ||
| let value = getArgument(instr, state.env, 0); | ||
| let index = parseInt(instr.args[1]); |
There was a problem hiding this comment.
TBH, I am still a little concerned about this syntax change. I don't have an alternative to propose, unfortunately, but it is pretty weird that for every other instruction, arguments are variable names, but for this one special instruction, the argument is a string that encodes a decimal integer and not a variable name at all. For example, consider writing a def/use analysis: you would now have to say "all the things in args are variable uses, except for this one instruction case, where one of the arguments does not name a variable."
Like I said, not sure what to do about this, but I think we may want to reconsider the abstract syntax for this instruction…
| @@ -29,16 +30,17 @@ | |||
| eop.2: op ";" | |||
| label.1: LABEL ":" | |||
|
|
|||
| op: IDENT (FUNC | LABEL | IDENT)* | |||
| op: IDENT (FUNC | LABEL | IDENT | SIGNED_INT)* | |||
There was a problem hiding this comment.
This is the concrete-syntax reflection of the abstract-syntax complaint I have above.
| type: IDENT "<" type ("," type)* ">" -> paramtype | ||
| | IDENT "<" ">" -> paramtype |
There was a problem hiding this comment.
Seems legit, but maybe this grammar could be a little simpler to allow "zero or more" types? (As in other locations, such as function arguments, where we allow zero or more of things.)
| if len(items) == 2: | ||
| return {items[0]: items[1]} |
There was a problem hiding this comment.
Ah, this is a little worrisome. It means it's not possible to have one-argument parameterized types. Like, doesn't product<int> now do something wrong (even if that's not a very practical type)? Not sure what to do about this, TBH… maybe the whole parameterized-type thing needs rethinking. 🙃
Work from my final project.
Adds the following algebraic type extension features:
briliandbril-txtbriltcrs2brilThe standalone tools are currently in the
algebraic-typesdirectory, but I can reorganize things if desired.