Re: [PATCH] Fix error messages involving internal definitions

[Andy Wingo <wingo@pobox.com>]

Hi Mark!

Thanks for tracking down this issue.  I'm sure it will make it in 2.0.4,
but I do have a couple questions.

On Fri 27 Jan 2012 08:26, Mark H Weaver <mhw@netris.org> writes:

> So, what does this fix?  The "definition in expression context" error
> message is broken in several ways.  First of all, source location
> information is _never_ provided if the rhs expression is an atom, even
> when compiling a module in the normal way.

Indeed.  In fact that was the reason for this terrible misguided hack:

> Secondly, instead of printing the definition form, it
> reports the identifier as the 'subform', and the rhs expression as the
> 'form'.

Since I knew the identifier would not carry source properties, I threw
the RHS into the error on the off chance that it would have source
properties, and could help the user track down the error.  But this led
to bad error messages even in the best case of the RHS being a pair...

> Thirdly, "definition in expression context" is a confusing message for
> Scheme beginners, who are likely to make this mistake.

The problem is that I'm not sure that the error message you suggest is
correct.  You show:

>   (let ((x 1))
>     #f
>     (define blah 3))
>
> Currently, you get a message like this:
>
>   unknown location: definition in expression context in subform blah of 3
>
> With this patch, you get a message like this:
>
>   /home/mhw/guile-modules/foo.scm:5:2: internal definition in a context where definitions are not allowed in form (define blah 3)

And this is much better.  But, it is not the right error message for a
form like:

  (if 1
      (define bar 2))

So, that's question 1: can we come up with some other message that's
more helpful while also being accurate?

"Definition in expression context" does have the advantage that it can
be searched for in the manual (if we put it there), or on the web.  If
all things were equal, it would have the advantage of being shorter as
well.

Question 2 is about the implementation.  I'm sure you winced as much as
I did at adding a seventh return value from syntax-type :)  I was
reading though and noted in the comment above syntax-type that the "s"
return value already has the source information for the expression.  So
a more minimal change like the attached patch yields the error message:

  /tmp/foo.scm:5:2: definition in expression context in form blah

WDYT?  I think I prefer the more minimal approach in that patch, but
either way is fine.

Feel free to commit whatever you think is best, here.

Andy

diff --git a/module/ice-9/psyntax.scm b/module/ice-9/psyntax.scm
index 20ea8eb..46883fe 100644
--- a/module/ice-9/psyntax.scm
+++ b/module/ice-9/psyntax.scm
@@ -1319,13 +1319,13 @@
                     (expand-void))))))
           ((define-form define-syntax-form define-syntax-parameter-form)
            (syntax-violation #f "definition in expression context"
-                             e (wrap value w mod)))
+                             value #:source s))
           ((syntax)
            (syntax-violation #f "reference to pattern variable outside syntax form"
-                             (source-wrap e w s mod)))
+                             e #:source s))
           ((displaced-lexical)
            (syntax-violation #f "reference to identifier outside its scope"
-                             (source-wrap e w s mod)))
+                             e #:source s))
           (else (syntax-violation #f "unexpected syntax"
                                   (source-wrap e w s mod))))))
 
@@ -2542,12 +2542,12 @@
             (bound-id=? x y)))
 
     (set! syntax-violation
-          (lambda* (who message form #:optional subform)
+          (lambda* (who message form #:optional subform
+                        #:key (source (source-annotation (or form subform))))
             (arg-check (lambda (x) (or (not x) (string? x) (symbol? x)))
                        who 'syntax-violation)
             (arg-check string? message 'syntax-violation)
-            (throw 'syntax-error who message
-                   (source-annotation (or form subform))
+            (throw 'syntax-error who message source
                    (strip form empty-wrap)
                    (and subform (strip subform empty-wrap)))))
 

-- 
http://wingolog.org/