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

Use-site binder hygiene bug for let #474

Open
michaelballantyne opened this issue Feb 7, 2024 · 7 comments
Open

Use-site binder hygiene bug for let #474

michaelballantyne opened this issue Feb 7, 2024 · 7 comments

Comments

@michaelballantyne
Copy link
Contributor

I've previously discussed this with @mflatt , but I figure I should make a ticket.

Consider this program:

#lang rhombus/and_meta

block:
  let x = "outer"
  defn.macro 'm $id':
    'let $id = "inner"
     x'
  m x

It returns "inner", but I think it should return "outer" like the similar Racket program does:

(define x "outer")
(define-syntax-rule (m id)
 (let ([id "inner"])
   x))
(m x)

I want to point this out now because I saw racket/racket#4929, which proposes to add something new to the syntax-local- zoo to support the implementation of Rhombus let. I'm a little skeptical of the current implementation strategy for let given that it gets this use-site binder hygiene wrong, and it feels like it'd be unfortunate to expose more low level syntax-local- operations that could point users towards implementations of scoping structures that don't get hygiene right.

In contrast, an implementation of let using definition contexts and local-expand should be able to avoid this problem because one of the things definition contexts encapsulate is a set of local use-site scopes. I don't know enough about Rhombus to know what other implementation constraints there are, though.

@usaoc
Copy link
Collaborator

usaoc commented Feb 8, 2024

Given the current way Rhombus blocks are expanded, I think the alternative strategy you suggest will require much more significant change, because the expansion uses a trampolining macro, which can’t thread through an opaque internal-definition context value. I’ll do some investigation to see what is exactly happening, but I don’t think we can change the strategy very easily.

Another thing that’s curious about let is that a def after let works like “splicing” in the sense that they should be visible to forms before the let. There isn’t much precedent to this, except the historical define* form, which works like Rhombus let:

#lang racket/base
(require compatibility/package)

(define-package test ()
  (define* x "outer")
  (define-syntax-rule (m id)
    (begin
      (define* id "inner")
      (println x)))
  (m x))
;; prints "inner"

@usaoc
Copy link
Collaborator

usaoc commented Feb 8, 2024

After some more thinking, I’m not sure the Racket example really corresponds to the Rhombus counterpart either. Racket let doesn’t work like Rhombus let in that the former creates a new definition context, while the latter doesn’t. Because we have the same definition context, the use-site scope removal behavior applies and results in what you’re seeing—this behavior is arguably intended. I think the confusing part here is that I used a definition context purely for creating scopes that are intended to be pruned, but we don’t actually want a new definition context (the PR is also specifically for addressing this, not intended as a way for the user to replicate what definition contexts already do).

Edit: By the way, the situation is quite like that in Section 2.5 of “Binding as Sets of Scopes”.

@usaoc
Copy link
Collaborator

usaoc commented Feb 8, 2024

One thing that is broken:

#lang rhombus/and_meta

block:
  let x = "outer"
  expr.macro 'm $(id :: Term)':
    'block:
       let $id = "inner"
       x'
  m x

I think we can all agree that this should be "outer", not "inner".

Edit: And, I haven’t thought enough about this, but could #341 be a similar issue?
Edit 2: This is because Enforestation does not add any use-site scopes whatsoever, uh.

@mfelleisen
Copy link

(A general point. I don't think it is correct to ask 'how much change does it take' but 'do we need to widen the API' (with a new syntax-local-zoo-thingie) or 'what is a good way to solve this problem' (not 'best'!). Too often language design -- some of ours included -- piles on little tweaks here and more little tweaks there and the resulting system looks in the end like a mess.)

@michaelballantyne
Copy link
Contributor Author

After some more thinking, I’m not sure the Racket example really corresponds to the Rhombus counterpart either. Racket let doesn’t work like Rhombus let in that the former creates a new definition context, while the latter doesn’t.

To me, this depends on how we think about modeling scoping structure from a user's perspective. Things like use-site scopes and definition contexts are our internal implementation tools, but should users have to think in terms of them? To me, Rhombus let creates a new scope that is the child of the surrounding scope, just like Racket's let. It so happens that Rhombus implicitly puts all the following forms (except those that splice) in that scope, but I still understand it as a nested scope.

The racket/splicing forms are another analogue for the let + def / define* behavior.

@michaelballantyne
Copy link
Contributor Author

This is because Enforestation does not add any use-site scopes whatsoever, uh.

Would it be possible for Rhombus to leverage syntax-local-apply-transformer for its macro expansion step, rather than re-implementing hygiene via the pieces in https://github.com/racket/rhombus-prototype/blob/649e30b7420019c4a3f8bc9626035b0ea13fa35c/enforest/private/transform.rkt?

@mflatt
Copy link
Member

mflatt commented Feb 12, 2024

I'm not so worried about syntax-local-make-definition-context-introducer growing the zoo, because it's a kind of shortcut for functionality that's already available.

For use site scopes, than @usaoc for the reminder (and for tracking down) that it's not in place, yet. I'll work on filling that in. Thanks @michaelballantyne for the reminder that syntax-local-apply-transformer exists and maybe should be used for Rhombus-level expansion.

I don't agree that Rhombus let creates a scope that is a child of the surrounding scope. Of course, the way I use the word "scope", there's no such notion as "child". Whether something ("scope", "scope set", or other) needs to fit in a hierarchy like that is our main difference of opinion, I guess.

Since I think about let differently, I think the closer analog to the Rhombus program at the start here is

#lang racket

(define-syntax-rule (m id)
 (begin
   (define id "inner")
   x))
(m x)

where the result is "inner". Of course, I removed the definition of x to outer, because there's not a define*-like form in #lang racket.

usaoc added a commit to usaoc/rhombus that referenced this issue Feb 16, 2024
mflatt pushed a commit that referenced this issue Feb 16, 2024
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

No branches or pull requests

4 participants