Cover compile time errors in tests

[countvajhula]

Summary of Changes

Qi has never quite managed to reach 100% test coverage because there are some parts of the code that are only hit at compile time (e.g. syntax errors), which I didn't know how to write unit tests for. Michael recently mentioned convert-compile-time-error which converts a compile-time error into a runtime error, allowing us to write unit tests for it. Now with this available, we should be able to get to 100% test coverage.

Public Domain Dedication

• In contributing, I relinquish any copyright claims on my contribution and freely release it into the public domain in the simple hope that it will provide value.

(Why: The freely released, copyright-free work in this repository represents an investment in a better way of doing things called attribution-based economics. Attribution-based economics is based on the simple idea that we gain more by giving more, not by holding on to things that, truly, we could only create because we, in our turn, received from others. As it turns out, an economic system based on attribution -- where those who give more are more empowered -- is significantly more efficient than capitalism while also being stable and fair (unlike capitalism, on both counts), giving it transformative power to elevate the human condition and address the problems that face us today along with a host of others that have been intractable since the beginning. You can help make this a reality by releasing your work in the same way -- freely into the public domain in the simple hope of providing value. Learn more about attribution-based economics at drym.org, tell your friends, do your part.)

[countvajhula]

This is ready for review. We are now almost at 100% coverage! There are just a few lines missing in deforest.rkt. For some of them, it looks like they aren't being hit right now when we try expressions like (~>> (1 2 3 4) (range _) (filter odd?) (map sqr)) in the REPL. I'm not sure if that is intentional.

In increasing coverage, it turned out that a lot of lines that weren't covered were indicative of a real problem, so this PR includes some of those fixes as well, and it also uncovered a potential problem:

It looks like we aren't deforesting nested positions, so something like (☯ (>< (~>> (filter odd?) (map sqr)))) is not being deforested. I've added a failing test that shows this. My impression is that since we are using find-and-map/qi in deforest-pass, it should match and transform nested positions. The unit tests for find-and-map/qi in qi-test/tests/compiler/util.rkt seem to confirm this.

You can get a coverage report locally by running make cover btw.

Any input appreciated!

[countvajhula]

So, I ran into the issue that @dzoep had a little while ago suspected might be present, which is, premature termination of compiler passes upon receiving, or not receiving, a false return value.

The commit message fixing the issue explains it well:

Both `fix` and `find-and-map` apply the same type of function (i.e. a
compiler rewrite rule) to syntax, but they have incompatible
expectations about the return value. Specifically, `fix` terminates on
a false return value, while `find-and-map` continues. This reconciles
them so that they both terminate upon receiving false, and both
continue if the transformed syntax is identical to the original.

Unfortunately, there is now a weird failing test, and I don't understand what is going on with it. I added some logs to normalize-rewrite to see what sequence of expressions it is receiving, and weirdly enough, for this input expression:

(thread tee collect)

... somewhere in the course of traversing the expression using find-and-map/qi and applying normalization-rewrite (even if we remove fix), it seems to pass this expression to normalization-rewrite:

(tee collect)

