From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Andy Wingo Newsgroups: gmane.lisp.guile.devel Subject: Re: [PATCH] Fix error messages involving internal definitions Date: Fri, 27 Jan 2012 12:27:16 +0100 Message-ID: <87obtpgzcb.fsf@pobox.com> References: <87sjj1obca.fsf@netris.org> NNTP-Posting-Host: lo.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Trace: dough.gmane.org 1327672153 10227 80.91.229.12 (27 Jan 2012 13:49:13 GMT) X-Complaints-To: usenet@dough.gmane.org NNTP-Posting-Date: Fri, 27 Jan 2012 13:49:13 +0000 (UTC) Cc: guile-devel@gnu.org To: Mark H Weaver Original-X-From: guile-devel-bounces+guile-devel=m.gmane.org@gnu.org Fri Jan 27 14:49:09 2012 Return-path: Envelope-to: guile-devel@m.gmane.org Original-Received: from lists.gnu.org ([140.186.70.17]) by lo.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1RqmAu-0000b7-9t for guile-devel@m.gmane.org; Fri, 27 Jan 2012 14:49:08 +0100 Original-Received: from localhost ([::1]:39218 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RqmAt-0003wH-Fv for guile-devel@m.gmane.org; Fri, 27 Jan 2012 08:49:07 -0500 Original-Received: from eggs.gnu.org ([140.186.70.92]:58850) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RqmAn-0003wC-7L for guile-devel@gnu.org; Fri, 27 Jan 2012 08:49:05 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RqmAg-0007Tq-TL for guile-devel@gnu.org; Fri, 27 Jan 2012 08:49:01 -0500 Original-Received: from a-pb-sasl-sd.pobox.com ([74.115.168.62]:35261 helo=sasl.smtp.pobox.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RqmAg-0007Tb-Ou for guile-devel@gnu.org; Fri, 27 Jan 2012 08:48:54 -0500 Original-Received: from sasl.smtp.pobox.com (unknown [127.0.0.1]) by a-pb-sasl-sd.pobox.com (Postfix) with ESMTP id 3FC22852D; Fri, 27 Jan 2012 08:48:53 -0500 (EST) DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=pobox.com; h=from:to:cc :subject:in-reply-to:references:date:message-id:mime-version :content-type; s=sasl; bh=sg9K92Ef0ipUmzOHCihBx8EZDN4=; b=su7R+B 0gA8BWuzXYTFCFrtiX4rfmrY2UsXhg6JE7c+22BUzmJ/heXvxVKj/S7TUBjmi11D C59M16AwsF32yJTZpbwZT0THCBkK245URGwLC6MGdSIBU4AYiHKEu6Mr/lTqzra4 VYEPKq1PZTGs+MRHR+E/AW34uhCw2aTIm6On0= DomainKey-Signature: a=rsa-sha1; c=nofws; d=pobox.com; h=from:to:cc :subject:in-reply-to:references:date:message-id:mime-version :content-type; q=dns; s=sasl; b=CKAZVUqscdJw+ZAlcacIq0GDfUr6jFb/ 4EzVvugky2KIOpZa3NmPClL+J36cTXLBzW3bvJPmCytkJcc+juMwFTEIDiy2vmWc jzDJGF9cPDRzh77strQ+I2hn2c5t5ATIeQqXswRQUGllYwAlupRD22QyQkudV3vq sKIr31K9DQA= Original-Received: from a-pb-sasl-sd.pobox.com (unknown [127.0.0.1]) by a-pb-sasl-sd.pobox.com (Postfix) with ESMTP id 3A15D852C; Fri, 27 Jan 2012 08:48:53 -0500 (EST) Original-Received: from badger (unknown [90.164.198.39]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by a-pb-sasl-sd.pobox.com (Postfix) with ESMTPSA id 71A42852B; Fri, 27 Jan 2012 08:48:52 -0500 (EST) In-Reply-To: <87sjj1obca.fsf@netris.org> (Mark H. Weaver's message of "Fri, 27 Jan 2012 02:26:13 -0500") User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.3 (gnu/linux) X-Pobox-Relay-ID: A5EE5020-48ED-11E1-BC01-65B1DE995924-02397024!a-pb-sasl-sd.pobox.com X-detected-operating-system: by eggs.gnu.org: Solaris 10 (beta) X-Received-From: 74.115.168.62 X-BeenThere: guile-devel@gnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "Developers list for Guile, the GNU extensibility library" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guile-devel-bounces+guile-devel=m.gmane.org@gnu.org Original-Sender: guile-devel-bounces+guile-devel=m.gmane.org@gnu.org Xref: news.gmane.org gmane.lisp.guile.devel:13708 Archived-At: 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 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/