[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?
?