[PATCH] guix: records: Improve error reporting.

  • Open
  • quality assurance status badge
Details
3 participants
  • Sarah Morgensen
  • Julien Lepiller
  • Ludovic Courtès
Owner
unassigned
Submitted by
Julien Lepiller
Severity
normal
J
J
Julien Lepiller wrote on 21 Jul 2021 01:40
(address . guix-patches@gnu.org)
20210721014047.3878a0c7@tachikoma.lepiller.eu
Hi Guix!

This patch improves error reporting a bit when making mistakes in guix
records. This is motivated by a user getting "invalid field specifier"
for their whole services field in their os record. With this patches,
they would have seen:

multiple values in field specifier. Got 2 values associated with key
services. Values are:
(append (list (service ...) ...))
(modify-services %desktop-services ...)

Which would have hinted them at fixing the parenthesis. Or at least, it
would have saved us some time trying to count them :)

Here are the cases that are handled and the associated message:

(operating-system
services)
guix system: error: services: invalid field specifier. The format of a
field is `(services value)'

(operating-system
(services))
test.scm:2:2: error: (services): Value missing in field specifier. The
format of a field is `(services value)'.

(operating-system
(services 1 2 3))
test.scm:2:2: error: (services 1 2 3): multiple values in field
specifier. Got 3 values associated with key services. Values are:
1
2
3

