Fix representation of debugging information

[sorawee]

Make the representation of debugging information coincide with that from errortrace, so that #lang errortrace works properly in DrRacket. This PR essentially reverts f4c495b27e5db5e9.

Fixes #564

[rfindler]

My knee-jerk reaction to fixing #564 is to change things so that DrRacket's introduced errortrace annotations don't interfere with the ones that the program is putting into itself. Is there a reason to avoid that approach? Put another way, I don't want someone to be able to disable the debugging information that DrRacket is inserting so taking a path that makes it (as) invisible (as possible) seems better to me, at least a priori.

[sorawee]

Let's analyze what's going on with:

#lang errortrace racket

(error 'foo)

With debugging on

1) DrRacket calls errortrace-annotate with

(module Untitled errortrace/lang/body
  (#%module-begin
   racket
   (error 'foo)))

2) The program is expanded (by DrRacket's request). But this in turn triggers (a) another expansion by #lang errortrace and (b) an annotation by #lang errortrace

(module Untitled errortrace/lang/body
  (#%plain-module-begin
   (#%require (only racket))
   (#%require
    (only errortrace))
   (#%require
    (for-meta
     0
     errortrace/errortrace-key))
   (module configure-runtime '#%kernel
     (#%plain-module-begin
      (#%require
       (for-meta
        0
        errortrace/errortrace-key))
      (#%require
       racket/runtime-config)
      (#%app configure '#f)))
   (#%app
    call-with-values
    (lambda ()
      (with-continuation-mark
       ek:errortrace-key
       '((error 'foo)
         #<path:/Users/soraweep/Untitled.rkt>
         3
         0
         26
         12)
       (#%app error 'foo)))
    print-values)))

3) The program is now annotated as requested by DrRacket:

(module Untitled errortrace/lang/body
  (#%plain-module-begin
   (#%require (only racket))
   (#%require
    (only errortrace))
   (#%require
    (for-meta
     0
     errortrace/errortrace-key))
   (module configure-runtime '#%kernel
     (#%plain-module-begin
      (#%require
       (for-meta
        0
        errortrace/errortrace-key))
      (#%require
       (for-meta
        0
        errortrace/errortrace-key))
      (#%require
       racket/runtime-config)
      (#%app configure '#f)))
   (#%app
    call-with-values
    (lambda ()
      (with-continuation-mark
       ek:errortrace-key
       '((error 'foo)
         #<path:/Users/soraweep/Untitled.rkt>
         3
         0
         26
         12)
       (with-continuation-mark
        errortrace-key
        '#(#<path:/Users/soraweep/Untitled.rkt>
           3
           0
           26
           12)
        (#%app error 'foo))))
    print-values)))

Notice how there are debugging information from both instrumentations:

(#%app
    call-with-values
    (lambda ()
      (with-continuation-mark
       ek:errortrace-key
       '((error 'foo)
         #<path:/Users/soraweep/Untitled.rkt>
         3
         0
         26
         12)
       (with-continuation-mark
        errortrace-key
        '#(#<path:/Users/soraweep/Untitled.rkt>
           3
           0
           26
           12)
        (#%app error 'foo))))
    print-values)

However, they both share the same errortrace-key, and since they are in the same frame, the ones from DrRacket override the ones from #lang errortrace.

The issue is in printing out the error message. The added (#%require (only errortrace)) as a part of the expansion installs a new error display handler, which cooperates with the regular #lang errortrace protocol (which expects a list shape), but not DrRacket's protocol (which expects a vector shape).

With debugging off

(module n racket (error 'foo))

is expanded to (by #lang errortrace)

(module n racket
  (#%module-begin
   (module configure-runtime '#%kernel
     (#%module-begin
      (#%require
       racket/runtime-config)
      (#%app configure '#f)))
   (#%app
    call-with-values
    (lambda ()
      (#%app error 'foo))
    print-values)))

and is further annotated to (by #lang errortrace):

(module n racket
  (#%plain-module-begin
   (module configure-runtime '#%kernel
     (#%plain-module-begin
      (#%require
       (for-meta
        0
        errortrace/errortrace-key))
      (#%require
       racket/runtime-config)
      (#%app configure '#f)))
   (#%app
    call-with-values
    (lambda ()
      (with-continuation-mark
       ek:errortrace-key
       '((error 'foo)
         #<path:/Users/soraweep/Untitled.rkt>
         3
         0
         26
         12)
       (#%app error 'foo)))
    print-values)))

But then DrRacket for some reasons decides to use errortrace-stack-item->srcloc on the exception, even though the debugging is off. In other #lang, it would find nothing, so there's no problem. But for #lang errortrace, it would find list shapes, but errortrace-stack-item->srcloc expects the vector shape.

Thoughts

The issue when debugging is off is very easy to fix (and arguably should be fixed regardless of the status of the current PR). I'll submit another PR.

The issue when debugging is on is much more difficult to deal with. A couple of possibilities:

• This PR: make it so that they both share the same protocol
  • Pros: seems like the right way to do it. One way to think about it is that errortrace is fine as it is. There's a protocol created by errortrace, and DrRacket should respect it.
  • Cons: errortrace...: in the error message is kinda useless since it would use the information from DrRacket, which has no syntax information (it does contain line number though).
• Change errortrace's error display handler to further check that the debugging information is a list shape. If it's not, discard it.
  • Pros: if we think that errortrace...: in the error message is redundant, then this solution solves the problem by not presenting the information at all. However, doing so would not respect the use of #lang errortrace, so arguably it is wrong.
  • Cons: it's hacky. Fixing things should not require changing a component that does its job already well.
• Make DrRacket and #lang errortrace use different errortrace-key
  • Pros: The errortrace...: would have the proper syntax information in it.
  • Cons: more difficult to implement. It would require changing the protocols in both errortrace and DrRacket, potentially creating backward incompatibility, and there are probably subtle issues that I have not foreseen.

[rfindler]

I think that the third bullet you mention above ("Make DrRacket and") is the right approach. Errortrace is intended to be parameterized over which information constitutes the source locations (reusing the code that inserts and tracks the source locations) and DrRacket is already using that part of the API. I think the problem here is that DrRacket shouldn't be using the errortrace-key that #lang errortrace and (require errortrace) use and should instead have its own. DrRacket also gets to "go first" ahead of #lang errortrace so the error display handler that #lang errortrace chains to will be DrRacket's, not the built in one.

I'm not sure what I was thinking when I made it reuse errortrace-key actually! This looks like the error to me. But, just as you say, I have not tried out fixing it to understand what might go wrong. (This is a very old part of DrRacket so it is also surprising to me that we've not been at this point in the past, but I can't remember it anymore, if we have.)

[sorawee]

I think that the third bullet you mention above ("Make DrRacket and") is the right approach.

Fair enough, but would it make sense to merge this PR anyway? The "right approach" will probably be much more difficult and take long time to develop. We also know this PR is safe, since it's the state before f4c495b27e5db5e9.

DrRacket also gets to "go first" ahead of #lang errortrace so the error display handler that #lang errortrace chains to will be DrRacket's, not the built in one.

If I understand correctly, this is already the case! The problem is that in this situation, you want to go last, because you get to decide for the final decision. None of this matters though if you have separate keys.

[rfindler]

I think we have a full release cycle to sort this out and I have a vague memory that that commit was important for some reason (disappointingly the commit message doesn't clarify that).

[sorawee]

Got it, I'll try again. Thanks!

[rfindler]

Let me give it a try this weekend and see how far I get?