stepper fails for unbound id in test position of check-expect

[jbclements]

To reproduce:

1. Set language level to Beginner
2. put this program in defns window:

(check-expect (f (list 4))
(list 4 13))
(check-expect (f (list 4))
(list 7 13))

Click Step.

See this error:

recon-source: no matching clause for syntax: (quote-syntax f)

Looks like in this case the reconstructed source contains a quote-syntax.

This bug has been present since at least Racket 7.4.

[shhyou]

The test engine recently fixed the source location of syntax errors in check-expects (#152). Is it related or is the bug exposed by that?

[jbclements]

perhaps indirectly. I think the issue here is that the reconstructor is just blindsided by the appearance of a quote-syntax, which suggests that it has to do with the expansion of the test forms. Could be related, though. Programming is complicated....

[mikesperber]

perhaps indirectly. I think the issue here is that the reconstructor is just blindsided by the appearance of a quote-syntax, which suggests that it has to do with the expansion of the test forms.

The expansion of tests forms certainly did change, but there's no (obvious) quote-syntax sitting there. (And the macros stepper refuses to work in the presence of undefined variables, AFAICS.)

The relevant macro is here, if that helps:

htdp/htdp-lib/test-engine/syntax.rkt

Line 29 in fc6c2c2

(define-for-syntax (check-expect-maker stx checker-proc-stx test-expr embedded-stxes hint-tag)

[mikesperber]

So it's this line:

htdp/htdp-lib/test-engine/syntax.rkt

Line 40 in 29e1c78

(define test-expr-checked-for-syntax-error #`(convert-compile-time-error #,test-expr))

... which was recently discussed elsewhere.

Changing it to:

  (define test-expr-checked-for-syntax-error test-expr)

fixes the problem, makes the stepper refuse to start, with an appropriate error message.

[rfindler]

Does removing this line generally make the stepper work in a way we like with #lang-based check-expect or is there more to sort out here?

[mikesperber]

This problem is for both menu-based and #lang- so the bug fix is for both. AFAIK, the stepper has generally been fine with #lang.

[jbclements]

I meant to take a closer look at this ... but I haven't had time. @mikesperber , isn't this change simply removing a call to convert-compile-time-error ? I have no idea what this call was supposed to do, but is it safe to just remove it? I think you know way more about the test-engine here, so I guess I'm just asking you to explain the code to me, sorry...

Second question: AFAIK the #lang-based stepper directs the output of check-expect failures to DrRacket's stdout, which appears on the user's terminal... if they started DrRacket using a terminal, which for students is almost certainly not the case. In other words, if check-expect failures are now displayed correctly for #lang-based languages, this would be a big step forward! Not that I expect that, I would be very surprised, I think I'm just trying to clarify what robby said.

[mikesperber]

Second question: AFAIK the #lang-based stepper directs the output of check-expect failures to DrRacket's stdout, which appears on the user's terminal...

@jbclements Whoopsie, that's a bug - see #224.

[mikesperber]

@mikesperber , isn't this change simply removing a call to convert-compile-time-error ? I have no idea what this call was supposed to do, but is it safe to just remove it? I think you know way more about the test-engine here, so I guess I'm just asking you to explain the code to me, sorry...

@jbclements Yes, that's what the change is. Nobody remembers why the convert-compile-time-error was there in the first place, and we considered removing it for other reasons recently. So we'll run with it for now and see if it causes any problems.

[jbclements]

It looks to me like this use of convert-compile-time-error was inserted in 7143cd5 by @mfelleisen , cleaning up an existing use of local-expand that the comments suggest was intended to deal with arity errors. Could removing this cause arity errors in the test expression to be handled badly?

[jbclements]

Digging a bit further, it looks like the earlier version of the code was inserted in b050287, again by Matthias, in a commit titled "errors in tested expressions no longer abort unit testing (lib)" So.. perhaps the idea was to delay a compile-time error that halted testing to a run-time one? This still doesn't quite make sense to me.

[samth]

Right now, this program runs two tests and reports a failure in the first one in BSL:

(check-expect (1 + 2) 3)
(check-expect 3 (+ 1 2))

Unfortunately this is still a static error that prevents tests from running:

(check-expect 3 (1 + 2))

I think we should make more tests run in the face of syntax errors.

[jbclements]

Ah! Okay, so this is the age-old "can we provide more useful information if we don't just halt immediately on the first compilation error" goal. I ... don't really have a dog in this fight, and I think the stepper can be made to work either way, with a certain amount of work (specifically, it looks like judicious uses of "stepper-skip-completely" are the solution if we decide to stick with or extend the current direction; that is, assuming the wrapped pieces of code aren't actually going to be executed at all.

[mflatt]

Commit d701105 broke compilation of the "htdp-test" package. (There irony here is too much.)

[mfelleisen]

Sorry I busy until now .. and I pointed out that we needed to re-run the tests in “htdp-tests” before we push this through — in an off-line email thread that you can’t see. (Now that @jbclements <https://github.com/jbclements> is pointing out this commit, it starts ringing a bell or even two.)

[mflatt]

Should the commit be reverted for now? I think the Northwestern snapshot is going to fail, since it creates some distributions with "main-distribution-test".

[mfelleisen]

Let’s figure out what’s really going on. If Robby or Mike know, let’s stick with it. If not, let’s revert and let’s study the failing test case.

…

On Sep 10, 2024, at 1:06 AM, Matthew Flatt ***@***.***> wrote: Should the commit be reverted for now? I think the Northwestern snapshot is going to fail, since it creates some distributions with "main-distribution-test". — Reply to this email directly, view it on GitHub <#153 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAL7T65ZIPDWMQTA2HV6CMLZVZ43XAVCNFSM6AAAAABNUVIUK2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMZZGYZDMOBXGI>. You are receiving this because you were mentioned.

[mfelleisen]

Following up to my own message. From the parallel off-line mail thread with just Mike:

On Sep 10, 2024, at 3:23 AM, Michael Sperber ***@***.***> wrote: On Mon, Sep 09 2024, ***@***.*** wrote: > Did none of tests in test-engine/ fail? Ah sorry, I got confused by all those little branches and PRs I made over the weekend, and must have run the tests without the commit in it: Some do fail (for reasons I don't understand) - I've pushed a fix. (The problem is not the call to convert-compile-time-error, but how the code is spliced together.) -- Regards, Mike

Looks like we’re okay to go with this commit.

[mfelleisen]

@mikesperber I do not understand " (The problem is not the call to convert-compile-time-error, but how the
code is spliced together.)".

@mflatt (and everyone else I guess) -- I took a look at the code and the failing test case that came with Mike's change. I am beginning to recall why I shifted the mistake to compile time. The rationale is coming back to me. Someone sent in an email to complain that in a file such as this:

(define (f x) x)
(check-expect (f 1 2) 1)
(check-expect (f) 2)

only the first error is signaled to students and, after fixing it, they get a second one. Since BSL defines functions as syntax, this form of arity checking could be done at compile time so I inserted the shift (and then found someone had written a robust version and re-used this one).

(The test file mentions check-satisfied and that makes me think DVH requested such a change because he was teaching our F I at Maryland at the time, and I made several changes in response.)

The sad thing is that I didn't comment the added tests properly and/or leave a better rationale in the commit message to say why we're doing this.

We have two options:

1. roll back Mike's commit(s)
2. modify the test to focus on just the plural/singular issue of the error messages (which was a secondary concern).

What speaks for 1: The design of BSL makes it much more static than Racket. So why not check for such properties early.
What speaks for 2: If we are still thinking of BSL as a step toward Racket where you get one error at a time, we should stick with Mike's change and adjust the test.

My own sense is that 1 is something good for @rfindler 's soul and probably good for students' spirit of overcoming early hurdles in a course like F I. At the same time, uniformity and fixing the stepper speak for 2.

Opinions?

(And again sorry for the delay. Sw Dev kept me busy.)

[samth]

I'm not sure which behavior your 1 and 2 correspond to, but I am strongly in favor of keeping those errors as dynamic errors so that other tests also can run.

[mfelleisen]

On Sep 10, 2024, at 11:22 AM, Sam Tobin-Hochstadt ***@***.***> wrote "but I am strongly in favor of keeping those errors as dynamic errors so that other tests also can run.” Sam, if they become dynamic errors, they shut down a run. In the program I included, the second error does not show up at run-time until the first error is fixed. If the error discovery goes back to static, students see both errors up front.

[samth]

In my post #153 (comment) I gave two examples of behavior. Are you proposing to change the current (ie 8.14) behavior of either of those two examples? In which way?

[mfelleisen]

1. I am investigating a problem we encountered due to what Mike and Robby agreed to in Milan. 2. I am not proposing a change. I am requesting opinions of whether — we should keep this change and modify the one currently failing test — we should roll back and re-establish that all tests pass 3. My note lays out the rationale for both alternatives. My personal inclination is to keep Mike’s change. 4. Neither of your test cases in comment #153 is affected by Mike and Robby’s work.

[mflatt]

I'll revert d701105 for now. It broke the Utah snapshots, too, as well as daily the build plots, and it's showing up as an error for anyone who builds Racket from a Git checkout.

[shhyou]

@mikesperber In our class, it is important that the expression position of check-expect do NOT raise unbound-identifier error at expansion time. We commonly provide exemplar check-expect in our starter code for the purpose of explaining the problem specification like this:

#lang htdp/isl+

;; add2 : number -> number
;; Adds 2 to the input.
;;
;; Example:
(check-expect (add2 10) 12)

I hope such code would not err, for otherwise background expansion would not work.

[mikesperber]

Re-opening, as the patch got reverted.