(operating-system
())
guix system: error: (): invalid field specifier. The format of a field
is `(field value)'

(operating-system
((services %desktop-services)))
test.scm:2:2: error: ((services %desktop-services)): invalid field
specifier. (services %desktop-services) is not a valid field name.


Of course, we can improve these error messages, and internationalize
them.

WDYT?
From 4cde5555682ca6a3c78cb274cc308087e51ea0e9 Mon Sep 17 00:00:00 2001
From: Julien Lepiller <julien@lepiller.eu>
Date: Wed, 21 Jul 2021 01:30:04 +0200
Subject: [PATCH] guix: records: Improve error reporting.

* guix/records.scm (report-invalid-field-specifier): Handle various
invalidity causes separately.
---
guix/records.scm | 37 ++++++++++++++++++++++++++++++++++---
1 file changed, 34 insertions(+), 3 deletions(-)

Toggle diff (58 lines)
diff --git a/guix/records.scm b/guix/records.scm
index 3d54a51956..3c3dd478a5 100644
--- a/guix/records.scm
+++ b/guix/records.scm
@@ -1,6 +1,7 @@
;;; GNU Guix --- Functional package management for GNU
;;; Copyright © 2012, 2013, 2014, 2015, 2016, 2017, 2018, 2019, 2020 Ludovic Courtès <ludo@gnu.org>
;;; Copyright © 2018 Mark H Weaver <mhw@netris.org>
+;;; Copyright © 2021 Julien Lepiller <julien@lepiller.eu>
;;;
;;; This file is part of GNU Guix.
;;;
@@ -83,10 +84,40 @@ error-reporting purposes."
;; WEIRD may be an identifier, thus lacking source location info, and
;; BINDINGS is a list, also lacking source location info. Hopefully
;; PARENT-FORM provides source location info.
- (apply syntax-violation name "invalid field specifier"
- (if parent-form
+ (syntax-case #'weird ()
+ (() ;the empty list
+ (apply syntax-violation name
+ "invalid field specifier. The format of a field is `(field value)'"
(list parent-form #'weird)
- (list #'weird)))))))
+ (list #'weird)))
+ (((field ...) _ ...) ;a list whose first element is a list
+ (apply syntax-violation name
+ (format #f "invalid field specifier. ~a is not a valid field name."
+ (map syntax->datum #'(field ...)))
+ (list parent-form #'weird)
+ (list #'weird)))
+ ((field) ;a list with one element
+ (apply syntax-violation name
+ (format #f "Value missing in field specifier. The format of \
+a field is `(~a value)'."
+ (syntax->datum #'field))
+ (list parent-form #'weird)
+ (list #'weird)))
+ ((field value ...) ;any other list
+ (apply syntax-violation name
+ (format #f "multiple values in field specifier. Got ~a values \
+associated with key ~a. Values are:~%~{~a~%~}"
+ (length #'(value ...)) (syntax->datum #'field)
+ (map syntax->datum #'(value ...)))
+ (list parent-form #'weird)
+ (list #'weird)))
+ (field ;not a list
+ (apply syntax-violation name
+ (format #f "invalid field specifier. The format of a field \
+is `(~a value)'"
+ (syntax->datum #'field))
+ (list parent-form #'weird)
+ (list #'weird))))))))
(define (report-duplicate-field-specifier name ctor)
"Report the first duplicate identifier among the bindings in CTOR."
--
2.32.0
S
S
Sarah Morgensen wrote on 21 Jul 2021 21:21
(name . Julien Lepiller)(address . julien@lepiller.eu)(address . 49671@debbugs.gnu.org)
86zgufscb0.fsf@mgsn.dev
Hi Julien,

Julien Lepiller <julien@lepiller.eu> writes:

Toggle quote (7 lines)
> Hi Guix!
>
> This patch improves error reporting a bit when making mistakes in guix
> records. This is motivated by a user getting "invalid field specifier"
> for their whole services field in their os record. With this patches,
> they would have seen:

After applying your patch, I get:

Toggle snippet (3 lines)
guix/records.scm:108:19: warning: "multiple values in field specifier. Got ~a values associated with key ~a. Values are:~%~{~a~%~}": unsupported format option ~{, use (ice-9 format) instead

After adding `(ice-9 format)` to imports it works as expected. I see
this also applies to package records! This will be great for those
starting to package in Guix.

Toggle quote (25 lines)
>
> multiple values in field specifier. Got 2 values associated with key
> services. Values are:
> (append (list (service ...) ...))
> (modify-services %desktop-services ...)
>
> Which would have hinted them at fixing the parenthesis. Or at least, it
> would have saved us some time trying to count them :)
>
> Here are the cases that are handled and the associated message:
>
> (operating-system
> services)
> guix system: error: services: invalid field specifier. The format of a
> field is `(services value)'
>
> (operating-system
> (services))
> test.scm:2:2: error: (services): Value missing in field specifier. The
> format of a field is `(services value)'.
>
> (operating-system
> (services 1 2 3))
> test.scm:2:2: error: (services 1 2 3): multiple values in field
> specifier. Got 3 values associated with key services. Values are:
^ Wrap in `'?
Toggle quote (13 lines)
> 1
> 2
> 3
>
> (operating-system
> ())
> guix system: error: (): invalid field specifier. The format of a field
> is `(field value)'
>
> (operating-system
> ((services %desktop-services)))
> test.scm:2:2: error: ((services %desktop-services)): invalid field
> specifier. (services %desktop-services) is not a valid field name.
^ Should this also be wrapped in `'?

Why do some of these messages lose their context and come from `guix
system` instead?

Toggle quote (6 lines)
>
> Of course, we can improve these error messages, and internationalize
> them.
>
> WDYT?

[...]

Toggle quote (10 lines)
> - (apply syntax-violation name "invalid field specifier"
> - (if parent-form
> + (syntax-case #'weird ()
> + (() ;the empty list
> + (apply syntax-violation name
> + "invalid field specifier. The format of a field is `(field value)'"
> (list parent-form #'weird)
> - (list #'weird)))))))
> + (list #'weird)))

Why the extra `(list #'weird')`? AFAICT right now this is providing
`(list parent-form #:'weird)` as the parent form. And since parent-form
is optional, shouldn't this be

Toggle snippet (5 lines)
(apply syntax-violation name
"invalid field specifier. The format of a field is `(field value)'"
(if parent-form (list parent-form #:'weird) (list weird)))

(and similar for the others)?

--
Sarah
L
L
Ludovic Courtès wrote on 4 Aug 2021 17:19
(name . Julien Lepiller)(address . julien@lepiller.eu)(address . 49671@debbugs.gnu.org)
87bl6dp78s.fsf@gnu.org
Hello!

Julien Lepiller <julien@lepiller.eu> skribis:

Toggle quote (34 lines)
> Here are the cases that are handled and the associated message:
>
> (operating-system
> services)
> guix system: error: services: invalid field specifier. The format of a
> field is `(services value)'
>
> (operating-system
> (services))
> test.scm:2:2: error: (services): Value missing in field specifier. The
> format of a field is `(services value)'.
>
> (operating-system
> (services 1 2 3))
> test.scm:2:2: error: (services 1 2 3): multiple values in field
> specifier. Got 3 values associated with key services. Values are:
> 1
> 2
> 3
>
> (operating-system
> ())
> guix system: error: (): invalid field specifier. The format of a field
> is `(field value)'
>
> (operating-system
> ((services %desktop-services)))
> test.scm:2:2: error: ((services %desktop-services)): invalid field
> specifier. (services %desktop-services) is not a valid field name.
>
>
> Of course, we can improve these error messages, and internationalize
> them.

I like the idea!

I would prefer for error messages to be plain errors, without hints, so:

test.scm:2:2: error: (services 1 2 3): multiple values in field specifier

(Not a sentence, no period.)

But I’d also like to have hints, and ideally all hints should be
reported consistently, via ‘&fix-hint’ or similar.

Thankfully, we can do that via (guix diagnostics) + SRFI-35/34:

(raise (condition
(&syntax (form 'x) (subform 'y))
(&fix-hint (hint "Consider fixing this."))))

‘call-with-error-handling’ in (guix ui) might need to be adjusted.

The only downside is that (guix records) would now depend on another
Guix modules, which I tried to avoid so far so it could be more easily
reused elsewhere. But that’s the price to pay to get consistent error
reports, and I think it’s okay.

Note that ‘tests/guix-system.sh’ and a couple of other files test exact
error reports, so we’ll have to make sure it still works and possibly
augment the tests.

WDYT?

Thanks,
Ludo’.
L
L
Ludovic Courtès wrote on 4 Oct 2021 16:26
Re: bug#50914: [PATCH] records: Raise a &fix-hint if a field has multiple values.
(name . Maxime Devos)(address . maximedevos@telenet.be)
87r1d0ooh2.fsf@gnu.org
Hi,

Maxime Devos <maximedevos@telenet.be> skribis:

Toggle quote (4 lines)
> * guix/records.scm (report-invalid-field-specifier): If
> 'weird' is something like (field (record ...) extra ...), hint that 'extra
> ...' should probably be moved inside (record ...).


Toggle quote (8 lines)
> + (condition
> + (&origin (origin name))
> + (&message (message
> + (format #f "field ‘~a’ should only have one \
> +value, but an extra value ‘~a’ was passed as well. Perhaps this extra \
> +value was supposed to be a field specifier, and needs to be moved inside \
> +the record ‘~a’?"

We’ll need i18n here and straight quotes.

Toggle quote (4 lines)
> + (&syntax (form (car forms))
> + (subform (and (not (null? (cdr forms)))
> + (cadr forms))))

No cadrcdr please. :-)

Toggle quote (6 lines)
> + (&fix-hint (hint (object->string
> + (syntax->datum
> + #'(field
> + (record-name fields ... extra-value
> + extra-value* ...)))))))))

The ‘hint’ field must be a string, typically the message you had above;
see other examples in the code.

TIA!

Ludo’.
J
J
Julien Lepiller wrote on 31 Oct 2021 03:06
[PATCH] guix: records: Improve error reporting.
(address . 49671@debbugs.gnu.org)
20211031030635.520a3c6c@tachikoma.lepiller.eu
Hi Guix!

Here is a new patch that attempts to better catch various causes of
syntax errors in records. I think I fixed all the concerns Ludo had,
and I draw some inspiration from https://issues.guix.gnu.org/50914for
using conditions.

Here are examples of what you get:

(operating-system
())

test.scm:1:0: error: invalid field specifier: ()
hint: The format of a field is `(field value)'



(operating-system
((services %base-services)))

test.scm:1:0: error: invalid field specifier: (services %base-services)
is not a valid field name



(operating-system
(services))

test.scm:1:0: error: missing value in field specifier
hint: The field is missing a value: `(services <value>)'.



(operating-system
(services (service tor-service-type) %base-services))

test.scm:1:0: error: multiple values in field specifier
hint: 2 values were associated with field `services'. We got the
following values:

(service tor-service-type)
%base-services

Perhaps the additional values were intended to be other field
specifiers. This usually indicates missing or misplaced parenthesis.



(operating-system
services %base-services)

test.scm:1:0: error: invalid field specifier: #<syntax services>
hint: The format of a field is `(field value)'



(not sure why the last one is wrapped in syntax...)
From 9e08a887a08da3f0cc132d148b748eb2ce7db135 Mon Sep 17 00:00:00 2001
Message-Id: <9e08a887a08da3f0cc132d148b748eb2ce7db135.1635645523.git.julien@lepiller.eu>
From: Julien Lepiller <julien@lepiller.eu>
Date: Sun, 31 Oct 2021 02:58:14 +0100
Subject: [PATCH] guix: records: Improve error reporting.

* guix/records.scm (report-invalid-field-specifier): Handle various
invalidity causes separately.
---
guix/records.scm | 56 ++++++++++++++++++++++++++++++++++++++++++++----
1 file changed, 52 insertions(+), 4 deletions(-)

Toggle diff (86 lines)
diff --git a/guix/records.scm b/guix/records.scm
index ed94c83dac..9baa2c2f1d 100644
--- a/guix/records.scm
+++ b/guix/records.scm
@@ -1,6 +1,7 @@
;;; GNU Guix --- Functional package management for GNU
;;; Copyright © 2012, 2013, 2014, 2015, 2016, 2017, 2018, 2019, 2020, 2021 Ludovic Courtès <ludo@gnu.org>
;;; Copyright © 2018 Mark H Weaver <mhw@netris.org>
+;;; Copyright © 2021 Julien Lepiller <julien@lepiller.eu>
;;;
;;; This file is part of GNU Guix.
;;;
@@ -21,9 +22,13 @@ (define-module (guix records)
#:use-module (srfi srfi-1)
#:use-module (srfi srfi-9)
#:use-module (srfi srfi-26)
+ #:use-module (srfi srfi-35)
+ #:use-module (ice-9 format)
#:use-module (ice-9 match)
#:use-module (ice-9 regex)
#:use-module (ice-9 rdelim)
+ #:use-module (guix diagnostics)
+ #:use-module (guix ui)
#:autoload (system base target) (target-most-positive-fixnum)
#:export (define-record-type*
this-record
@@ -83,10 +88,53 @@ (define-syntax record-error
;; WEIRD may be an identifier, thus lacking source location info, and
;; BINDINGS is a list, also lacking source location info. Hopefully
;; PARENT-FORM provides source location info.
- (apply syntax-violation name "invalid field specifier"
- (if parent-form
- (list parent-form #'weird)
- (list #'weird)))))))
+ (let* ((weird-properties (source-properties #'weird))
+ (parent-properties (and parent-form (syntax-source parent-form)))
+ (location (source-properties->location
+ (or (and (not (null? weird-properties)) weird-properties)
+ (and (not (null? parent-properties)) parent-properties)
+ '()))))
+ (syntax-case #'weird ()
+ (() ;the empty list
+ (raise-exception
+ (condition
+ (&message (message (format #f (G_ "invalid field specifier: ~a")
+ #'weird)))
+ (&error-location (location location))
+ (&fix-hint (hint (G_ "The format of a field is `(field value)'"))))))
+ (((field ...) _ ...) ;a list whose first element is a list
+ (raise-exception
+ (condition
+ (&message (message (format #f (G_ "invalid field specifier: ~a \
+is not a valid field name")
+ (map syntax->datum #'(field ...)))))
+ (&error-location (location location)))))
+ ((field) ;a list with one element
+ (raise-exception
+ (condition
+ (&message (message (G_ "missing value in field specifier")))
+ (&error-location (location location))
+ (&fix-hint (hint (format #f (G_ "The field is missing a value: `(~a <value>)'.")
+ (syntax->datum #'field)))))))
+ ((field value ...) ;any other list
+ (raise-exception
+ (condition
+ (&message (message (G_ "multiple values in field specifier")))
+ (&error-location (location location))
+ (&fix-hint (hint (format #f (G_ "~a values were associated with \
+field `~a'. We got the following values:~%@lisp~%~{~a~%~}~%@end lisp~%Perhaps the additional values
+were intended to be other field specifiers. This usually indicates missing or \
+misplaced parenthesis.")
+ (length #'(value ...))
+ (syntax->datum #'field)
+ (map syntax->datum #'(value ...))))))))
+ (field ;not a list
+ (raise-exception
+ (condition
+ (&message (message (format #f (G_ "invalid field specifier: ~a")
+ #'weird)))
+ (&error-location (location location))
+ (&fix-hint (hint (G_ "The format of a field is `(field value)'"))))))))))))
(define (report-duplicate-field-specifier name ctor)
"Report the first duplicate identifier among the bindings in CTOR."
--
2.33.1
J
J
Julien Lepiller wrote on 22 Nov 2021 03:40
Re: [bug#49671] [PATCH v3] guix: records: Improve error reporting.
(address . 49671@debbugs.gnu.org)
20211122034022.152daf55@tachikoma.lepiller.eu
Here is another improvement compared to v2. This time there are two
patches: the first adds support for &syntax in (guix ui), and will
print something like

in form: <pretty-printed-form>

where "in form" is in green.

The second patch is very similar to v2, but will now also raise a
&syntax condition, so it can be pretty-printed. The previous issue
where I printed #<syntax ...> is fixed, I simply forgot a syntax->datum.

WDYT?
From d1d857ea0ca76f1a917382777f57871e50026df3 Mon Sep 17 00:00:00 2001
Message-Id: <d1d857ea0ca76f1a917382777f57871e50026df3.1637548566.git.julien@lepiller.eu>
From: Julien Lepiller <julien@lepiller.eu>
Date: Mon, 22 Nov 2021 02:54:06 +0100
Subject: [PATCH 1/2] guix: ui: Print syntax errors.

* guix/ui.scm (display-syntax): New procedure.
(call-with-error-handling, report-load-error): Print syntax errors.
---
guix/ui.scm | 24 ++++++++++++++++++++++++
1 file changed, 24 insertions(+)

Toggle diff (68 lines)
diff --git a/guix/ui.scm b/guix/ui.scm
index bd999103ff..79aab2db84 100644
--- a/guix/ui.scm
+++ b/guix/ui.scm
@@ -69,9 +69,11 @@ (define-module (guix ui)
#:use-module (srfi srfi-31)
#:use-module (srfi srfi-34)
#:use-module (srfi srfi-35)
+ #:use-module (ice-9 exceptions)
#:autoload (ice-9 ftw) (scandir)
#:use-module (ice-9 match)
#:use-module (ice-9 format)
+ #:use-module (ice-9 pretty-print)
#:use-module (ice-9 regex)
#:autoload (ice-9 popen) (open-pipe* close-pipe)
#:autoload (system repl repl) (start-repl)
@@ -315,6 +317,19 @@ (define* (display-hint message #:optional (port (current-error-port)))
(texi->plain-text message))
port))
+(define %syntax-color (color BOLD GREEN))
+
+(define* (display-syntax form #:optional (port (current-error-port)))
+ "Display FORM, an sexp, to PORT"
+ (define colorize
+ (if (color-output? port)
+ (lambda (str)
+ (colorize-string str %syntax-color))
+ identity))
+
+ (display (colorize (G_ "in form: ")) port)
+ (pretty-print form port))
+
(define* (report-unbound-variable-error args #:key frame)
"Return the given unbound-variable error, where ARGS is the list of 'throw'
arguments."
@@ -398,6 +413,9 @@ (define* (report-load-error file args #:optional frame)
(formatted-message-arguments obj)))
(else
(report-error (G_ "exception thrown: ~s~%") obj)))
+ (when (syntax-error? obj)
+ (let ((form (or (syntax-error-subform obj) (syntax-error-form obj))))
+ (display-syntax form)))
(when (fix-hint? obj)
(display-hint (condition-fix-hint obj))))
((key args ...)
@@ -801,6 +819,9 @@ (define (call-with-error-handling thunk)
(and (error-location? c) (error-location c))
(gettext (formatted-message-string c) %gettext-domain)
(formatted-message-arguments c))
+ (when (syntax-error? c)
+ (let ((form (or (syntax-error-subform c) (syntax-error-form c))))
+ (display-syntax form)))
(when (fix-hint? c)
(display-hint (condition-fix-hint c)))
(exit 1))
@@ -826,6 +847,9 @@ (define (call-with-error-handling thunk)
(report-error (and (error-location? c) (error-location c))
(G_ "~a~%")
(gettext (condition-message c) %gettext-domain))
+ (when (syntax-error? c)
+ (let ((form (or (syntax-error-subform c) (syntax-error-form c))))
+ (display-syntax form)))
(when (fix-hint? c)
(display-hint (condition-fix-hint c)))
(exit 1)))
--
2.33.1
Attachment: 0002-guix-records-Improve-error-reporting.patch
J
J
Julien Lepiller wrote on 17 Jul 2022 08:23
(address . 49671@debbugs.gnu.org)
20220717082339.665a7385@sybil.lepiller.eu
Hi Guix!

A few months have passed without comment. I don't feel confident enough
to change Guix internals without comments.

What are your thoughts on this?

Le Mon, 22 Nov 2021 03:40:22 +0100,
Julien Lepiller <julien@lepiller.eu> a écrit :

Toggle quote (14 lines)
> Here is another improvement compared to v2. This time there are two
> patches: the first adds support for &syntax in (guix ui), and will
> print something like
>
> in form: <pretty-printed-form>
>
> where "in form" is in green.
>
> The second patch is very similar to v2, but will now also raise a
> &syntax condition, so it can be pretty-printed. The previous issue
> where I printed #<syntax ...> is fixed, I simply forgot a
> syntax->datum.
>
> WDYT?
?
Your comment

Commenting via the web interface is currently disabled.

To comment on this conversation send an email to 49671@debbugs.gnu.org

To respond to this issue using the mumi CLI, first switch to it
mumi current 49671
Then, you may apply the latest patchset in this issue (with sign off)
mumi am -- -s
Or, compose a reply to this issue
mumi compose
Or, send patches to this issue
mumi send-email *.patch