search

brownplt / pyret-lang

The Pyret language.
Other
1.07k stars 111 forks source link

Add annotation for table-from-raw-array(), checking arg is indeed a raw array of rows #1759

Open ds26gte opened 2 months ago

ds26gte commented 2 months ago

addresses issue #1758

jpolitz commented 2 months ago

Thanks!

A few tests here would help make it clear what this is for (and verify the right thing is happening automatically): https://github.com/brownplt/pyret-lang/blob/horizon/tests/pyret/tests/test-tables.arr#L653

You can see some similar style of tests (for dynamic annotation-checking errors in the table library) here: https://github.com/brownplt/pyret-lang/blob/horizon/tests/pyret/tests/test-tables.arr#L507

ds26gte commented 2 months ago

Just one test added. Is there more variety needed in this situation?

blerner commented 2 months ago

This looks like it merely tests that the argument is a raw array; it doesn't check that the elements are checked as well.

ds26gte commented 2 months ago

@blerner, are you referring to having a test case that checks that something like 

  [table-from-rows:
    [raw-row: 5, {"B"; 7}, {"C"; 8}],
    [raw-row: {"A"; 1}, {"B"; 2}, {"C"; 3}]]

fails?

Or are you saying that the predicate I used in tables.arr is merely checking that the argument is a raw-array of raw-rows, but doesn't further check that each raw-row contains only key-value pairs? The latter is already taken care of by the existing code once the initial check that it's all raw-rows passes.

I can add the test case above to test-tables.arr, but you already have in place a more stringent test, i.e., that the key-value pairs have matching keys in the correct order across all the rows. This new test is pretty basic in comparison.

blerner commented 2 months ago

I'm saying you have no test case that fails in the is-row check of https://github.com/brownplt/pyret-lang/pull/1759/files#diff-9e511711584173e90288d8fe12034bea103c3de800fdd9b50fcf38dd0479b273R92. So [table-from-rows: 1, [raw-row: ...], false, [raw-row: ...], "a string that's definitely not a row", ...].

Additionally, implementing the annotation in Pyret as a refinement will give you a lousier error message than if you implemented it as an annotation in JS, as we do in the image library, as I pointed you towards in the original issue.

ds26gte commented 2 months ago

Shriram's original example (the one that used a [list: ...] around the [raw-rows: ...]s) -- which I'd already added to the tests actually covers this case. But I've added another, more explicit, example that matches yours.

My question had however to do with the other type of error, where the table-from-rows arguments are indeed raw-rows, but the content of the rows are not tuples, let alone tuples with matching keys. If I'm reading you correctly, you don't want this be tested by the type annotation of table-from-raw-array. It will eventually be caught by the %(is-kv-pairs) or :: KeyValPair<C?>.

I looked at the annListColor defined and used in CPO's internal-image-typed.js. It is used internally within the JS, not exported to be used in an .arr file. If I'm matching that behavior for pyret-lang's tables, I could define a predicate and export it as a method of a table-row (similar to get-column-names()) but this seems wasteful. Wouldn't it make sense to use such a predicate immediately within tables.js (not tables.arr), viz., inside the body of makeRowFromArray(), which doesn't do much checking. (I do see @jpolitz's comment suggesting tests should not be done here though.)