Fix scope bug with expanded syntax cacher

[michaelballantyne]

When the eval-handler is called for a use of eval-syntax, avoid adding scopes to the syntax by using expand-syntax rather than expand.

Here's a file that demonstrates the effect:

#lang racket/base

(namespace-require 'racket/base)
(eval '(define x 5))

; Ensure `id` has a source location to trigger racket-mode's expanded syntax cacher.
(define id (datum->syntax #f 'x #'here))

; Works because `eval` adds the namespace outside-edge scope.
(eval id)

; Should be unbound; `eval-syntax` should not add the scope.
; In racket-mode it instead evaluates to 5.
(eval-syntax id)

I don't understand the expanded syntax cacher well enough to know whether the other uses of expand should be changed as well; changing just this occurrence fixed the problem I had.

[greghendershott]

Thanks for the report and the PR!

My concern is the cache would have a mix of (a) syntax that is expanded and "enriched" (expand), as well as (b) expanded but not enriched (expand-syntax). Would some of the cache users need it enriched?

To avoid that, we could both put expanded syntax into the cache (status quo), and do another expand-syntax for the eval handler (as in your PR). But expansion can be expensive (ergo the motivation for a cache); expanding twice seems bad.

Is there a cheaper way to enrich (or "de-enrich", either way could work) already-expanded syntax? (I feel like this is a dumb question and I should know more about scopes, but since you clearly do know more than me, I'm happy to ask you. :smile:)

[greghendershott]

[The motivation for having an eval handler cache the expanded syntax was this scenario: I run a file. Various imported modules are evaluated; if their expansions are cached, then when you visit a definition in one, my code to search within it for the location works faster (using the "pre-loaded" cached expanded syntax). That's a fairly common scenario -- open a file, visit a definition in a file it imports -- and so this use of the cache seemed to help a lot, especially for projects where expansion is expensive. So that's part of the motivation for doing one of the most horrible things in computer science, here. :smile: Some other motivations/uses include visiting definitions inside modules, and drracket/check-syntax.]

[greghendershott]

I think maybe that was a really dumb question: I think the answer is simply, namespace-syntax-introduce. In other words expand is the composition of expand-syntax and namespace-syntax-introduce -- correct?

So would something like the following seem correct to you? The idea is to give the original eval handler the result of expand-syntax as in your PR's fix, but to put in the cache that run through namespace-syntax-introduce (as if it came from expand).

 (define ((make-eval-handler [orig-eval (current-eval)]) e)
   (cond [(and (syntax? e)
               (not (compiled-expression? (syntax-e e)))
               (syntax-source e)
               (path-string? (syntax-source e))
               (complete-path? (syntax-source e))
               (file-exists? (syntax-source e)))
-         (define expanded-stx (expand e))
+         (define expanded-stx (expand-syntax e))
+         (define expanded+enriched-stx (namespace-syntax-introduce expanded-stx))
          (define-values (code-str digest) (file->string+digest (syntax-source e)))
-         (cache-set! (syntax-source e) code-str e expanded-stx digest)
+         (cache-set! (syntax-source e) code-str e expanded+enriched-stx digest)
          (orig-eval expanded-stx)]
         [else (orig-eval e)]))

 (define (->path v)
   (cond [(path? v) v]
         [(path-string? v) (string->path v)]
         [else (error '->path "not path? or path-string?" v)]))

[greghendershott]

From Slack discussion it seems that won't work; namespace-syntax-introduce must be applied before given to expand-syntax.

Another idea:

modified   racket/syntax.rkt
@@ -65,19 +65,28 @@

 (define ((make-eval-handler [orig-eval (current-eval)]) e)
   (cond [(and (syntax? e)
               (not (compiled-expression? (syntax-e e)))
               (syntax-source e)
               (path-string? (syntax-source e))
               (complete-path? (syntax-source e))
               (file-exists? (syntax-source e)))
-         (define expanded-stx (expand e))
+         ;; Give orig-eval syntax that's just expanded; see #640.
+         (define expanded-stx (expand-syntax e))
+         ;; In our cache put expanded syntax that's also "enriched" by
+         ;; expand. Note: Although this should do almost no
+         ;; _expansion_ work per se, it will need to traverse (and
+         ;; enrich) the pieces of a potentially very large syntax
+         ;; object. If that turns out to be too slow, we could in this
+         ;; case put a thunk/promise in the cache, which would do the
+         ;; entiching expand only if/as/when needed.
+         (define expanded+enriched-stx (expand expanded-stx))
          (define-values (code-str digest) (file->string+digest (syntax-source e)))
-         (cache-set! (syntax-source e) code-str e expanded-stx digest)
+         (cache-set! (syntax-source e) code-str e expanded+enriched-stx digest)
          (orig-eval expanded-stx)]
         [else (orig-eval e)]))

[michaelballantyne]

My concern is the cache would have a mix of (a) syntax that is expanded and "enriched" (expand), as well as (b) expanded but not enriched (expand-syntax). Would some of the cache users need it enriched?

I'm not sure I understand the situation where this might become a problem. Is the cache sometimes used other than for complete modules?

In the case of a module, I think the initial scopes are only relevant for resolving the module identifier, given that the expansion of a module removes other module or namespace scopes from its body. When the expander passes a module loaded from a file to the eval handler, it will have created a module identifier with appropriate scopes using (namespace-module-identifier).

[greghendershott]

Giving the result of expand-syntax to drracket/check-syntax doesn't seem to work, for example; it needs expand.

---

One thing I realized yesterday, is that doing this cache-warming work in an eval handler is probably the wrong spot. Instead it should be a compiled load handler.

(I added this some years ago, and sadly can't recall a justification for doing this with an eval handler. The more I think about it, now, the more wrong it seems.)

So I've been working on making that change, which also indirectly fixes the issue you saw, by not contaminating your user program eval.

[michaelballantyne]

Thanks Greg!

On Tue, Sep 13, 2022 at 10:02 AM Greg Hendershott @.***> wrote:

Closed #640 https://github.com/greghendershott/racket-mode/pull/640 via caa9259 https://github.com/greghendershott/racket-mode/commit/caa9259ad08f3aa7b9fb4441933ca3ac7254fad2 .

— Reply to this email directly, view it on GitHub https://github.com/greghendershott/racket-mode/pull/640#event-7377001246, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAK46UYI3L2JLM6XISJHLVTV6CCN7ANCNFSM6AAAAAAQFDMYXI . You are receiving this because you authored the thread.Message ID: @.***>