* [PATCH] Fix error messages involving internal definitions
@ 2012-01-27 7:26 Mark H Weaver
2012-01-27 7:58 ` Mark H Weaver
2012-01-27 11:27 ` Andy Wingo
0 siblings, 2 replies; 4+ messages in thread
From: Mark H Weaver @ 2012-01-27 7:26 UTC (permalink / raw)
To: Andy Wingo; +Cc: guile-devel
[-- Attachment #1: Type: text/plain, Size: 1855 bytes --]
Hi Andy,
I know this is very last minute, but it would be great if you could
include this fix in 2.0.4. Since the release is so close, I was _very_
careful writing it and testing it, and am quite confident that it is
correct. (After regenerating psyntax-pp.scm, I removed all .go files,
recompiled everything, and did a 'make check'. I also did several other
tests by hand targetting the changed code.)
I very nearly pushed this, but I know that you are very opinionated
about psyntax changes, and this fix did require adding another parameter
to 'expand-expr' and another return value to 'syntax-type', so I wanted
to run it by you. Unless you object to the basic strategy here, please
take my word for it that it is correct, and push it.
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, even when compiling a module in the
normal way. Secondly, instead of printing the definition form, it
reports the identifier as the 'subform', and the rhs expression as the
'form'. Thirdly, "definition in expression context" is a confusing
message for Scheme beginners, who are likely to make this mistake.
This came up because Bruce Korb recently posted here, quite confused
(and understandably so) about what could be the cause of this misleading
error message.
For example, if you try to compile a module containing:
(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)
Okay, here's the patch, with the syntax-pp.scm portion removed.
Mark
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: [PATCH] Fix error messages involving internal definitions --]
[-- Type: text/x-patch, Size: 12473 bytes --]
From 278aed1c13ba183458a9e4cd6ec66df1dadf98c9 Mon Sep 17 00:00:00 2001
From: Mark H Weaver <mhw@netris.org>
Date: Thu, 26 Jan 2012 23:55:24 -0500
Subject: [PATCH] Fix error messages involving internal definitions
* module/ice-9/psyntax.scm (syntax-type): Return an additional value
that contains the entire form in _all_ cases, including for definition
forms. Previously, the entire form was not returned for definition
forms.
(expand-expr): Add an additional argument that contains the entire
form in _all_ cases, including for definition forms. Use it to
include the entire form in the error message for internal definitions
in contexts where they are not allowed, and improve the error message
to be more comprehensible for Scheme beginners. Also include the
source location information, which was previously missing from the
error message.
(expand-top-sequence, expand, expand-body): Receive the additional
return value from 'syntax-type'.
* module/ice-9/psyntax-pp.scm: Regenerate.
* NEWS: Add news entry.
---
NEWS | 1 +
module/ice-9/psyntax-pp.scm |12980 ++++++++++++++++++++++---------------------
module/ice-9/psyntax.scm | 75 +-
3 files changed, 6587 insertions(+), 6469 deletions(-)
diff --git a/NEWS b/NEWS
index ad0910c..5a74fc9 100644
--- a/NEWS
+++ b/NEWS
@@ -173,6 +173,7 @@ Search the manual for these identifiers and modules, for more.
** Fix <dynwind> serialization.
** Fix erroneous check in `set-procedure-properties!'.
** Fix generalized-vector-{ref,set!} for slices.
+** Fix error messages involving internal definitions.
** HTTP: Extend handling of "Cache-Control" header.
** HTTP: Fix qstring writing of cache-extension values
** HTTP: Fix validators for various list-style headers.
diff --git a/module/ice-9/psyntax.scm b/module/ice-9/psyntax.scm
index 20ea8eb..9812f3a 100644
--- a/module/ice-9/psyntax.scm
+++ b/module/ice-9/psyntax.scm
@@ -982,7 +982,7 @@
(lambda ()
(let ((e (car body)))
(syntax-type e r w (or (source-annotation e) s) #f mod #f)))
- (lambda (type value e w s mod)
+ (lambda (type value form e w s mod)
(case type
((begin-form)
(syntax-case e ()
@@ -1079,18 +1079,20 @@
exps)))
((displaced-lexical)
(syntax-violation #f "identifier out of context"
- e (wrap value w mod)))
+ (source-wrap form w s mod)
+ (wrap value w mod)))
(else
(syntax-violation #f "cannot define keyword at top level"
- e (wrap value w mod))))))
+ (source-wrap form w s mod)
+ (wrap value w mod))))))
(else
(values (cons
(if (eq? m 'c&e)
- (let ((x (expand-expr type value e r w s mod)))
+ (let ((x (expand-expr type value form e r w s mod)))
(top-level-eval-hook x mod)
x)
(lambda ()
- (expand-expr type value e r w s mod)))
+ (expand-expr type value form e r w s mod)))
exps)))))))
(lambda (exps)
(scan (cdr body) r w s m esew mod exps))))))
@@ -1132,8 +1134,8 @@
(syntax-violation 'eval-when "invalid situation" e
(car l))))))))
- ;; syntax-type returns six values: type, value, e, w, s, and mod. The
- ;; first two are described in the table below.
+ ;; syntax-type returns seven values: type, value, form, e, w, s, and
+ ;; mod. The first two are described in the table below.
;;
;; type value explanation
;; -------------------------------------------------------------------
@@ -1162,10 +1164,11 @@
;; constant none self-evaluating datum
;; other none anything else
;;
- ;; For definition forms (define-form, define-syntax-parameter-form,
- ;; and define-syntax-form), e is the rhs expression. For all
- ;; others, e is the entire form. w is the wrap for e. s is the
- ;; source for the entire form. mod is the module for e.
+ ;; form is the entire form. For definition forms (define-form,
+ ;; define-syntax-form, and define-syntax-parameter-form), e is the
+ ;; rhs expression. For all others, e is the entire form. w is the
+ ;; wrap for both form and e. s is the source for the entire form.
+ ;; mod is the module for both form and e.
;;
;; syntax-type expands macros and unwraps as necessary to get to one
;; of the forms above. It also parses definition forms, although
@@ -1179,28 +1182,28 @@
(b (lookup n r mod))
(type (binding-type b)))
(case type
- ((lexical) (values type (binding-value b) e w s mod))
- ((global) (values type n e w s mod))
+ ((lexical) (values type (binding-value b) e e w s mod))
+ ((global) (values type n e e w s mod))
((macro)
(if for-car?
- (values type (binding-value b) e w s mod)
+ (values type (binding-value b) e e w s mod)
(syntax-type (expand-macro (binding-value b) e r w s rib mod)
r empty-wrap s rib mod #f)))
- (else (values type (binding-value b) e w s mod)))))
+ (else (values type (binding-value b) e e w s mod)))))
((pair? e)
(let ((first (car e)))
(call-with-values
(lambda () (syntax-type first r w s rib mod #t))
- (lambda (ftype fval fe fw fs fmod)
+ (lambda (ftype fval fform fe fw fs fmod)
(case ftype
((lexical)
- (values 'lexical-call fval e w s mod))
+ (values 'lexical-call fval e e w s mod))
((global)
;; If we got here via an (@@ ...) expansion, we need to
;; make sure the fmod information is propagated back
;; correctly -- hence this consing.
(values 'global-call (make-syntax-object fval w fmod)
- e w s mod))
+ e e w s mod))
((macro)
(syntax-type (expand-macro fval e r w s rib mod)
r empty-wrap s rib mod for-car?))
@@ -1209,23 +1212,24 @@
(lambda (e r w s mod)
(syntax-type e r w s rib mod for-car?))))
((core)
- (values 'core-form fval e w s mod))
+ (values 'core-form fval e e w s mod))
((local-syntax)
- (values 'local-syntax-form fval e w s mod))
+ (values 'local-syntax-form fval e e w s mod))
((begin)
- (values 'begin-form #f e w s mod))
+ (values 'begin-form #f e e w s mod))
((eval-when)
- (values 'eval-when-form #f e w s mod))
+ (values 'eval-when-form #f e e w s mod))
((define)
(syntax-case e ()
((_ name val)
(id? #'name)
- (values 'define-form #'name #'val w s mod))
+ (values 'define-form #'name e #'val w s mod))
((_ (name . args) e1 e2 ...)
(and (id? #'name)
(valid-bound-ids? (lambda-var-list #'args)))
;; need lambda here...
(values 'define-form (wrap #'name w mod)
+ (wrap e w mod)
(decorate-source
(cons #'lambda (wrap #'(args e1 e2 ...) w mod))
s)
@@ -1233,38 +1237,39 @@
((_ name)
(id? #'name)
(values 'define-form (wrap #'name w mod)
+ (wrap e w mod)
#'(if #f #f)
empty-wrap s mod))))
((define-syntax)
(syntax-case e ()
((_ name val)
(id? #'name)
- (values 'define-syntax-form #'name #'val w s mod))))
+ (values 'define-syntax-form #'name e #'val w s mod))))
((define-syntax-parameter)
(syntax-case e ()
((_ name val)
(id? #'name)
- (values 'define-syntax-parameter-form #'name #'val w s mod))))
+ (values 'define-syntax-parameter-form #'name e #'val w s mod))))
(else
- (values 'call #f e w s mod)))))))
+ (values 'call #f e e w s mod)))))))
((syntax-object? e)
(syntax-type (syntax-object-expression e)
r
(join-wraps w (syntax-object-wrap e))
(or (source-annotation e) s) rib
(or (syntax-object-module e) mod) for-car?))
- ((self-evaluating? e) (values 'constant #f e w s mod))
- (else (values 'other #f e w s mod)))))
+ ((self-evaluating? e) (values 'constant #f e e w s mod))
+ (else (values 'other #f e e w s mod)))))
(define expand
(lambda (e r w mod)
(call-with-values
(lambda () (syntax-type e r w (source-annotation e) #f mod #f))
- (lambda (type value e w s mod)
- (expand-expr type value e r w s mod)))))
+ (lambda (type value form e w s mod)
+ (expand-expr type value form e r w s mod)))))
(define expand-expr
- (lambda (type value e r w s mod)
+ (lambda (type value form e r w s mod)
(case type
((lexical)
(build-lexical-reference 'value s e value))
@@ -1318,8 +1323,8 @@
(expand-sequence #'(e1 e2 ...) r w s mod)
(expand-void))))))
((define-form define-syntax-form define-syntax-parameter-form)
- (syntax-violation #f "definition in expression context"
- e (wrap value w mod)))
+ (syntax-violation #f "internal definition in a context where definitions are not allowed"
+ (source-wrap form w s mod)))
((syntax)
(syntax-violation #f "reference to pattern variable outside syntax form"
(source-wrap e w s mod)))
@@ -1463,7 +1468,7 @@
(let ((e (cdar body)) (er (caar body)))
(call-with-values
(lambda () (syntax-type e er empty-wrap (source-annotation er) ribcage mod #f))
- (lambda (type value e w s mod)
+ (lambda (type value form e w s mod)
(case type
((define-form)
(let ((id (wrap value w mod)) (label (gen-label)))
@@ -2222,7 +2227,7 @@
((_ (head tail ...) val)
(call-with-values
(lambda () (syntax-type #'head r empty-wrap no-source #f mod #t))
- (lambda (type value ee ww ss modmod)
+ (lambda (type value formform ee ww ss modmod)
(case type
((module-ref)
(let ((val (expand #'val r w mod)))
--
1.7.5.4
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] Fix error messages involving internal definitions
2012-01-27 7:26 [PATCH] Fix error messages involving internal definitions Mark H Weaver
@ 2012-01-27 7:58 ` Mark H Weaver
2012-01-27 11:27 ` Andy Wingo
1 sibling, 0 replies; 4+ messages in thread
From: Mark H Weaver @ 2012-01-27 7:58 UTC (permalink / raw)
To: Andy Wingo; +Cc: guile-devel
I wrote:
> 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, [...]
Sorry, I meant to say "is _never_ provided if the rhs expression is an
atom".
Mark
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Fix error messages involving internal definitions
2012-01-27 7:26 [PATCH] Fix error messages involving internal definitions Mark H Weaver
2012-01-27 7:58 ` Mark H Weaver
@ 2012-01-27 11:27 ` Andy Wingo
2012-01-27 14:26 ` Mark H Weaver
1 sibling, 1 reply; 4+ messages in thread
From: Andy Wingo @ 2012-01-27 11:27 UTC (permalink / raw)
To: Mark H Weaver; +Cc: guile-devel
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/
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] Fix error messages involving internal definitions
2012-01-27 11:27 ` Andy Wingo
@ 2012-01-27 14:26 ` Mark H Weaver
0 siblings, 0 replies; 4+ messages in thread
From: Mark H Weaver @ 2012-01-27 14:26 UTC (permalink / raw)
To: Andy Wingo; +Cc: guile-devel
Andy Wingo <wingo@pobox.com> writes:
>> 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?
How about if we simply remove 'internal' from the error message?
> "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.
How about this?
"definition in expression context, where definitions are not allowed,"
> 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.
I agree that it is painful to add another value, but personally I think
it is very important to include the entire from to make the error
message comprehensible, especially for Scheme beginners who are quite
likely to make this mistake.
> Feel free to commit whatever you think is best, here.
Thanks!
Mark
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2012-01-27 14:26 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-01-27 7:26 [PATCH] Fix error messages involving internal definitions Mark H Weaver
2012-01-27 7:58 ` Mark H Weaver
2012-01-27 11:27 ` Andy Wingo
2012-01-27 14:26 ` Mark H Weaver
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).