syncheck:add-definition-target needs to supply phase level?

[greghendershott]

In this program, I think we want jump-to-definition to work for the phase 0 x and the phase 1 x -- work for both, treating them as the independent things that they are.

#lang racket/base
(require (for-syntax racket/base))

(module m racket/base
  (require (for-syntax racket/base))

  (define x 0)
  (provide x)

  (begin-for-syntax
    (define x 1)
    (provide x)))

(require 'm)
(printf "0 = ~a\n" x)

(begin-for-syntax
  (printf "1 = ~a\n" x))

I don't see how this can work with the status quo add-definition-target because it does not supply the phase level. As a result, you can't store the two definition targets distinctly in a hash-table or database table which is keyed by phase level as well as file, submods, and symbol.

Similarly status quo add-jump-to-definition doesn't supply the phase level.

To validate this, I hacked a temporary alternative to add-definition-target that is phase-aware, and made my table of definition targets keyed by file, submods, symbol, and phase. That way I am able to implement the desired behavior.

---

Admittedly, I feel like my example -- providing two identifiers with the same symbol but different phases -- is an unlikely, nit picky, corner case. How often do people do that? Would it be so bad to shrug and say, "well stop doing that"?

Is it really worth handling that -- including needing to roll out two new syncheck methods to supersede add-definition-target and add-jump-to-definition (much like add-arrow got superseded)?

I'm not sure but I wanted to ask.

[rfindler]

Hi @greghendershott -- sorry for the long delay here. Does the commit I just pushed match what you had in mind? I'm happy to revise if you had something else in mind (and we're past the release branch creation so we have time to do something different as this won't be in 8.7).

[greghendershott]

I had looked at this in the context of a project to implement a sqlite "program information database" (effectively a check-syntax on-disk cache). Alas I haven't worked on that for awhile, it's a bit "stale", and so is my memory of the details.

One thing I notice now, in my code from back then: I had named the new, extra method parameter something like phase+space, not just phase-level. Around that time, Matthew had added "spaces" (e.g. for-space). I think that's just a naming difference, though. You're already handling it as a potential phase+space value, from what I can tell. So TL;DR LGTM.

[rfindler]

Thank you, Greg! Indeed, that information was missing (and I've added it in now).

[greghendershott]

AFAICT your commit closes this issue, so thanks! And I'll close it.