[bug#51922] [PATCH 0/2] Improve the reported location of configuration warnings

[bug#51922] [PATCH 0/2] Improve the reported location of configuration warnings

[Josselin Poiret via Guix-patches via]

Hello everyone,

While working on the swap-space patch, I noticed that currently,
warnings about deprecated fields in guix records use the location of
the record definition macro, rather than of the invalid value.  For
some records such as 'operating-system', this makes it rather user
unfriendly and confusing.

This patchset first adds the syntax 'define-with-syntax-properties',
which helps avoid boilerplate code to define sanitizers with proper
location reporting.  I put it in guix/diagnostics.scm as I thought
this was the place that was most likely to be use-module'd for warning
messages, as this is quite tied to that use.  The second patch makes
use of this new helper to update two warnings: the one about 'target'
to 'targets' in bootloader.scm, and the one about setuid-programs.  In
both cases, a `guix system reconfigure` now reports the exact location
of the incriminating values, rather than of the
'bootloader-configuration' or 'operating-system' lines respectively.

The approach I've taken for the helper was to make something simple
and general enough for most uses.  It should correctly report syntax
errors with custom errors messages rather than the generic "source
expression failed to match any pattern ...".  Note although that it
isn't possible to do any defines in the body of the macro, as this
doesn't use any lambda-like macros (see the second case for an
example).

I can see two drawbacks to this macro:
1) This macro will not help you write expand-time checkers.  This
would introduce too much complexity, and I'm not sure the end-user
would notice a significant change.
2) It doesn't deconstruct values such as lists to get the individual
list values's properties.  This would also introduce too much
complexity (eg. checking if the list is literal, deconstructing it,
and falling back to a generic source location if the list is only
available at eval-time).

Best,
Josselin Poiret

Josselin Poiret (2):
  guix: Add syntax to capture arguments' syntax-properties.
  gnu: system: Improve location of some configuration warnings.

 gnu/bootloader.scm   | 16 +++++++---------
 gnu/system.scm       | 11 ++++++-----
 guix/diagnostics.scm | 38 +++++++++++++++++++++++++++++++++++++-
 3 files changed, 50 insertions(+), 15 deletions(-)

-- 
2.33.1

[bug#51922] [PATCH 1/2] guix: Add syntax to capture arguments' syntax-properties.

[Josselin Poiret via Guix-patches via]

* guix/diagnostics.scm (define-with-syntax-properties): Add it.
---
 guix/diagnostics.scm | 38 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 37 insertions(+), 1 deletion(-)

diff --git a/guix/diagnostics.scm b/guix/diagnostics.scm
index 6a792febd4..337a73c1a2 100644
--- a/guix/diagnostics.scm
+++ b/guix/diagnostics.scm
@@ -54,7 +54,9 @@ (define-module (guix diagnostics)
             condition-fix-hint
 
             guix-warning-port
-            program-name))
+            program-name
+
+            define-with-syntax-properties))
 
 ;;; Commentary:
 ;;;
