better error messages through assertions

better error messages through assertions

[Ricardo Wurmus]

Hi Guix,

today on IRC someone reported an ugly error message when reconfiguring
their system:

--8<---------------cut here---------------start------------->8---
Backtrace:
          18 (primitive-load "/home/me/.config/guix/current/bin/…")
In guix/ui.scm:
   2209:7 17 (run-guix . _)
  2172:10 16 (run-guix-command _ . _)
In ice-9/boot-9.scm:
  1752:10 15 (with-exception-handler _ _ #:unwind? _ # _)
In guix/status.scm:
    822:3 14 (_)
    802:4 13 (call-with-status-report _ _)
In guix/scripts/system.scm:
   1256:4 12 (_)
In ice-9/boot-9.scm:
  1752:10 11 (with-exception-handler _ _ #:unwind? _ # _)
In guix/store.scm:
   658:37 10 (thunk)
   1320:8  9 (call-with-build-handler #<procedure 7fecaf8570c0 at g…> …)
  2123:24  8 (run-with-store #<store-connection 256.99 7fecb75c7230> …)
In guix/scripts/system.scm:
    827:2  7 (_ _)
    703:7  6 (_ #<store-connection 256.99 7fecb75c7230>)
In gnu/system.scm:
  1227:19  5 (operating-system-derivation _)
In gnu/services.scm:
   1091:6  4 (instantiate-missing-services _)
In srfi/srfi-1.scm:
   460:18  3 (fold #<procedure 7fecb73c0960 at gnu/services.scm:109…> …)
In gnu/services.scm:
  1092:27  2 (_ (#<<service> type: #<service-type gdm 7fecbd17f6…> …) …)
In ice-9/boot-9.scm:
  1685:16  1 (raise-exception _ #:continuable? _)
  1685:16  0 (raise-exception _ #:continuable? _)

ice-9/boot-9.scm:1685:16: In procedure raise-exception:
In procedure struct-vtable: Wrong type argument in position 1 (expecting struct):
--8<---------------cut here---------------end--------------->8---

As you can probably tell easily by looking at this message, the
“service” field of the operating system configuration looked something
like this:

  (services (append (list a b c %desktop-services) #;oops))

instead of this

  (services (append (list a b c) %desktop-services))

This is because INSTANTIATE-MISSING-SERVICES — and FOLD-SERVICES, and
many more — assumes that it is only passed a plain list of services.  It
then proceeds to call SERVICE-KIND on what may or may not be a service.

I think we should add simple type checks, something like this:

  (define (listof pred)
    (lambda (thing)
     (and (list? thing) (every pred thing))))
  …
  (define (assert-type type-check thing message)
    (or (false-if-exception (type-check thing))
        (report-error (G_ "type error: …\n" message))))

  ;; Use ASSERT-TYPE in an example procedure.
  (define (do-something-with-services services)
    (assert-type (listof service?) services
                 "SERVICES must be a list of <service> values.")

    ;; Do things…
    (map service-kind services))

What do you think?  There are many different ways of implementing this
(a new variant of DEFINE that also accepts a type declaration, an assert
like above, a fancier assert that composes a helpful error message by
itself, a separate type declaration that is looked up only when the
corresponding procedure is called in a certain context, etc), but I’d
first like to know if there is consensus that we want something like
this.

-- 
Ricardo

Re: better error messages through assertions

[Maxime Devos]

[-- Attachment #1: Type: text/plain, Size: 627 bytes --]

Ricardo Wurmus schreef op ma 14-02-2022 om 23:32 [+0100]:
> I think we should add simple type checks, something like this:
> [...]
> 
> What do you think?  There are many different ways of implementing this
> (a new variant of DEFINE that also accepts a type declaration, an assert
> like above, a fancier assert that composes a helpful error message by
> itself, a separate type declaration that is looked up only when the
> corresponding procedure is called in a certain context, etc), but I’d
> first like to know if there is consensus that we want something like
> this.

Seems nice.

Greetings,
Maxime.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

Re: better error messages through assertions

[Philip McGrath]

Hi,

On 2/14/22 17:32, Ricardo Wurmus wrote:
> As you can probably tell easily by looking at this message, the
> “service” field of the operating system configuration looked something
> like this:
> 
>    (services (append (list a b c %desktop-services) #;oops))
> 
> instead of this
> 
>    (services (append (list a b c) %desktop-services))
> 
> This is because INSTANTIATE-MISSING-SERVICES — and FOLD-SERVICES, and
> many more — assumes that it is only passed a plain list of services.  It
> then proceeds to call SERVICE-KIND on what may or may not be a service.
> 
> I think we should add simple type checks, something like this:
> 
>    (define (listof pred)
>      (lambda (thing)
>       (and (list? thing) (every pred thing))))
>    …
>    (define (assert-type type-check thing message)
>      (or (false-if-exception (type-check thing))
>          (report-error (G_ "type error: …\n" message))))
> 
>    ;; Use ASSERT-TYPE in an example procedure.
>    (define (do-something-with-services services)
>      (assert-type (listof service?) services
>                   "SERVICES must be a list of <service> values.")
> 
>      ;; Do things…
>      (map service-kind services))
> 
> What do you think?  There are many different ways of implementing this
> (a new variant of DEFINE that also accepts a type declaration, an assert
> like above, a fancier assert that composes a helpful error message by
> itself, a separate type declaration that is looked up only when the
> corresponding procedure is called in a certain context, etc), but I’d
> first like to know if there is consensus that we want something like
> this.
> 

As a Guix user and contributor, I would love better error messages.

As a Racketeer, I think you're half way to reinventing contracts.

In particular, since the operating system services field is thunked, 
this example already points to the desirability of higher-order contracts.

Using "contracts" need not initially involve all the bells and whistles 
of Racket's contract library. For example, higher-order contracts can be 
implemented with `lambda` in the absence of special runtime support for 
chaperones and impersonators, as illustrated in [1]. But the Racket 
community has accumulated a great deal of both theoretical insight and 
implementation experience in many subtle corners of this problem: I hope 
Guix can build on that experience.

-Philip

[1]: https://docs.racket-lang.org/guide/Building_New_Contracts.html

Re: better error messages through assertions

[Ricardo Wurmus]

Philip McGrath <philip@philipmcgrath.com> writes:

> As a Racketeer, I think you're half way to reinventing contracts.

Yes, I was in fact thinking of contracts, but shied away from mentioning
them :)  The reason is that I think we can cover a lot of distance with
just a few simple assertions to avoid plowing ahead on bad arguments.

-- 
Ricardo

Re: better error messages through assertions

[Ludovic Courtès]

Hi!

Ricardo Wurmus <rekado@elephly.net> skribis:

> Philip McGrath <philip@philipmcgrath.com> writes:
>
>> As a Racketeer, I think you're half way to reinventing contracts.
>
> Yes, I was in fact thinking of contracts, but shied away from mentioning
> them :)  The reason is that I think we can cover a lot of distance with
> just a few simple assertions to avoid plowing ahead on bad arguments.

I’d very much like to have contracts.

For record types, we have “sanitizers” right now, which we could use to
insert procedural type checks.  It wouldn’t be as nice as proper
declarative contracts though, unless we arrange to make them look like
contracts.

Thoughts?

Ludo’.

Re: better error messages through assertions

[Philip McGrath]

[-- Attachment #1: Type: text/plain, Size: 8341 bytes --]

Hi,

On Monday, February 28, 2022 7:59:02 AM EST Ludovic Courtès wrote:
> Hi!
> 
> Ricardo Wurmus <rekado@elephly.net> skribis:
> > Philip McGrath <philip@philipmcgrath.com> writes:
> >> As a Racketeer, I think you're half way to reinventing contracts.
> > 
> > Yes, I was in fact thinking of contracts, but shied away from mentioning
> > them :)  The reason is that I think we can cover a lot of distance with
> > just a few simple assertions to avoid plowing ahead on bad arguments.
> 
> I’d very much like to have contracts.
> 
> For record types, we have “sanitizers” right now, which we could use to
> insert procedural type checks.  It wouldn’t be as nice as proper
> declarative contracts though, unless we arrange to make them look like
> contracts.
> 

I'm going to try to sketch what I think is a minimal API for contracts.

Racket's state-of-the-art contract system has many features and nuances. I *do
not* think anyone should try to implement them all in one fell swoop. I'm
hoping there's a way to implement your simple assertions with only a modest
amount of overhead that will provide the right base on which to grow the rest
of a contract system. In the short term, the advantage over:

>     (assert-type (listof service?) services
>                  "SERVICES must be a list of <service> values.")

is that you don't have to write error messages by hand.

You need two types of values:

 1. Contracts, recognized by `contract?`; and
 2. Blame objects, recognized by `blame?`.

In simplest terms, a blame object holds context information for error
reporting. Ultimately, it is used to raise an exception with
`raise-blame-error`[1]: this can support e.g. substituting "expected"/"given"
with "promised"/"produced" in error messages based on the context. A contract
combinator will typically give its sub-contract a blame object extended using
`blame-add-context`[2].

The primitive operation on a contract is `get/build-late-neg-projection`[3]:
by skipping the decades experimenting with other APIs that turned out to
perform less well, you have the opportunity to give this function a better
name. In pseudo-code, `get/build-late-neg-projection` has the contract:

    (-> contract? (-> blame? (-> val neg-party val*)))

That is, given a contract, it returns a function that, when given a blame
object, returns a function that enforces the contract. The enforcer function
accepts the value being checked and the "negative party" and either raises an
exception or returns the checked value, possibly wrapped to enforce higher-
order checks.

(The "negative party" is simultaneously deeply important and something you
could ignore to start with. Simply put, if a module `(server)` provides a
contracted value that is used by a module `(client foo)`, the "negative party"
represents `(client foo)`, e.g. via a source location. It is the only part
that would be different between `(client foo)` and `(client bar)`, so this API
lets everything else be shared.)

The multiple levels of `lambda` let contract combinators do some of their work
ahead of time, improving performance when the contracted value is used.

As a motivating example, here is a very basic definition of the `listof`
contract combinator. Notice in particular:

  * Predicate functions and certain kinds of primitive data are implicitly
    promoted to contracts. This wasn't original to Racket's contract system,
    but the ergonomics are so compelling you might want it from the beginning.

  * Forms like `define/contract`, `struct/contract`, and `contract-out` (which
    is preferred) attach contracts to values. Guix might want
    `define-record-type*/contract`. There is no public API for constructing
    blame objects.

> #lang racket
> (define (listof elem/c)
>   (let* (;; error if elem/c isn't a contract
>          [elem/c (coerce-contract 'listof elem/c)]
>          [elem-late-neg (get/build-late-neg-projection elem/c)])
>     (make-contract
>      #:name (build-compound-type-name 'listof elem/c)
>      #:late-neg-projection
>      (λ (blame)
>        (define elem-proj+blame
>          (elem-late-neg (blame-add-context blame "an element of")))
>        (λ (val neg-party)
>          (unless (list? val)
>            (raise-blame-error
>             blame val #:missing-party neg-party
>             `(expected: "a list" given: "~e")
>             val))
>          (for-each (λ (elem)
>                      (elem-proj+blame elem neg-party))
>                    val)
>          val)))))
> 
> (define/contract numbers
>   (listof number?)
>   '(1 2 2.5 3))
> 
> (define/contract fruits
>   (listof (or/c 'apple 'banana))
>   (list (λ (x) (x x))))

The resulting error message:

> fruits: broke its own contract
>   promised: (or/c (quote apple) (quote banana))
>   produced: #<procedure:...of-contract-mvp.rkt:27:8>
>   in: an element of
>       (listof (or/c 'apple 'banana))
>   contract from: (definition fruits)
>   blaming: (definition fruits)
>    (assuming the contract is correct)
>   at: /tmp/listof-contract-mvp.rkt:25:17

The most important optimization I left out is providing a specialized first-
order check for use by `or/c`. But even that, in principle, wouldn't have to
be part of an initial implementation.

And, again, you should only need even this much work for a combinator or a
higher-order contract. 

Do note that this is all fresh off the top of my head in about an hour, and
I've only incidentally hacked on the internals of the Racket contract system!
If using contracts in Guix is at all appealing, I'd strongly suggest asking
for advice at <https://racket.discourse.group> before starting to write code.
A few other resources:

  * "Inside Racket Seminar: Robby Findler on Contracts"
    <https://www.youtube.com/watch?v=eUsDKbKtrLs>
    A two-hour video walkthrough/Q&A about the implementation of Racket's
    contract system. Probably the most comprehensive resource that exists on
    how to implement contracts, as opposed to interesting things to do with
    contracts once you've got them.

  * Robby Findler, "Behavioral Software Contracts" (ICFP 2014)
    https://www.youtube.com/watch?v=gXTbMPVFP1M
    Wonderful slides and illustrations help to illuminate some of the more
    difficult concepts.
    In particular, I recommend the section @11:10, "Contracts are not Types",
    which explains some very interesting contracts like the one on `dc`[4]
    that take advantage of contracts being checked *at runtime* to enforce
    very nuanced invariants, somewhat like "dependent types" requiring a
    drawing procedure to restore any changes it makes to Cairo's mutable
    state. This would support examples like Arun's: say, checking that no two
    services are trying to listen on the same TCP port.
    The concept of "boundaries" as a design principle is an important lesson
    learned.
    The source code for the slides (which are a program in `#lang slideshow`,
    of course) is at <https://github.com/rfindler/icfp-2014-contracts-talk>.

  * "Oh Lord, Please Don’t Let Contracts Be Misunderstood"
    Dimoulas, New, Findler, & Felleisen (ICFP 2016)
    https://users.cs.northwestern.edu/~robby/pubs/papers/icfp2016-dnff.pdf
    Focused on pragmatics (as opposed to fancy math), with discussion both of
    how a contract system is put together and of some interesting uses.

I would love to have contracts in Guix, even very rudimentary contracts. If
it's something the community more generally would be interested in, I'd be
glad to help as much as I can.

-Philip

[1]: https://docs.racket-lang.org/reference/Building_New_Contract_Combinators.html#%28def._%28%28lib._racket%2Fcontract%2Fprivate%2Fblame..rkt%29._raise-blame-error%29%29
[2]: https://docs.racket-lang.org/reference/Building_New_Contract_Combinators.html#%28def._%28%28lib._racket%2Fcontract%2Fprivate%2Fblame..rkt%29._blame-add-context%29%29
[2]: https://docs.racket-lang.org/reference/contract-utilities.html#%28def._%28%28lib._racket%2Fcontract%2Fprivate%2Fguts..rkt%29._get%2Fbuild-late-neg-projection%29%29
[4]: https://docs.racket-lang.org/pict/Basic_Pict_Constructors.html#%28def._%28%28lib._pict%2Fmain..rkt%29._dc%29%29

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: better error messages through assertions

[Ludovic Courtès]

Hi Philip,

Philip McGrath <philip@philipmcgrath.com> skribis:

> Racket's state-of-the-art contract system has many features and nuances. I *do
> not* think anyone should try to implement them all in one fell swoop. I'm
> hoping there's a way to implement your simple assertions with only a modest
> amount of overhead that will provide the right base on which to grow the rest
> of a contract system. In the short term, the advantage over:
>
>>     (assert-type (listof service?) services
>>                  "SERVICES must be a list of <service> values.")
>
> is that you don't have to write error messages by hand.
>
> You need two types of values:
>
>  1. Contracts, recognized by `contract?`; and
>  2. Blame objects, recognized by `blame?`.

[...]

Thanks for the explanation and references!  I had briefly looked at
Racket’s contract API in the past but your message gave a clearer view
of how this all fits together.

> I would love to have contracts in Guix, even very rudimentary contracts. If
> it's something the community more generally would be interested in, I'd be
> glad to help as much as I can.

It’d be great to benefit from your expertise here.  Like you wrote, I
think we should start with a simple contract system, certainly simpler
than Racket’s, and build from there.

If you’re willing and able to spend time prototyping this, that’s great.
:-)

Thanks,
Ludo’.

Re: better error messages through assertions

[Philip McGrath]

Hi,

On 3/7/22 05:13, Ludovic Courtès wrote:
> Hi Philip,
> 
> Philip McGrath <philip@philipmcgrath.com> skribis:
> 
>> Racket's state-of-the-art contract system has many features and nuances. I *do
>> not* think anyone should try to implement them all in one fell swoop. I'm
>> hoping there's a way to implement your simple assertions with only a modest
>> amount of overhead that will provide the right base on which to grow the rest
>> of a contract system. In the short term, the advantage over:
>>
>>>      (assert-type (listof service?) services
>>>                   "SERVICES must be a list of <service> values.")
>>
>> is that you don't have to write error messages by hand.
>>
>> You need two types of values:
>>
>>   1. Contracts, recognized by `contract?`; and
>>   2. Blame objects, recognized by `blame?`.
> 
> [...]
> 
> Thanks for the explanation and references!  I had briefly looked at
> Racket’s contract API in the past but your message gave a clearer view
> of how this all fits together.
> 

I'm glad this is something Guix people are interested in!

>> I would love to have contracts in Guix, even very rudimentary contracts. If
>> it's something the community more generally would be interested in, I'd be
>> glad to help as much as I can.
> 
> It’d be great to benefit from your expertise here.  Like you wrote, I
> think we should start with a simple contract system, certainly simpler
> than Racket’s, and build from there.
> 
> If you’re willing and able to spend time prototyping this, that’s great.
> :-)
> 

I'm interested in putting together a prototype.

I've taken my own suggestion and asked the Racket community for more 
advice: 
https://racket.discourse.group/t/advice-on-implementing-a-contract-system/832

To quote the end of my last message there,

> The tl;dr of all that is that `(guix records)` seems to ultimately call for "indy-dependent" contracts[1].
> 
> On the one hand, the distinction between "indy-dependent" `->i`[2] and "lax-dependent" `->d`[3] is exactly the sort of hard-learned lesson that I hope the Guix community can draw from Racket's decades of experience.
> 
> On the other hand, I'm increasingly intrigued by the idea of starting with forms along the lines of `invariant-assertion`[4] and `struct-guard/c`[5] and truly sticking to flat contracts to start with, leaving all the higher-order complexity for another day.

I'm thinking that a reasonable place to start might be to implement a 
`contract->sanitizer` form that would allow using contracts to create 
sanitizers, ideally with no changes to `(guix records)`.

In addition to the questions about contract system design, I realized I 
have a few questions about Guix/Guile that would be relevant when 
starting a prototype.

What is the preferred mechanism for exceptions? I know about:

   * (rnrs exceptions)
   * (ice-9 exceptions)
   * (srfi srfi-34)
   * (srfi srfi-35)

and IIRC I've seen more than one of them used in the Guix codebase.

Likewise, what record system should I use? I think the answer should 
*not* be (guix records): instead, I think (guix records) should 
eventually use (guix contracts). But should I use:

   * (rnrs records syntactic)
   * (rnrs records procedural)
   * (srfi srfi-9)
   * (oop goops)

Of those, I'm most familiar with R6RS records. I know (guix records) is 
implemented on top of (srfi srfi-9), though I vaguely remember some 
discussion about potentially changing that.

Also, I don't know much about how the "abi" aspect of (guix records) 
works and what types of changes there would trigger rebuilds. (Though, 
again, I hope no changes would be needed for the proof-of-concept phase.)

Finally, when I looked again at the example at the top of this thread:

On 2/14/22 17:32, Ricardo Wurmus wrote:
> ice-9/boot-9.scm:1685:16: In procedure raise-exception:
> In procedure struct-vtable: Wrong type argument in position 1 (expecting struct):
> --8<---------------cut here---------------end--------------->8---
> 
> As you can probably tell easily by looking at this message, the
> “service” field of the operating system configuration looked something
> like this:
> 
>   (services (append (list a b c %desktop-services) #;oops))
> 
> instead of this
> 
>   (services (append (list a b c) %desktop-services))
> 
> This is because INSTANTIATE-MISSING-SERVICES — and FOLD-SERVICES, and
> many more — assumes that it is only passed a plain list of services.  It
> then proceeds to call SERVICE-KIND on what may or may not be a service.

Another problem here seems to be the fault of (srfi srfi-9). For example:

```
$ guile
GNU Guile 3.0.8
Copyright (C) 1995-2021 Free Software Foundation, Inc.

Guile comes with ABSOLUTELY NO WARRANTY; for details type `,show w'.
This program is free software, and you are welcome to redistribute it
under certain conditions; type `,show c' for details.

Enter `,help' for help.
scheme@(guile-user)> ,use (srfi srfi-9)
scheme@(guile-user)> (define-record-type container (make-container 
contents) container? (contents container-contents))
scheme@(guile-user)> (container-contents '())
ice-9/boot-9.scm:1685:16: In procedure raise-exception:
In procedure struct-vtable: Wrong type argument in position 1 (expecting 
struct): ()

Entering a new prompt.  Type `,bt' for a backtrace or `,q' to continue.
scheme@(guile-user) [1]> ,bt
In current input:
       3:0  1 (_)
In ice-9/boot-9.scm:
   1685:16  0 (raise-exception _ #:continuable? _)
```

It seems like `container-contents` and other field accessors ought to 
check their arguments with `container?` (or the applicable predicate) 
and not leave error reporting to `struct-vtable`.

Perhaps this could be fixed in the (guix records) layer?

-Philip

[1]: https://www2.ccs.neu.edu/racket/pubs/popl11-dfff.pdf
[2]: 
https://docs.racket-lang.org/reference/function-contracts.html#%28form._%28%28lib._racket%2Fcontract%2Fbase..rkt%29._-~3ei%29%29
[3]: 
https://docs.racket-lang.org/reference/function-contracts.html#%28form._%28%28lib._racket%2Fcontract%2Fbase..rkt%29._-~3ed%29%29
[4]: 
https://docs.racket-lang.org/reference/attaching-contracts-to-values.html#%28form._%28%28lib._racket%2Fcontract%2Fprivate%25in2Fbase..rkt%29._invariant-assertion%29%29
[5]: 
https://docs.racket-lang.org/reference/attaching-contracts-to-values.html#%28form._%28%28lib._racket%2Fcontract%2Fbase..rkt%29._struct-guard%2Fc%29%29

Re: better error messages through assertions

[Ludovic Courtès]

Hi Philip,

Philip McGrath <philip@philipmcgrath.com> skribis:

> I'm thinking that a reasonable place to start might be to implement a
> `contract->sanitizer` form that would allow using contracts to create 
> sanitizers, ideally with no changes to `(guix records)`.

OK.  I’d prefer if people who define record types could directly write:

  (field getter (contract integer/c))

rather than:

  (field getter (sanitizer (contract->sanitizer integer/c)))

But that’s more of a detail.

> What is the preferred mechanism for exceptions?

For Guix code, SRFI-34/35.

> Likewise, what record system should I use?

SRFI-9.

(Perhaps we should put answers to these questions in the “Coding Style”
section of the manual.)

> Also, I don't know much about how the "abi" aspect of (guix records)
> works and what types of changes there would trigger rebuilds. (Though, 
> again, I hope no changes would be needed for the proof-of-concept phase.)

I don’t think you need to worry about that.

> Another problem here seems to be the fault of (srfi srfi-9). For example:

[...]

> scheme@(guile-user)> (container-contents '())
> ice-9/boot-9.scm:1685:16: In procedure raise-exception:
> In procedure struct-vtable: Wrong type argument in position 1
> (expecting struct): ()
>
> Entering a new prompt.  Type `,bt' for a backtrace or `,q' to continue.
> scheme@(guile-user) [1]> ,bt
> In current input:
>       3:0  1 (_)
> In ice-9/boot-9.scm:
>   1685:16  0 (raise-exception _ #:continuable? _)
> ```
>
> It seems like `container-contents` and other field accessors ought to
> check their arguments with `container?` (or the applicable predicate) 
> and not leave error reporting to `struct-vtable`.

SRFI-9 generates the smallest amount of code for the job:

--8<---------------cut here---------------start------------->8---
scheme@(guile-user)> ,use(srfi srfi-9)
scheme@(guile-user)> (define-record-type <foo>
		       (make-foo x)
		       foo?
		       (x foo-x))
scheme@(guile-user)> ,optimize (foo-x '())
$9 = (if (eq? (struct-vtable '()) <foo>)
  (struct-ref '() 0)
  (throw 'wrong-type-arg
         'foo-x
         "Wrong type argument: ~S"
         (list '())
         (list '())))
--8<---------------cut here---------------end--------------->8---

With Guile 3, it might be that adding an extra ‘struct?’ test would have
little effect on performance; we’d need to check.

> Perhaps this could be fixed in the (guix records) layer?

Could be, yes.

Thanks for looking into this!

Ludo’.

Re: better error messages through assertions

[Andy Wingo]

On Wed 30 Mar 2022 11:37, Ludovic Courtès <ludo@gnu.org> writes:

>> scheme@(guile-user)> (container-contents '())
>> ice-9/boot-9.scm:1685:16: In procedure raise-exception:
>> In procedure struct-vtable: Wrong type argument in position 1
> scheme@(guile-user)> ,use(srfi srfi-9)
> scheme@(guile-user)> (define-record-type <foo>
> 		       (make-foo x)
> 		       foo?
> 		       (x foo-x))
> scheme@(guile-user)> ,optimize (foo-x '())
> $9 = (if (eq? (struct-vtable '()) <foo>)
>   (struct-ref '() 0)
>   (throw 'wrong-type-arg
>          'foo-x
>          "Wrong type argument: ~S"
>          (list '())
>          (list '())))
>
> With Guile 3, it might be that adding an extra ‘struct?’ test would have
> little effect on performance; we’d need to check.

Would have no effect.

Incidentally, you might want to use ,optimize-cps;

  scheme@(guile-user)> ,optimize (foo-x '())
  $9 = (if (eq? (struct-vtable '()) <foo>)
    (struct-ref '() 0)
    (throw 'wrong-type-arg
           'foo-x
           "Wrong type argument: ~S"
           (list '())
           (list '())))
  scheme@(guile-user)> ,optimize-cps (foo-x '())
  L0:                                           ;               at <unknown>:15:14
    v0 := self
    L1(...)
  L1:
    receive()
    v1 := const ()                              ; arg           at <unknown>:15:21
    throw throw/value+data[#(wrong-type-arg "struct-vtable" "Wrong type argument in position 1 (expecting struct): ~S")](v1) ;  at <unknown>:15:14

L1(...) means, pass all values to L1.  In this case because there are
varargs on the stack from the procedure call.  L1 parses them with the
receive().  Anyway, here we see that with respect to the immediate '(),
that all the tests folded.  If we instead lift to a procedure:

  scheme@(guile-user)> ,optimize-cps (lambda (x) (foo-x x))
  L0:                                           ;               at <unknown>:16:14
    v0 := self
    L1(...)
  L1:
    receive()
    v1 := current-module()                      ; module        at <unknown>:16:14
    cache-set![0](v1)                           ;               at <unknown>:16:14
    v2 := const-fun L7                          ; _ 
    return v2                                   ;               at <unknown>:16:14

  L7:                                           ;               at <unknown>:16:14
    v3 := self
    L8(...)
  L8:
    v4 := receive(x)                            ; x 
    heap-object?(v4) ? L10() : L38()            ;               at <unknown>:16:26
  L10():
    struct?(v4) ? L11() : L38()                 ;               at <unknown>:16:26
  L38():
    throw throw/value+data[#(wrong-type-arg "struct-vtable" "Wrong type argument in position 1 (expecting struct): ~S")](v4) ;  at <unknown>:16:26
  L11():
    v5 := scm-ref/tag[struct](v4)               ; vtable        at <unknown>:16:26
    v6 := cache-ref[(0 . <foo>)]()              ; cached        at <unknown>:10:20
    heap-object?(v6) ? L19() : L14()            ;               at <unknown>:10:20
  L19():
    L20(v6)                                     ;               at <unknown>:10:20
  L14():
    v7 := cache-ref[0]()                        ; mod           at <unknown>:10:20
    v8 := const <foo>                           ; name          at <unknown>:10:20
    v9 := lookup-bound(v7, v8)                  ; var           at <unknown>:10:20
    cache-set![(0 . <foo>)](v9)                 ;               at <unknown>:10:20
    L20(v9)                                     ;               at <unknown>:10:20
  L20(v10):                                     ; box 
    v11 := scm-ref/immediate[(box . 1)](v10)    ; arg           at <unknown>:10:20
    eq?(v5, v11) ? L22() : L37()                ;               at <unknown>:16:26
  L37():
    throw throw/value+data[#(wrong-type-arg foo-x "Wrong type argument: ~S")](v4) ;  at <unknown>:16:26
  L22():
    v12 := word-ref/immediate[(struct . 6)](v5) ; rfields       at <unknown>:16:26
    v13 := v12                                  ; nfields       at <unknown>:16:26
    imm-u64-<[0](v13) ? L25() : L35()           ;               at <unknown>:16:26
  L35():
    v21 := const 0                              ; _             at <unknown>:16:26
    throw throw/value+data[#(out-of-range "struct-ref/immediate" "Argument 2 out of range: ~S")](v21) ;  at <unknown>:16:26
  L25():
    v14 := pointer-ref/immediate[(struct . 7)](v5) ; ptr        at <unknown>:16:26
    v15 := load-u64[0]()                        ; word          at <unknown>:16:26
    v16 := u32-ref[bitmask](v5, v14, v15)       ; bits          at <unknown>:16:26
    v17 := load-u64[1]()                        ; mask          at <unknown>:16:26
    v18 := ulogand(v17, v16)                    ; res           at <unknown>:16:26
    u64-imm-=[0](v18) ? L31() : L33()           ;               at <unknown>:16:26
  L33():
    v20 := const 0                              ; _             at <unknown>:16:26
    throw throw/value+data[#(wrong-type-arg "struct-ref/immediate" "Wrong type argument in position 2 (expecting boxed field): ~S")](v20) ;  at <unknown>:16:26
  L31():
    v19 := scm-ref/immediate[(struct . 1)](v4)  ; val           at <unknown>:16:26
    return v19                                  ;               at <unknown>:16:26

Here we see the first procedure which is the thunk that wraps the
expression.  Then in the beginning of the procedure at L7 you can see
there is a check for struct?, which has to be dominated by a true
heap-object? check.  Duplicate checks are elided.  So if SRFI-9 added a
`struct?` check it wouldn't be more code; rather it would be less,
actually, because instead of branching to L38, you'd branch to L37.

Too bad about all that other crap about checking whether the index is in
range and the field is boxed or not, though :-/  Probably there is a
better design...

Andy

Re: better error messages through assertions

[Ludovic Courtès]

Hi,

Andy Wingo <wingo@igalia.com> skribis:

> Here we see the first procedure which is the thunk that wraps the
> expression.  Then in the beginning of the procedure at L7 you can see
> there is a check for struct?, which has to be dominated by a true
> heap-object? check.  Duplicate checks are elided.  So if SRFI-9 added a
> `struct?` check it wouldn't be more code; rather it would be less,
> actually, because instead of branching to L38, you'd branch to L37.

Oh very nice, thanks for the explanation!  That ‘,optimize-cps’ command
proves useful.

So we can have better error reports with no performance penalty,
Philip.  Way to go!

Ludo’.

Re: better error messages through assertions

[Philip McGrath]

Hi,

On 3/30/22 09:28, Andy Wingo wrote:
> 
> Too bad about all that other crap about checking whether the index is in
> range and the field is boxed or not, though :-/  Probably there is a
> better design...
> 
> Andy

For the index-out-of-range part, when I saw `record-accessor`, I thought 
of it as similar to Racket's `make-struct-field-accessor`[1], which can 
check the index just once, when the accessor is created, rather than 
each time the accessor is used. That's (part of) what Racket's `struct` 
form expands to.

Would it be reasonable to use `record-accessor` in the implementation of 
SRFI 9?

-Philip

[1]: 
https://docs.racket-lang.org/reference/creatingmorestructs.html#%28def._%28%28quote._~23~25kernel%29._make-struct-field-accessor%29%29

Re: better error messages through assertions

[Ludovic Courtès]

Hi,

Philip McGrath <philip@philipmcgrath.com> skribis:

> On 3/30/22 09:28, Andy Wingo wrote:
>> Too bad about all that other crap about checking whether the index
>> is in
>> range and the field is boxed or not, though :-/  Probably there is a
>> better design...
>> Andy
>
> For the index-out-of-range part, when I saw `record-accessor`, I
> thought of it as similar to Racket's `make-struct-field-accessor`[1],
> which can check the index just once, when the accessor is created,
> rather than each time the accessor is used. That's (part of) what
> Racket's `struct` form expands to.
>
> Would it be reasonable to use `record-accessor` in the implementation
> of SRFI 9?

Yes, or in (guix records).

Medium-term, I thought we could rebase (guix records) on Guile records
with an eye on adding support for inheritance.  So perhaps we could do
both.

Ludo’.

Re: better error messages through assertions

[Philip McGrath]

Hi,

On 3/30/22 05:37, Ludovic Courtès wrote:
> 
>> What is the preferred mechanism for exceptions?
> 
> For Guix code, SRFI-34/35.
> 
>> Likewise, what record system should I use?
> 
> SRFI-9.
> 
> (Perhaps we should put answers to these questions in the “Coding Style”
> section of the manual.)
> 

As I've looked more closely, I think I'd be porting a fair amount of 
code that looks like this:

```
(define-struct (blame-no-swap blame) ()
   #:property prop:equal+hash
   (list blame=? blame-hash blame-secondary-hash))
```

That code creates a new opaque struct type `blame-no-swap` with parent 
type `blame` and customizes the behavior of `equal?` and `equal?`-based 
hashing (but the customization only applies when the two 
potentially-equal values are both immediate instances of 
`blame-no-swap`, not a supertype or subtype).

It looks like SRFI 9 doesn't have a notion of opaque or parent types, so 
it seems like it would be easier to use R6RS records as the basis for 
porting.

(I think it should still work to use SRFI 9 records for the interface. 
But I know the representation of blame, among other details, has gotten 
a lot of attention over the years, so it seems like it would be good to 
keep the port as close as reasonably possible to the Racket implementation.)

I'm less sure about `equal?` and hashing. I see in [1] that `equal?` can 
be controlled via GOOPS, and I have the impression that there's some 
kind of bridging between basic types and GOOPS, but I'm not clear on the 
details. I don't see anything explicitly about customizing `hash`[2], 
but maybe that would work the same way?

-Philip

[1]: 
https://www.gnu.org/software/guile/manual/html_node/GOOPS-Object-Miscellany.html
[2]: 
https://www.gnu.org/software/guile/manual/html_node/Hash-Table-Reference.html#index-hash

Re: better error messages through assertions

[Arun Isaac]

[-- Attachment #1: Type: text/plain, Size: 635 bytes --]


Hi Ricardo,

I too would like to see much better error checking for our services,
though I don't know what the best way to achieve that is.

Type checking alone may not be sufficient. All types in a configuration
record could be valid, but some combination of field values in the
record may not be valid. For a quick and dumb example, it may not be
allowed to configure a service to listen on a TCP port and a Unix socket
at the same time, and we should error out if it is told to do so. For
these use cases, we need to support more complex predicates that take
the whole configuration record and check if it is valid.

Regards,
Arun

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 524 bytes --]

Re: better error messages through assertions

[Maxim Cournoyer]

Hello Ricardo,

Ricardo Wurmus <rekado@elephly.net> writes:

> Hi Guix,
>
> today on IRC someone reported an ugly error message when reconfiguring
> their system:
>
> Backtrace:
>           18 (primitive-load "/home/me/.config/guix/current/bin/…")
> In guix/ui.scm:
>    2209:7 17 (run-guix . _)
>   2172:10 16 (run-guix-command _ . _)
> In ice-9/boot-9.scm:
>   1752:10 15 (with-exception-handler _ _ #:unwind? _ # _)
> In guix/status.scm:
>     822:3 14 (_)
>     802:4 13 (call-with-status-report _ _)
> In guix/scripts/system.scm:
>    1256:4 12 (_)
> In ice-9/boot-9.scm:
>   1752:10 11 (with-exception-handler _ _ #:unwind? _ # _)
> In guix/store.scm:
>    658:37 10 (thunk)
>    1320:8  9 (call-with-build-handler #<procedure 7fecaf8570c0 at g…> …)
>   2123:24  8 (run-with-store #<store-connection 256.99 7fecb75c7230> …)
> In guix/scripts/system.scm:
>     827:2  7 (_ _)
>     703:7  6 (_ #<store-connection 256.99 7fecb75c7230>)
> In gnu/system.scm:
>   1227:19  5 (operating-system-derivation _)
> In gnu/services.scm:
>    1091:6  4 (instantiate-missing-services _)
> In srfi/srfi-1.scm:
>    460:18  3 (fold #<procedure 7fecb73c0960 at gnu/services.scm:109…> …)
> In gnu/services.scm:
>   1092:27  2 (_ (#<<service> type: #<service-type gdm 7fecbd17f6…> …) …)
> In ice-9/boot-9.scm:
>   1685:16  1 (raise-exception _ #:continuable? _)
>   1685:16  0 (raise-exception _ #:continuable? _)
>
> ice-9/boot-9.scm:1685:16: In procedure raise-exception:
> In procedure struct-vtable: Wrong type argument in position 1 (expecting struct):
>
> As you can probably tell easily by looking at this message, the
> “service” field of the operating system configuration looked something
> like this:
>
>   (services (append (list a b c %desktop-services) #;oops))
>
> instead of this
>
>   (services (append (list a b c) %desktop-services))
>
> This is because INSTANTIATE-MISSING-SERVICES — and FOLD-SERVICES, and
> many more — assumes that it is only passed a plain list of services.  It
> then proceeds to call SERVICE-KIND on what may or may not be a service.
>
> I think we should add simple type checks, something like this:
>
>   (define (listof pred)
>     (lambda (thing)
>      (and (list? thing) (every pred thing))))
>   …
>   (define (assert-type type-check thing message)
>     (or (false-if-exception (type-check thing))
>         (report-error (G_ "type error: …\n" message))))
>
>   ;; Use ASSERT-TYPE in an example procedure.
>   (define (do-something-with-services services)
>     (assert-type (listof service?) services
>                  "SERVICES must be a list of <service> values.")
>
>     ;; Do things…
>     (map service-kind services))
>
> What do you think?  There are many different ways of implementing this
> (a new variant of DEFINE that also accepts a type declaration, an assert
> like above, a fancier assert that composes a helpful error message by
> itself, a separate type declaration that is looked up only when the
> corresponding procedure is called in a certain context, etc), but I’d
> first like to know if there is consensus that we want something like
> this.

I hear we now have "field sanitizers" on Guix records; without having
dug the details, it seems to be we could add a predicate validating the
input there?  The nice thing about it is that it'd be a one place
change, instead of asserts to sprinkle around various places.

Thanks,

Maxim

Re: better error messages through assertions

[Ricardo Wurmus]

Maxim Cournoyer <maxim.cournoyer@gmail.com> writes:

> I hear we now have "field sanitizers" on Guix records; without having
> dug the details, it seems to be we could add a predicate validating the
> input there?

I don’t see how that would help here.  In my example the service values
themselves are all right.  It’s a procedure acting on what it assumes is
a list of service values that fails.

Record field validation would not have prevented that error.

-- 
Ricardo

Re: better error messages through assertions

[Maxim Cournoyer]

Hi Ricardo,

Ricardo Wurmus <rekado@elephly.net> writes:

> Maxim Cournoyer <maxim.cournoyer@gmail.com> writes:
>
>> I hear we now have "field sanitizers" on Guix records; without having
>> dug the details, it seems to be we could add a predicate validating the
>> input there?
>
> I don’t see how that would help here.  In my example the service values
> themselves are all right.  It’s a procedure acting on what it assumes is
> a list of service values that fails.
>
> Record field validation would not have prevented that error.

Thanks for explaining.  For what it's worth, what you are proposing
seems a sound improvement to me, so feel free to go ahead with patches!

Thanks,

Maxim

Re: better error messages through assertions

[Ludovic Courtès]

Ricardo Wurmus <rekado@elephly.net> skribis:

> Maxim Cournoyer <maxim.cournoyer@gmail.com> writes:
>
>> I hear we now have "field sanitizers" on Guix records; without having
>> dug the details, it seems to be we could add a predicate validating the
>> input there?
>
> I don’t see how that would help here.  In my example the service values
> themselves are all right.  It’s a procedure acting on what it assumes is
> a list of service values that fails.
>
> Record field validation would not have prevented that error.

I think we could validate primarily at the user/core boundary.  If the
‘services’ field had a sanitizer ensuring it’s a list of services, then
it’d be fine.

Ludo’.

Re: better error messages through assertions

[Maxim Cournoyer]

Hello,

Ludovic Courtès <ludo@gnu.org> writes:

> Ricardo Wurmus <rekado@elephly.net> skribis:
>
>> Maxim Cournoyer <maxim.cournoyer@gmail.com> writes:
>>
>>> I hear we now have "field sanitizers" on Guix records; without having
>>> dug the details, it seems to be we could add a predicate validating the
>>> input there?
>>
>> I don’t see how that would help here.  In my example the service values
>> themselves are all right.  It’s a procedure acting on what it assumes is
>> a list of service values that fails.
>>
>> Record field validation would not have prevented that error.
>
> I think we could validate primarily at the user/core boundary.  If the
> ‘services’ field had a sanitizer ensuring it’s a list of services, then
> it’d be fine.

That seems a fine approach to me; thank you for your input.  Ricardo,
would you like to give idea a shot and see if it suites the bill? :-).

Maxim