racket / drracket

DrRacket, IDE for Racket
http://www.racket-lang.org/
Other
450 stars 95 forks source link

DrRacket errortrace annotates macro-introduced code #157

Open michaelballantyne opened 6 years ago

michaelballantyne commented 6 years ago

This issue describes one of the root causes of https://github.com/racket/htdp/issues/42.

The problem

Macro-introduced expressions in a file evaluated by DrRacket with debug turned on are always errortrace annotated, even when they originate from a file that was not errortrace annotated. For example, if an installed package with collection foo includes bar.rkt that is compiled to bytecode without errortrace annotation:

#lang racket
(define-syntax-rule (m f)
  (define (f arg)
    (error 'bad "bad")))
(provide m)

And I run baz.rkt, which is installed in a different package, in DrRacket:

#lang racket
(require foo/bar)
(m f)
(define (g arg)
  (f arg))
(g '())

The resulting stacktrace includes an entry for (error 'bad "bad"), but not for (f arg), because the macro-introduced (error 'bad "bad") expression was annotated and thus its continuation mark overwrites that from (f arg).

I'm not certain of the design intent here, but it seems as though DrRacket tries to avoid pointing to internals of library implementations when reporting errors unless those library implementations are in the same package as the file being run or some file from the implementation package is open in DrRacket for editing. For example, the stacktrace for:

#lang racket
(define (f x)
  (first x))
(f 5)

includes the call to (first 5) and not any location within the implementation of first in racket/list. The behavior for macro-introduced code seems inconsistent with this.

Likely cause

DrRacket overrides current-eval here with an implementation that expands syntax with a stack checkpoint wrapped around before passing it along.

That means that syntax that reaches the errortrace current-compile handler installed here will already be fully-expanded, so the dance errortrace does with the errortrace:annotate property to avoid annotating introduced syntax doesn't help.

I don't understand the design of the overridden current-eval well enough to have an idea of how to fix things. I'm not sure why

(current-eval
  (let ([oe (current-eval)])
    (lambda (sexp/syntax) 
       (with-stack-checkpoint
          (oe sexp/syntax)))))

wouldn't suffice.

I'm happy to work on a fix if someone can point me in the right direction!

rfindler commented 6 years ago

I think the current design is just wrong. It was somewhat more reasonable before we moved to the pkg world where there was a clear set of collections that shouldn't be annotated. But the "is there a file open in this pkg?" predicate does not seem to be cutting it. In addition to various technical problems with the current implementation attempt (beyond the one you outline, I think the current implementation gets confused about which .zo files to use when it encounters a file that depends on an unannotated one and that one, in turn, depends back on an annotated one; something like that anyway, I'm forgetting the precise details now).

In any case, if you think there is a better approach that we should use, I'd be delighted to discuss it and then review pull requests on it. This has long been on my list, but I just have not found the time.

rfindler commented 6 years ago

Hm, on a re-read I see that you were not offering to work on the problem I was thinking you were offering to help with. :)

To the question at hand: I cannot recall if this is the only reason, but one reason why DrRacket might be doing that is to avoid the stack frames that do that "dance" inside the eval handler.

As for the suggested change, I'm not seeing the right stack with that change either. Printing cms in lang/htdp-langs (in the body of teaching-languages-error-display-handler) shows only lang/private/teach (line 1002) and lang/private/teach-module-begin (line 30). Do you see differently?

mfelleisen commented 6 years ago

@rfindler:

As for the suggested change, I'm not seeing the right stack with that change either. Printing cms in lang/htdp-langs (in the body of teaching-languages-error-display-handler) shows only lang/private/teach (line 1002) and lang/private/teach-module-begin (line 30). Do you see differently

I ran into the same problem with #42 and also printed cms there with your result. I believe Michael got to here from there but see other thread for fix of 42.

michaelballantyne commented 6 years ago

The teaching languages fiddle with evaluation as well, which complicates things.

Replacing DrRacket's evaluation handler

with

(current-eval
  (let ([oe (current-eval)])
    (lambda (sexp/syntax) 
       (with-stack-checkpoint
          (oe sexp/syntax)))))

appears to fix the stacktrace when running BSL as a #lang, like this:

#lang htdp/bsl
(define-struct s [x])
(s-x '())

but not with the Beginning Student language option. I suspect this is to blame.

michaelballantyne commented 6 years ago

That evaluation handler change exposes stack frames in the errortrace compile handler, for one.

#lang racket
(define-syntax (m stx)
  (error 'b "b"))
(m)

trace

One idea would be to factor "stack-checkpoint.rkt" out of DrRacket and have tooling that drives expansion, like errortrace, use with-stack-checkpoint.

Another would be to do all the filtering of stack frames after the fact, discarding frames based on the "is there a file open in this package?" rule rather than just not annotating such files.