@@ -331,3 +333,37 @@ (define guix-warning-port
 (define program-name
   ;; Name of the command-line program currently executing, or #f.
   (make-parameter #f))
+
+\f
+(define-syntax define-with-syntax-properties
+  (lambda (x)
+    "Define BINDING to be a syntax form replacing each VALUE-IDENTIFIER and
+SYNTAX-PROPERTIES-IDENTIFIER in body by the syntax and syntax-properties,
+respectively, of each ensuing syntax object."
+    (syntax-case x ()
+      ((_ (binding (value-identifier syntax-properties-identifier)
+                   ...)
+          body ...)
+       (and (and-map identifier? #'(value-identifier ...))
+            (and-map identifier? #'(syntax-properties-identifier ...)))
+       #'(define-syntax binding
+           (lambda (y)
+             (with-ellipsis :::
+               (syntax-case y ()
+                 ((_ value-identifier ...)
+                  (with-syntax ((syntax-properties-identifier
+                                 #`'#,(datum->syntax y
+                                                     (syntax-source
+                                                      #'value-identifier)))
+                                ...)
+                    #'(begin body ...)))
+                 (_
+                  (syntax-violation #f (format #f
+                                               "Expected (~a~{ ~a~})"
+                                               'binding
+                                               '(value-identifier ...))
+                                    y)))))))
+      (_
+       (syntax-violation #f "Expected a definition of the form \
+(define-with-syntax-properties (binding (value syntax-properties) \
+...) body ...)" x)))))
-- 
2.33.1

[bug#51922] [PATCH 2/2] gnu: system: Improve location of some configuration warnings.

[Josselin Poiret via Guix-patches via]

* gnu/bootloader.scm (%warn-target-field-deprecation): Remove it.
* gnu/bootloader.scm (warn-target-field-deprecation): Use
define-with-syntax-properties.
* gnu/system.scm (ensure-setuid-program-list): Ditto.  Also rename the
'location' variable to 'properties'.
---
 gnu/bootloader.scm | 16 +++++++---------
 gnu/system.scm     | 11 ++++++-----
 2 files changed, 13 insertions(+), 14 deletions(-)

diff --git a/gnu/bootloader.scm b/gnu/bootloader.scm
index d1c72c0c85..9cf5457873 100644
--- a/gnu/bootloader.scm
+++ b/gnu/bootloader.scm
@@ -183,8 +183,13 @@ (define-record-type* <bootloader>
 ;; The <bootloader-configuration> record contains bootloader independant
 ;; configuration used to fill bootloader configuration file.
 
-(define-syntax-rule (warn-target-field-deprecation value)
-  (%warn-target-field-deprecation value (current-source-location)))
+(define-with-syntax-properties (warn-target-field-deprecation
+                                (value properties))
+  (when value
+    (warning (source-properties->location properties)
+             (G_ "the 'target' field is deprecated, please use 'targets' \
+instead~%")))
+  value)
 
 (define-record-type* <bootloader-configuration>
   bootloader-configuration make-bootloader-configuration
@@ -213,13 +218,6 @@ (define-record-type* <bootloader-configuration>
   (serial-speed       bootloader-configuration-serial-speed ;integer | #f
                       (default #f)))
 
-(define (%warn-target-field-deprecation value location)
-  (when value
-    (warning (source-properties->location location)
-             (G_ "the 'target' field is deprecated, please use 'targets' \
-instead~%")))
-  value)
-
 (define-deprecated (bootloader-configuration-target config)
   bootloader-configuration-targets
   (%bootloader-configuration-target config))
diff --git a/gnu/system.scm b/gnu/system.scm
index 03fb55db15..9de0f79b44 100644
--- a/gnu/system.scm
+++ b/gnu/system.scm
@@ -1107,16 +1107,17 @@ (define (operating-system-environment-variables os)
     ;; TODO: Remove when glibc@2.23 is long gone.
     ("GUIX_LOCPATH" . "/run/current-system/locale")))
 
-(define-syntax-rule (ensure-setuid-program-list lst)
-  "Ensure LST is a list of <setuid-program> records and warn otherwise."
-  (%ensure-setuid-program-list lst (current-source-location)))
+;; Ensure LST is a list of <setuid-program> records and warn otherwise.
+(define-with-syntax-properties (ensure-setuid-program-list (lst properties))
+  (%ensure-setuid-program-list lst properties))
 
-(define (%ensure-setuid-program-list lst location)
+;; We want to be able to use defines, so define a procedure.
+(define (%ensure-setuid-program-list lst properties)
   (define warned? #f)
 
   (define (warn-once)
     (unless warned?
-      (warning (source-properties->location location)
+      (warning (source-properties->location properties)
                (G_ "representing setuid programs with file-like objects is \
 deprecated; use 'setuid-program' instead~%"))
       (set! warned? #t)))
-- 
2.33.1

bug#51922: [PATCH 0/2] Improve the reported location of configuration warnings

[Ludovic Courtès]

Hi,

Josselin Poiret <dev@jpoiret.xyz> skribis:

> While working on the swap-space patch, I noticed that currently,
> warnings about deprecated fields in guix records use the location of
> the record definition macro, rather than of the invalid value.  For
> some records such as 'operating-system', this makes it rather user
> unfriendly and confusing.
>
> This patchset first adds the syntax 'define-with-syntax-properties',
> which helps avoid boilerplate code to define sanitizers with proper
> location reporting.  I put it in guix/diagnostics.scm as I thought
> this was the place that was most likely to be use-module'd for warning
> messages, as this is quite tied to that use.  The second patch makes
> use of this new helper to update two warnings: the one about 'target'
> to 'targets' in bootloader.scm, and the one about setuid-programs.  In
> both cases, a `guix system reconfigure` now reports the exact location
> of the incriminating values, rather than of the
> 'bootloader-configuration' or 'operating-system' lines respectively.

Neat!  That’s a much welcome improvement.

> The approach I've taken for the helper was to make something simple
> and general enough for most uses.  It should correctly report syntax
> errors with custom errors messages rather than the generic "source
> expression failed to match any pattern ...".  Note although that it
> isn't possible to do any defines in the body of the macro, as this
> doesn't use any lambda-like macros (see the second case for an
> example).

I think that’s OK, that’s a reasonable approach.

> I can see two drawbacks to this macro:
> 1) This macro will not help you write expand-time checkers.  This
> would introduce too much complexity, and I'm not sure the end-user
> would notice a significant change.
> 2) It doesn't deconstruct values such as lists to get the individual
> list values's properties.  This would also introduce too much
> complexity (eg. checking if the list is literal, deconstructing it,
> and falling back to a generic source location if the list is only
> available at eval-time).

Again, I think that’s fine: this macro solves what it was designed to
address, and if we need something fancier, we can think about it later.

>   guix: Add syntax to capture arguments' syntax-properties.
>   gnu: system: Improve location of some configuration warnings.

I tweaked the first commit log and applied it.

Thank you!

Ludo’.