This doesn't look right since it isn't a true syntax node in the input expression but more of a "sublist." In any case, once it receives this syntax, it matches this normalization rule:

    ;; trivial tee junction
    [(tee f)
     #'f]

... which results in the containing expression becoming:

(thread . collect)

... resulting in the error:

; .../qi/qi-test/tests/flow.rkt:1562:3: thread: bad syntax
;   in: (thread . collect)

The debug logs I added show this sequence of syntax objects passed to normalize-rewrite:

input syntax to normalization is #<syntax:flow.rkt:1562:3 (thread tee collect)>
#<syntax:flow.rkt:1562:3 (thread tee collect)>
#<syntax:/Users/siddhartha/work/qi/qi-lib/flow/extended/expander.rkt:43:5 thread>
#<syntax:.../compile/syntax-spec.rkt:101:11 (tee collect)>
#<syntax:flow.rkt:1562:10 collect>
; /Users/siddhartha/work/lisp/racket/qi/qi-test/tests/flow.rkt:1562:3: thread: bad syntax
;   in: (thread . collect)

For comparison, when I manually evaluate all the necessary functions and then invoke them like so:

> (find-and-map/qi normalize-rewrite
                   #'(thread tee collect))
#<syntax:flow.rkt:1688:29 (thread tee collect)>
#<syntax:flow.rkt:1688:30 thread>
#<syntax:flow.rkt:1688:37 tee>
#<syntax:flow.rkt:1688:41 collect>
#<syntax:flow.rkt:1688:29 (thread tee collect)>

... the output looks sensible. In particular, it never passes (tee collect) to normalize-rewrite.

One unusual thing in the traced output is that the offending (tee collect) syntax seems to come from .../compile/syntax-spec.rkt. I'm not sure I understand how this module could be involved here...

In case you're back from vacay @michaelballantyne , would love your thoughts! ⛷️ 😼

[benknoble]

I seem to recall a (cdr (syntax->list stx)) in one of the syntax parsers; that could be why it’s getting the sublist? So maybe that function needs to always return a syntax list (so the tee would return #'(f)), or else that function should not do the cdr madness?

[benknoble]

Related: isn’t (~> -< collect) a bit of a syntax error? I don’t think I’ve ever seen a tee without child flows.

[countvajhula]

I seem to recall a (cdr (syntax->list stx)) in one of the syntax parsers; that could be why it’s getting the sublist? So maybe that function needs to always return a syntax list (so the tee would return #'(f)), or else that function should not do the cdr madness?

Good idea. Do you mean this one? That is happening at the code generation stage (i.e. qi0->racket), whereas I believe this error is happening during normalization, i.e. in the optimize-flow stage, so this seems unlikely to be the cause.

I'm also suspicious of this part of find-and-map, where we apply a transformation to the syntax list during tree traversal.

Related: isn’t (~> -< collect) a bit of a syntax error? I don’t think I’ve ever seen a tee without child flows.

Yeah I didn't recognize it at first either 😆 . But then I remembered that we'd added support for -< to treat its first input as a "control input" specifying the number of tines to create when used in identifier form. This was to allow it to serve as a core form that fanout could compile to, IIRC.

[benknoble]

I'm concerned about prettify/partition/try and keeping all the passes straight, as well as a couple minor refactoring questions.

A few comments are just some comments trying to explain to myself where things have gone :)

[benknoble]

I wouldn't mind if commit 9956d74 explained why we stop wrapping the partition and try sub-forms, esp. if this function is ever used for output (like in the contract machinery for error reporting?).

Indeed, I see the later comment about jumbling… I would have expected the template to be (partition [e1-pretty e2-pretty] ...) and similar for try.

[countvajhula]

The e1-pretty syntax is already a syntax list, so it comes with its own parens 😄 . The wrapping in the template was causing it to have an extra set of parens.

This ✨ de-expander ✨ , although a top of the line chrome plated model 🏎️ , is super hacky and we're hoping to eliminate it pretty soon as Michael is working on adding syntax tracking to Syntax Spec that will give us access to source syntax "the right way."

[benknoble]

This still seems wrong, like it jumbles the patterns... wouldn't it appear extra bizarre with 3 try clauses? I think the version I suggested with ellipses might work fine, modulo any finagling to make list of syntax work I suppose.

[countvajhula]

Ah yes, you're right.

[benknoble]

Likewise, I'd expect these tests to reflect structure with more variation, like (partition [a b] [c d]) and (try [a b] [c d]).

[countvajhula]

ditto, this is just for basic test coverage of the de-expander. Prettify/etc. is definitely a temporary solution to be able to provide source-syntax-level error messages and we'll hopefully have a proper solution soon.

[benknoble]

I can't figure out why these moved to qi-lib/space.rkt. They're not the only things that use the make-interned-syntax-introducer (which should maybe be bound once as qi-introducer?).

[countvajhula]

The thinking was that binding and referencing identifiers in a certain space isn't specifically about macros, but just about bindings. So I moved those core interfaces, that bind and refer, to space.rkt. Like we could technically have (define-qi-syntax abc 5) and it wouldn't be macro. But everything else in the module is specifically about defining and using syntax transformers so I retained them in macro.rkt.

And good idea! I've bound (make-interned-syntax-introducer 'qi) to introduce-qi-syntax.

[benknoble]

This looks like it's used only for testing. Would it make sense to export it only in a private submodule?

[countvajhula]

That seems reasonable. Not too familiar with proper submodule use, but I tried a few variations of using module+, module* and module and none of them worked. E.g.:

space.rkt:

(module+ refer
  (provide reference-qi)

  (define-syntax-parser reference-qi
    ...))

and then in the tests/space.rkt module:

(require qi/flow/space/refer)

... says "collection not found."

[benknoble]

You probably need module+, but might not. The require is (submod qi/flow/space refer) (or some other variant using relative module paths to refer to the supermodule, if you prefer).

[benknoble]

Commit 268a5fd says these were removed because they are in qi-lib/flow/extended/impl.rkt, but those implementations are different (they omit the boolean casting; I think we prefer without it).

I believe what actually happened is that commit 8e4338f removed their use in favor of folds; later, commit c4244bc turned those forms into macros. I think then commit 22d2cee re-introduced ~all? as a Qi function (so it's now a Qi function instead of a macro, and it does double-duty serving AND). Similar for commit 8156eb0 and ~any?, and commit c0531e7 and ~none?.

[countvajhula]

Truth is stranger than commit messages 🌠

[benknoble]

It would be nice if this was more traceable, though. Spelunking through commits to answer logs is helped by cross-references and detailed commit messages :)

[countvajhula]

True, I'll try to write better commit messages and think more "archeologically" about this :)

[benknoble]

Commit 0fb1afa says this has been in the expander for a while; I think it was effectively introduced (and then several times refactored) by commit 5e1e06e as part of the flow macro (this was Oct. 2021 and is in main!).

This does make me wonder what machinery besides coverage reports we have to make sure that all passes, parsers, etc., are tied together. For example, with the compiler->expander pipeline, I think there are several stages to trace through to follow an input flow syntax to an output syntax: how do we make the sure each "pass" or "stage" handles all (and only) the outputs of its previous stage?

[countvajhula]

I definitely agree the implementation has reached a level of complexity where having many different and complementary ways to test things would be valuable and would save us a lot of time. I'll summarize what we have today and what is planned -- lmk if this addresses your concern adequately or if you feel more is needed:

• we have regular unit tests validating the semantics of the language (e.g. qi-test/tests/flow.rkt). In addition to local tests of individual forms, this includes some nonlocal flows like the counterexamples you found, and in general we'd want these to tell us if we change the meaning of the language (including unsound rules).
• there are starter "expander" tests (qi-test/tests/expander.rkt) that verify that surface expressions are correctly expanded to core expressions (but it can only compare the result as datums for now)
• there are compiler "rules" tests (qi-test/tests/compiler/rules.rkt) that check individual rewrite rules, and also check transformations effected by overall individual compiler passes like normalization (which may repeatedly apply sets of rules to a fixed point). These verify that when given correct input they would produce the expected optimized output.
• compiler "semantics" tests (qi-test/tests/compiler/semantics.rkt) that check various combinations of patterns that are matched by optimization rules, to verify that they produce the same output (including raising errors) as the original expression. These patterns are specific to the compiler implementation and that's why they aren't language level (regular) unit tests.

Planned:
The above tests would validate that different components behave as expected when they are invoked, but we still don't have a way to know whether they are invoked for a particular expression. Some folks on discourse suggested "trace" testing, where we add logging to each pass of the compiler and then verify that a given expression goes through the correct sequence of compiler passes by capturing logs from invoking the compiler. E.g. we would expect (~> (~> f)) to be normalized, (~>> (filter odd?) (map sqr)) to be deforested, and (~>> (~>> (filter odd?) (map sqr)) to be both normalized as well as deforested (FTR every expression today does go through both normalize -> deforest, but for any given expression, either or both of these passes may leave it unchanged).

Wdyt? Anything else that would be useful?

[benknoble]

All sounds reasonable.

One thing I was considering was: how do I check what the syntax (or other input) to each "stage" is? Such as compiler, expander, optimizer, etc. What is the output?

I would call them languages, but HtDP might call them data definitions 😅 basically a "this is what we can expect to see coming in and out, and code should handle all these cases." That would pave way to follow the code through each pass, for example, to trace a specific form through the pipeline.

[countvajhula]

I'll give this some more thought. It would be nice to be able to trace an expression like this on-demand in a testing/REPL context. The macro stepper should be one way to gain visibility of this kind but currently doesn't give us the most support due to some APIs being private and not supported IIUC.

[benknoble]

I'm also suspicious of this part of find-and-map, where we apply a transformation to the syntax list during tree traversal.

That's almost definitely the problem, but I don't see an easy fix.

Does everything still work if the cons match pattern is gone? It seems to basically say "if the input is a list, run find-and-map on the head and the tail." Why/where do we use that power? I assume it's to traverse deep trees or something… but in this case we need to traverse the tree while maintaining some context? Or perhaps it would be better to write [(? list? xs) (map (curry find-and-map f) xs)] so that we run the function on each child node, rather than on the head & tail (since the tail doesn't seem to make much sense?)?

[countvajhula]

Thanks for the detailed review!

Looking at find-and-map again, it actually does look right to me. Even though we're getting the cdr of the syntax list and calling find-and-map recursively on it, the pattern matching ensures that it will only apply the actual transforming function to syntax, which should only ever be the car position (the cdr should keep recursing structurally) (i.e. (f stx) on this line).

What's weird is that this test for normalization of the core language passes (normalization is mostly all datum rather than literal matching), and adding a debug log at the top of normalize-rewrite to print the input syntax shows the correct sequence of syntaxes being passed to it (cf. my earlier comment about this bug). But when we run effectively the same test using the surface language (i.e. the currently failing test), it improperly passes the sublist to normalize-rewrite and breaks.

That could imply that (tee collect) is answering affirmatively to syntax? in find-and-map. I'll see if I can verify that. I'll also try modifying/removing the cons pattern and see what happens.

[countvajhula]

I'll continue looking into (1) private submodule, (2) pattern jumbling, (3) find-and-map, (4) testing ideas. But I'll go ahead and merge this PR now in case @dzoep wants to experiment with things.

Thanks!