racket / htdp

Other
93 stars 70 forks source link

Enable `use-function-output-syntax` for ISL[+]. Fixes #208. #229

Closed shhyou closed 2 weeks ago

shhyou commented 1 month ago

Fixes #208.

The printer for ISL, ISL+ and ASL relies on use-named/undefined-handler and named/undefined-handler to correctly print named lambdas instead of outputting (lambda (a1 a2 ...) ...). In particular, use-named/undefined-handler checks whether the option use-function-output-syntax is set.

If use-function-output-syntax is not enabled, then during module instantiation, user-written functions like my-add1 would be printed differently. This also affects the error message from the check-expects.

#lang htdp/isl+
(define (my-add1 n) (+ n 1))
my-add1
(check-expect my-add1 2)

Before:

Welcome to DrRacket.
(lambda (a1) ...)                                        ;; <- no good

Ran 1 test.
0 tests passed.

check-expect ... error ... :: first argument of equality
cannot be a function, given (lambda (a1) ...)            ;; <- no good

> my-add1
my-add1

This PR parameterize the current-namespace to the current module's namespace during the printing process.

rfindler commented 1 month ago

Another approach would be to simply (or conditionally) remove this test. It isn't clear to me what value there is in checking the current namespace as the program executes. (Indeed, I wonder if built-executables will produce different results than running from inside DrRacket, due to this check.)

Maybe @mfelleisen knows the rationale for this check and whether or not we can just simply skip it for the #lang-based teaching languages?

mfelleisen commented 1 month ago

I don’t think I ever touched this file. (I recall looking at it, I think.)

rfindler commented 1 month ago

I don’t think I ever touched this file. (I recall looking at it, I think.)

I @mfelleisen 'd because of the semantics not because of the implementation.

To ask the question more better, I'm wondering under what conditions do you think we should see the name of a function and under what conditions should we see something (lambda (a1) ...)?

Right now the criteria is a bit wonky, using the current namespace. That strikes me as unlikely to be the best criteria (although it may have been reasonable in the 1990s). In particular, I wonder if lambdas that are bound locally should get the name or not? Or only top-level definitions? Or is there some other way to decide which ones get printed with names and which don't?

mfelleisen commented 1 month ago

The semantics matches the pedagogy. So:

From the perspective of a BSL programmer, always use the name of a function. Period.

For an ISL+ programmer, it should be (lambda …).

For an ISL programmer, it’s an open question:

`(local ((define (f x) …)) …)`

is lifted to the top level with a minor renaming of f because it could be lifted more than once. Indeed, when kids make the usual mistake an call the outer function from the inner one and something goes wrong, they might understand that f13 means “f was lifted 13 times” — but this depends a lot on the capabilities of the instructor and most of them aren’t capable. Period. Meaning we should sadly go with just the plain name f.

Makes sense?

shhyou commented 1 month ago

In BSL/BSL+, I think printing functions are mostly impossible, so only ISL/ISL+ are within the scope.

