[PATCH] ice-9: exceptions: Properly format the error message.

  • Open
  • quality assurance status badge
Details
4 participants
  • Bengt Richter
  • Maxim Cournoyer
  • Adriano Peluso
  • Ricardo Wurmus
Owner
unassigned
Submitted by
Maxim Cournoyer
Severity
normal
M
M
Maxim Cournoyer wrote on 19 Jun 2020 23:33
(address . bug-guile@gnu.org)
87eeqad9m9.fsf@gmail.com
Hello,

I had this problem in Guix where 'guix deploy my-config.scm' would
unhelpfully report an error like:

guix deploy: error: failed to deploy my-host: ~A: ~S

Digging a bit, I could reproduce at the REPL with:

Toggle snippet (8 lines)
(guard (c ((message-condition? c)
(format #t "error: ~a~%" (condition-message c))))
;; This is what (canonicalize-path "/do/not/exist) ends up doing:
(throw 'system-error "canonicalize-path" "~A" '("No such file or directory")))

--> error: ~A

It seems our native -> srfi-34 style exception converter should populate
the message field with a formatted message, given that's what happens to
present errors as explained in libguile/error.c:

When an error is reported,\n
"these are replaced by formatting the corresponding members of\n"
"@var{args}: @code{~A} (was @code{%s} in older versions of\n"
"Guile) formats using @code{display} and @code{~S}

I'm not sure about the second ~S that appeared in the Guix output;
possibly the exception got re-thrown and suffered from a slightly
different conversion problem?

Anyway, the simple patch attached should fix the "~A" exception message.

Thank you,

Maxim
From adaa2f66fec7684e9e65491158afc5923613e3da Mon Sep 17 00:00:00 2001
From: Maxim Cournoyer <maxim.cournoyer@gmail.com>
Date: Fri, 19 Jun 2020 14:30:05 -0400
Subject: [PATCH] ice-9: exceptions: Properly format the error message.

Before this change, native exceptions such as system errors caught with
srfi-34's `guard' would be converted to an exception with its message
unhelpfully set to "~A".

Thus, apply the message arguments to the message format to derive a human
readable exception message.

* module/ice-9/exceptions.scm (guile-common-exceptions): Format the
message string using its arguments.
---
module/ice-9/exceptions.scm | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

Toggle diff (15 lines)
diff --git a/module/ice-9/exceptions.scm b/module/ice-9/exceptions.scm
index 143e7aa3e..c990790e9 100644
--- a/module/ice-9/exceptions.scm
+++ b/module/ice-9/exceptions.scm
@@ -189,7 +189,7 @@
((subr msg margs . _)
(make-exception
(make-exception-with-origin subr)
- (make-exception-with-message msg)
+ (make-exception-with-message (apply format #f msg margs))
(make-exception-with-irritants margs)))
(_ (make-exception-with-irritants args)))
args))
--
2.26.2
M
M
maxim.cournoyer wrote on 20 Jun 2020 07:46
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)(address . 41956@debbugs.gnu.org)
87wo42mgre.fsf@hurd.i-did-not-set--mail-host-address--so-tickle-me
Maxim Cournoyer <maxim.cournoyer@gmail.com> writes:

Toggle quote (16 lines)
> Hello,
>
> I had this problem in Guix where 'guix deploy my-config.scm' would
> unhelpfully report an error like:
>
> guix deploy: error: failed to deploy my-host: ~A: ~S
>
> Digging a bit, I could reproduce at the REPL with:
>
> (guard (c ((message-condition? c)
> (format #t "error: ~a~%" (condition-message c))))
> ;; This is what (canonicalize-path "/do/not/exist) ends up doing:
> (throw 'system-error "canonicalize-path" "~A" '("No such file or directory")))
>
> --> error: ~A

[...]

Unfortunately the previous patch breaks the tests, with errors like:

ERROR: bytevectors.test: Datum Syntax: incorrect prefix - arguments: ((wrong-type-arg "apply" "Apply to non-list: ~S" (#\i) (#\i)))

I'm out of ideas for now, I last tried:

Toggle snippet (15 lines)
modified module/ice-9/exceptions.scm
@@ -189,7 +189,10 @@
((subr msg margs . _)
(make-exception
(make-exception-with-origin subr)
- (make-exception-with-message msg)
+ (let ((msg (if (null? margs)
+ msg
+ (apply simple-format #f msg margs))))
+ (make-exception-with-message msg))
(make-exception-with-irritants margs)))
(_ (make-exception-with-irritants args)))
args))

To the same effect.

Maxim
B
B
Bengt Richter wrote on 20 Jun 2020 20:33
(address . maxim.cournoyer@gmail.com)(address . 41956@debbugs.gnu.org)
20200620183334.GA9490@LionPure
Hi Maxim,

tl;dr:
Does module/ice-9/exceptions.scm use the default format?
Maybe (use-modules (ice-9) format) will help get to the next bug ?? :)

On +2020-06-20 01:46:13 -0400, maxim.cournoyer@gmail.com wrote:
Toggle quote (49 lines)
> Maxim Cournoyer <maxim.cournoyer@gmail.com> writes:
>
> > Hello,
> >
> > I had this problem in Guix where 'guix deploy my-config.scm' would
> > unhelpfully report an error like:
> >
> > guix deploy: error: failed to deploy my-host: ~A: ~S
> >
> > Digging a bit, I could reproduce at the REPL with:
> >
> > (guard (c ((message-condition? c)
> > (format #t "error: ~a~%" (condition-message c))))
> > ;; This is what (canonicalize-path "/do/not/exist) ends up doing:
> > (throw 'system-error "canonicalize-path" "~A" '("No such file or directory")))
> >
> > --> error: ~A
>
> [...]
>
> Unfortunately the previous patch breaks the tests, with errors like:
>
> ERROR: bytevectors.test: Datum Syntax: incorrect prefix - arguments: ((wrong-type-arg "apply" "Apply to non-list: ~S" (#\i) (#\i)))
>
> I'm out of ideas for now, I last tried:
>
> --8<---------------cut here---------------start------------->8---
> modified module/ice-9/exceptions.scm
> @@ -189,7 +189,10 @@
> ((subr msg margs . _)
> (make-exception
> (make-exception-with-origin subr)
> - (make-exception-with-message msg)
> + (let ((msg (if (null? margs)
> + msg
> + (apply simple-format #f msg margs))))
> + (make-exception-with-message msg))
> (make-exception-with-irritants margs)))
> (_ (make-exception-with-irritants args)))
> args))
> --8<---------------cut here---------------end--------------->8---
>
> To the same effect.
>
> Maxim
>
>
>

HTH
--
Regards,
Bengt Richter
M
M
Maxim Cournoyer wrote on 21 Jun 2020 05:49
(name . Bengt Richter)(address . bokr@bokr.com)(address . 41956@debbugs.gnu.org)
87r1u9m62o.fsf@gmail.com
Hello Bengt!

Bengt Richter <bokr@bokr.com> writes:

Toggle quote (6 lines)
> Hi Maxim,
>
> tl;dr:
> Does module/ice-9/exceptions.scm use the default format?
> Maybe (use-modules (ice-9) format) will help get to the next bug ?? :)

Thanks for suggesting! I tried but got the same result.

I'm now testing a slightly different version:

@@ -189,7 +189,10 @@
((subr msg margs . _)
(make-exception
(make-exception-with-origin subr)
- (make-exception-with-message msg)
+ (let ((msg (if (list? margs)
+ (apply simple-format #f msg margs)
+ msg)))
+ (make-exception-with-message msg))
(make-exception-with-irritants margs)))
(_ (make-exception-with-irritants args)))
args))

Maxim
R
R
Ricardo Wurmus wrote on 25 Jun 2020 12:04
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)
87bll7qx5g.fsf@elephly.net
Hi Maxim,

here’s what I did in the REPL:

Toggle snippet (26 lines)
scheme@(guile-user)> ,m (ice-9 exceptions)
scheme@(ice-9 exceptions)> (define (my/guile-system-error-converter key args)
(apply (case-lambda
((subr msg-args msg errno . rest)
;; XXX TODO we should return a more specific error
;; (usually an I/O error) as expected by R6RS programs.
;; Unfortunately this often requires the 'filename' (or
;; other?) which is not currently provided by the native
;; Guile exceptions.
(make-exception
(make-external-error)
(make-exception-with-origin subr)
(apply make-exception-with-message msg)
(make-exception-with-irritants msg-args)))
(_ (guile-external-error-converter key args)))
args))
scheme@(ice-9 exceptions)> (set! guile-exception-converters (acons 'system-error my/guile-system-error-converter guile-exception-converters))
scheme@(ice-9 exceptions)> ,m (guile-user)
scheme@(guile-user)> (guard (c ((message-condition? c)
(format #t "message: ~a~%" (condition-message c))))
(canonicalize-path "/doesntexist"))
message: No such file or directory
$11 = #t
scheme@(guile-user)>

--
Ricardo
B
B
Bengt Richter wrote on 25 Jun 2020 18:33
(name . Ricardo Wurmus)(address . rekado@elephly.net)
20200625163341.GA4622@LionPure
Attachment: file
M
M
Maxim Cournoyer wrote on 28 Jun 2020 06:17
(name . Ricardo Wurmus)(address . rekado@elephly.net)
87y2o7lt8f.fsf@gmail.com
Hello Ricardo!

Ricardo Wurmus <rekado@elephly.net> writes:

Toggle quote (29 lines)
> Hi Maxim,
>
> here’s what I did in the REPL:
>
> scheme@(guile-user)> ,m (ice-9 exceptions)
> scheme@(ice-9 exceptions)> (define (my/guile-system-error-converter key args)
> (apply (case-lambda
> ((subr msg-args msg errno . rest)
> ;; XXX TODO we should return a more specific error
> ;; (usually an I/O error) as expected by R6RS programs.
> ;; Unfortunately this often requires the 'filename' (or
> ;; other?) which is not currently provided by the native
> ;; Guile exceptions.
> (make-exception
> (make-external-error)
> (make-exception-with-origin subr)
> (apply make-exception-with-message msg)
> (make-exception-with-irritants msg-args)))
> (_ (guile-external-error-converter key args)))
> args))
> scheme@(ice-9 exceptions)> (set! guile-exception-converters (acons 'system-error my/guile-system-error-converter guile-exception-converters))
> scheme@(ice-9 exceptions)> ,m (guile-user)
> scheme@(guile-user)> (guard (c ((message-condition? c)
> (format #t "message: ~a~%" (condition-message c))))
> (canonicalize-path "/doesntexist"))
> message: No such file or directory
> $11 = #t
> scheme@(guile-user)>

I've tested that this indeed works, although I don't quite understand
how?

This brings embeds the definition of `guile-common-exceptions' into
`guile-system-error-converter', with a single change:

(make-exception-with-message msg) --> (apply make-exception-with-message
msg msg-args)

What is the magic I fail to see?

Is this fix proper to be merged into the original
guile-common-exceptions procedure?

Thank you!

Maxim
M
M
Maxim Cournoyer wrote on 28 Jun 2020 06:25
(name . Bengt Richter)(address . bokr@bokr.com)
87tuyvlsuf.fsf@gmail.com
Hello Bengt,

Bengt Richter <bokr@bokr.com> writes:


[...]

Toggle quote (18 lines)
> What do you think of using (ice-9 match) to make a universal throwage-formatter,
> with the idea of making readable top level code for how exceptions become messages on screen?
>
> I started a hack to explore the (throw 'whatever any ...) space, beginning like
>
> (use-modules (ice-9 match))
> (define (make-exception-message key rest)
> (begin
> (let*((l (cons key rest)))
> (match l
> (('system-error subr message args data ...)
> ;; e.g. thrown with key 'system-error: ("open-fdes" "~A" ("No such file or directory") (2))
> (format #f (string-append "match-1: subr ~s threw '~s " message " sterror: ~s") subr key args (strerror (car (car data)))))
>
> (('signal any ...)
> ;; not yet implemented
> (format #f "match-2: <NYI:unix signal no> any: ~s" any))

Are you proposing to refactor (ice-9 exceptions) so that it's simpler to
follow a condition message is formed for a given exception type?

Maxim
R
R
Ricardo Wurmus wrote on 28 Jun 2020 06:31
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)
875zbbq09b.fsf@elephly.net
Hi Maxim,

Toggle quote (7 lines)
>> here’s what I did in the REPL:
>>
>> scheme@(guile-user)> ,m (ice-9 exceptions)
>> scheme@(ice-9 exceptions)> (define (my/guile-system-error-converter key args)
>> (apply (case-lambda
>> ((subr msg-args msg errno . rest)

Here I changed the order: “msg-args” appears before “msg”. I don’t know
why the converter that’s currently in Guile assumes that the message
comes first.

Toggle quote (2 lines)
>> scheme@(ice-9 exceptions)> (set! guile-exception-converters (acons 'system-error my/guile-system-error-converter guile-exception-converters))

guile-exception-converters is a lookup table in (ice-9 exceptions). It
associates error keys with converter procedures. Since
canonicalize-path throws a 'system-error I chose to only update the
'system-error association. I didn’t want to affect all the other
converter procedures that end up using the common converter; maybe they
should be affected — I don’t know because I don’t have any test cases
other than canonicalize-path.

Toggle quote (8 lines)
> This brings embeds the definition of `guile-common-exceptions' into
> `guile-system-error-converter', with a single change:
>
> (make-exception-with-message msg) --> (apply make-exception-with-message
> msg msg-args)
>
> What is the magic I fail to see?

I cannot parse your sentence, so I’m not sure what you mean.

Toggle quote (3 lines)
> Is this fix proper to be merged into the original
> guile-common-exceptions procedure?

I think we should add tests of different exceptions with different keys
to be sure that modifying the common handler doesn’t have any unexpected
side-effects.

--
Ricardo
B
B
Bengt Richter wrote on 28 Jun 2020 20:23
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)
20200628182313.GA2809@LionPure
Attachment: file
A
A
Adriano Peluso wrote on 2 Jun 2021 09:32
is this still current ?
(address . 41956@debbugs.gnu.org)
7fd3319aad78a597b9d8855ede45849f6d562e05.camel@riseup.net
I understand that a overhaul of the exceptions stack intervened while
this bug was open

Is this still current ?
M
M
Maxim Cournoyer wrote on 10 Nov 2023 05:17
(name . Adriano Peluso)(address . randomlooser@riseup.net)
875y2afdhz.fsf@gmail.com
Hi,

Adriano Peluso <randomlooser@riseup.net> writes:

Toggle quote (5 lines)
> I understand that a overhaul of the exceptions stack intervened while
> this bug was open
>
> Is this still current ?

Yes. The affected code hasn't been touched since 2019. To recall, the
reproducer is this:

Toggle snippet (7 lines)
(use-modules (srfi srfi-34)
(srfi srfi-35))
(guard (c ((message-condition? c)
(format #t "error: ~a~%" (condition-message c))))
(canonicalize-path "/no/such/path"))

or this:

Toggle snippet (10 lines)
(use-modules (srfi srfi-34)
(srfi srfi-35))
(guard (c ((message-condition? c)
(format #t "error: ~a~%" (condition-message c))))
;; This is what (canonicalize-path "/do/not/exist) ends up doing:
(throw 'system-error "canonicalize-path" "~A" '("No such file or directory")))

error: ~A

--
Thanks,
Maxim
?
Your comment

Commenting via the web interface is currently disabled.

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

To respond to this issue using the mumi CLI, first switch to it
mumi current 41956
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