Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

shhyou
Copy link
Collaborator

@shhyou shhyou commented Oct 25, 2024

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
Copy link
Member

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
Copy link
Contributor

mfelleisen commented Oct 25, 2024 via email

@rfindler
Copy link
Member

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
Copy link
Contributor

mfelleisen commented Oct 26, 2024 via email

@shhyou
Copy link
Collaborator Author

shhyou commented Oct 26, 2024

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 (8a6d34936606).

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

@mfelleisen
Copy link
Contributor

mfelleisen commented Oct 26, 2024 via email

@shhyou
Copy link
Collaborator Author

shhyou commented Oct 26, 2024

@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
Copy link
Collaborator Author

shhyou commented Oct 26, 2024

Some tests are added to module-lang-test.rkt @ racket/drracket@046311b in racket/drracket#689.

@shhyou
Copy link
Collaborator Author

shhyou commented Oct 29, 2024

@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
Copy link
Contributor

mfelleisen commented Oct 29, 2024 via email

@rfindler
Copy link
Member

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
Copy link
Collaborator Author

shhyou commented Oct 30, 2024

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
Copy link
Member

rfindler commented Oct 30, 2024

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
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor

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 added a commit to shhyou/drracket that referenced this pull request Nov 1, 2024
@shhyou shhyou changed the title Set current namespace to the current module. Fixes #208. Enable use-function-output-syntax for ISL[+]. Fixes #208. Nov 1, 2024
shhyou added a commit to shhyou/drracket that referenced this pull request Nov 1, 2024
@shhyou shhyou force-pushed the configure-namespace branch 2 times, most recently from 2f91396 to f89af9c Compare November 1, 2024 21:29
@shhyou
Copy link
Collaborator Author

shhyou commented Nov 1, 2024

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 racket/drracket@c60fc6a in:

@shhyou shhyou marked this pull request as draft November 2, 2024 01:07
@shhyou
Copy link
Collaborator Author

shhyou commented Nov 2, 2024

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
Copy link
Member

rfindler commented Nov 2, 2024

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 added a commit to shhyou/drracket that referenced this pull request Nov 2, 2024
shhyou added a commit to shhyou/drracket that referenced this pull request Nov 2, 2024
shhyou added a commit to shhyou/drracket that referenced this pull request Nov 2, 2024
shhyou added a commit to shhyou/drracket that referenced this pull request Nov 2, 2024
@shhyou shhyou marked this pull request as ready for review November 2, 2024 23:25
shhyou added a commit to shhyou/drracket that referenced this pull request Nov 3, 2024
@shhyou
Copy link
Collaborator Author

shhyou commented Nov 3, 2024

@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
Copy link
Member

rfindler commented Nov 3, 2024

@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
Copy link
Member

rfindler commented Nov 3, 2024

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)

…racket#208.

The printer for ISL, ISL+ and ASL relies on use-named/undefined-handler
and named/undefined-handler to correctly print named lambdas.
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)

Output:

    Welcome to DrRacket.
    (lambda (a1) ...)

    Ran 1 test.
    0 tests passed.

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

    > my-add1
    my-add1
@shhyou
Copy link
Collaborator Author

shhyou commented Nov 4, 2024

Done! Removing source locations solves the problem.

(htdp-err/rt-test (map (lambda (x y) (+ x y)) (list 2 3 4))
(exn-type-and-msg
exn:fail:contract?
#rx"intm-lam.rktl"))
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test has been moved to racket/drracket@4eaf30c.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The printing style of functions is inconsistent
3 participants