Add support for units of measure #1454
Conversation
|
Just wanted to say how very awesome this is. Thanks! (Been discussing it w/ @blerner .) |
|
Totally agreed with Shriram. This is very cool. Bug report: returns true. |
| return RUNTIME.getField(ast, 'u-base') | ||
| .app(pos(node.pos), name(node.kids[0])) | ||
| } else if (node.kids.length === 1 && node.kids[0].name === 'NUMBER') { | ||
| if (node.kids[0].value !== '1') { |
There was a problem hiding this comment.
This would probably be better handled as a well-formedness error later, rather than a parse-error now. Also, 1%<3> currently triggers an internal error rather than any clean error, so there's some more cleanup needed here.
There was a problem hiding this comment.
My only hold-up with making this a wf check is that we have to change u-one(l) into u-num(l, n). Not a major issue, something just feels weird about holding onto this value in the AST just for the sake of a wf check. Do you think it's worth holding onto?
|
Is there a predicate that distinguishes |
|
@sorawee You would have to use the There's no function like |
| unit-atom: (PARENNOSPACE|PARENSPACE) unit-expr RPAREN | ||
| | NAME | ||
| | unit-atom CARET NUMBER | ||
| | NUMBER |
|
IMO, annotation is for assertion, while predicate is for control, so they are not really equivalent. But yeah, non-first class value seems to be the problem here. |
|
Having a predicate here would allow programs to be non-parametric in their handling of units. The only two sensible behaviors I'd like are either "this function handles exactly this unit" or "this function is parametric over units". Anything else would allow students to be inconsistent with their units (e.g. "darn it this time it had the wrong units, ok, lemme check for that and cancel them out", instead of "wait why do I have inconsistent units in the first place?"), which seems counterproductive... |
BenMusch
force-pushed
the
units-of-measure
branch
from
f28e8bf
to
75a1dc9
Compare
|
Took some screenshots to showcase what the different runtime errors look like. I hard-coded conditional branches to be true/false to cover all of the statements in the rendering functions. Sorry for the large images. Errors when stack locs and asts are available:
When the ast is not available:
When no source file is available:
When no stack loc is available:
When 'is-builtin` is true:
|
|
Looks good. The main grammatical complaint is the Test 1 failures, where the last sentence just begins, "unit annotated ...", rather than "The unit annotated", or better yet, "The |
We need to return quickly if the arguments are identical and are not numbers, but need to avoid the identical-ness check for numbers so that we can return false in the case of within(1%<m>)(1%<s>, 1%<s>). The previous code had a logic bug where it skipped identical-ness checks on all within() calls, when it should only skip them for within() calls on numbers
PR for the work demo'd here
This adds support for units of measure in the runtime type system, but not for the static type-checker. There is currently no way to "define" a unit. Instead, the base unit values are effectively treated as strings and checked for literal equality
Most of it is complete to my knowledge, with some notes:
Runtime error messages are not complete in theFixed on 6/21render-fancy-reasonmethod. They all callrender-reasonright now, but I plan on changing thatpitometer/*files, I'll do that once the work here is finalized so I don't have to re-do any work as I change any of thesrc/*code