Currently ISL prints things as function:XYZ (https://github.com/racket/htdp/commit/8a6d34936606).

In ISL+, should functions like add, posn-x, +, reverse, etc., be printed as (lambda ...)?

mfelleisen commented 1 month ago

Semantically yes.

Pragmatically — context! means students comprehending the error message and acting on it — no, all functions with a name (not anonymous lambdas) should print-by-name not print-by-value.

shhyou commented 1 month ago

@rfindler wrote: Right now the criteria is a bit wonky, using the current namespace. That strikes me as unlikely to be the best criteria (although it may have been reasonable in the 1990s). In particular, I wonder if lambdas that are bound locally should get the name or not? Or only top-level definitions? Or is there some other way to decide which ones get printed with names and which don't?

Testing if an object has a name does not differentiate bound lambdas from anonymous ones since all lambdas have names like "PATH/FILE.rkt:LL:CC". Maybe the test has to be something like matching against #px"[.]rkt:\\d+:\\d+".

shhyou commented 1 month ago

Some tests are added to module-lang-test.rkt @ https://github.com/racket/drracket/pull/689/commits/046311b3bcf75830c9a95d17a90845bfe0c517f0 in https://github.com/racket/drracket/pull/689.

shhyou commented 4 weeks ago

@rfindler wrote: Another approach would be to simply (or conditionally) remove this test.

So: rather than setting the namespace, directly matching lambda's name against the pattern #px"[.]rkt:\\d+:\\d+" in pconvert-lib expectedly changes the output of (let ([f (lambda (x) x)]) f) from (lambda (a1) ...) to f.

@mfelleisen @rfindler Is this an acceptable fix?

raco test: (submod (file "language-test.rkt") test)
raco test: @(test-responsible '(robby matthias))
......
>> starting intermediate
>> finished intermediate
>> starting intermediate/lambda
FAILED: definitions (#rx"Intermediate Student with lambda(;|$)") expected "(let ([f (lambda (x) x)]) f)" to produce:
  "(lambda (a1) ...)"
got:
  "f"
instead
FAILED: interactions (#rx"Intermediate Student with lambda(;|$)") expected "(let ([f (lambda (x) x)]) f)" to produce:
  "(lambda (a1) ...)"
got:
  "f"
instead
>> finished intermediate/lambda
>> starting advanced
FAILED: definitions (#rx"Advanced Student(;|$)") expected "(let ([f (lambda (x) x)]) f)" to produce:
  "(lambda (a1) ...)"
got:
  "f"
instead
FAILED: interactions (#rx"Advanced Student(;|$)") expected "(let ([f (lambda (x) x)]) f)" to produce:
  "(lambda (a1) ...)"
got:
  "f"
instead
>> finished advanced
mfelleisen commented 4 weeks ago

Yeah, I think we can go with this.

rfindler commented 4 weeks ago

I'm a little bit suspicious of using a regular expression on the name. There might be a better way: can you clarify a little bit why just using object-name isn't okay?

shhyou commented 4 weeks ago

I'm a little bit suspicious of using a regular expression on the name. There might be a better way: can you clarify a little bit why just using object-name isn't okay?

Because all lambda functions in a (saved) file have names.

rfindler commented 3 weeks ago

I'm a little bit suspicious of using a regular expression on the name. There might be a better way: can you clarify a little bit why just using object-name isn't okay?

Because all lambda functions in a (saved) file have names.

Ah, right!

Thanks to a tip from @mflatt it turns out that this doesn't have to be true. Here's how to make a function without a name:

#lang racket
(require (for-syntax syntax/parse))
(define-syntax (no-srcloc-name-lambda stx)
  (syntax-parse stx
    [(_ . stuff)
     (if (syntax-local-name)
         #'(λ . stuff)
         (syntax-property #'(λ . stuff)
                          'inferred-name
                          (void)))]))

> (object-name (no-srcloc-name-lambda (x) x))
#f
> (object-name (let ([f (no-srcloc-name-lambda (x) x)]) f))
'f

So if we can adjust the expansion of the teaching language lambda to be like this one then we can add a parameter to print convert that lets us avoid the namespace check entirely.

mfelleisen commented 3 weeks ago

That's not bad but say we have a program like this (very contrived):

#lang htdp/isl

(define (many n)
  (local ((define (f x) n))
    f))

(check-satisfied 1 (third (build-list 10 many)))

We get a strange error.

(I also recommend stepping through this. The stepper shows way too much of the expanded code in check-satisfied. But see the names it gives to f.)

rfindler commented 3 weeks ago

Earlier, what you wrote, @mfelleisen, implies that the result of many, for any input, should print as f. I think that's what my suggestion would do. Am I getting something wrong about that?

mfelleisen commented 3 weeks ago

What I am getting at is this:

  1. The stepper correctly enumerates the functiion (by coincidence they are called f1, f2, .. so f combined with the index).
  2. Is f enough of a hint that f3 went wrong? -- My guess is "yes in most cases."

Thinking aloud here, so let's try f.

@jbclements See stepper for the above program. This should be fixed.

shhyou commented 3 weeks ago

Updated with a new solution.

As it seems, enable use-function-output-syntax plus some small changes fix the issue. No need to change pconvert-lib or touch the namespace.

The tests are available from https://github.com/racket/drracket/pull/689/commits/c60fc6a3744254638d7ff75252045ec552cbc6cc in:

shhyou commented 3 weeks ago

Oops, it is still not fixed for unnamed lambdas in files. The tests do not save buffers to files so they did not capture the bug.

rfindler commented 3 weeks ago

Nice find! Even looking at the diff, I'm remembering nothing about that!

Does this also solve the problem with lambdas that get names from the source location? Oops, I missed your followup message. Still, it seems like this is a good step in the right direction.

Along those lines, it seems to me that a source-location-based name might actually be useful in some situations to help debugging.

shhyou commented 3 weeks ago

@mikesperber this PR is ready. Do you see any obvious issues with turning on the use-function-output-syntax option, or with the regexps I used in the use-named/undefined-handler guard?

@rfindler The code has to be saved to disk when testing. From the documentation, 'inferred-name (void) removes inferred name. As it seems, the source-location name is used afterwards:

#lang racket
(define-syntax (no-srcloc-name-lambda stx)
  (syntax-case stx ()
    [(_ . stuff)
     (syntax-property #'(λ . stuff)
                      'inferred-name
                      (void))]))

(object-name (let ([f (no-srcloc-name-lambda (x) x)]) f))

#|
Welcome to DrRacket, version 8.15.0.3 [cs].
Language: racket, with debugging; memory limit: 256 MB.
'...o-inferred-name.rkt:5:24
> 
|#
rfindler commented 3 weeks ago

@shhyou hm; thanks for noticing that! I see that we could use an applicable struct and prop:object-name to avoid the regexps. I note that the regexps won't work if the file's name doesn't have a rkt in it. I'm sure that's not common, but I have seen students save files without any extension at all.

rfindler commented 3 weeks ago

It looks like we need to strip the source location from the syntax object too, eg:

#lang racket

(begin-for-syntax
  (define (remove-srcloc stx)
    (datum->syntax stx (syntax->datum stx) #f stx)))

(define-syntax (no-srcloc-name-lambda stx)
  (syntax-case stx ()
    [(_ . stuff)
     (syntax-property (remove-srcloc #'(λ . stuff))
                      'inferred-name
                      (void))]))

(object-name (let ([f (no-srcloc-name-lambda (x) x)]) f))

(we would also need to do this conditionally, I think, so that there is an enclosing let the function gets the name)

shhyou commented 3 weeks ago

Done! Removing source locations solves the problem.