Message ID | 20230114151457.1903-2-maxim.cournoyer@gmail.com |
---|---|
State | New |
Headers | show |
Series | [bug#60802,v3,1/2] platforms: Raise an exception when no suitable platform is found. | expand |
Hi Maxim, This looks good to me, although in the grand scheme of things I wonder if that change is a step forward: for those kinds of procedures, we could expect consumers to instead always properly handle the #f case themselves, rather than baby-sitting them and systematically relying on exceptions in the parent procedure, no? As a caricatural example: the SRFI-1 `find` could raise an exception instead of returning #f, but I don't think anyone would consider that proper behaviour. I don't have a particularly strong opinion towards either option, which probably means that there's some discussion to be had here. Best,
Hi Josselin, Josselin Poiret <dev@jpoiret.xyz> writes: > Hi Maxim, > > This looks good to me, although in the grand scheme of things I wonder > if that change is a step forward: for those kinds of procedures, we > could expect consumers to instead always properly handle the #f case > themselves, rather than baby-sitting them and systematically relying on > exceptions in the parent procedure, no? I like exceptions; I think they are an improvement to C-style ad-hoc checking of each call return; when things fail they fail early and in a clear fashion, rather than indirectly, which is easier to debug and removes the burden to duplicate the same checks at every call site. > As a caricatural example: the SRFI-1 `find` could raise an exception > instead of returning #f, but I don't think anyone would consider that > proper behaviour. I think the find interface is well established; lookup-platform-by-system and friends is not really intended to return #f, as shown in the few usages in our code base (there's only one place where it needed to be handled in previous code, and that's a TODO waiting to move more things to (guix platform)). > I don't have a particularly strong opinion towards either option, which > probably means that there's some discussion to be had here. Good, that's what reviews are for :-).
Hi Josselin, On dim., 15 janv. 2023 at 14:57, Josselin Poiret via Guix-patches via <guix-patches@gnu.org> wrote: > This looks good to me, although in the grand scheme of things I wonder > if that change is a step forward: for those kinds of procedures, we > could expect consumers to instead always properly handle the #f case > themselves, rather than baby-sitting them and systematically relying on > exceptions in the parent procedure, no? As a caricatural example: the > SRFI-1 `find` could raise an exception instead of returning #f, but I > don't think anyone would consider that proper behaviour. How to handle, is it not somehow a convention? Using the example ’find’, we could imagine it returns an exception ’not-found’ instead of the current plain #f – this #f appears to me a convention, no? Or ’find’ could return some Maybe monad (’Just the-result’ or None) – another convention. Well, how to handle appears to me a convention when composing. :-) Cheers, simon
Hi Josselin, Josselin Poiret <dev@jpoiret.xyz> skribis: > This looks good to me, although in the grand scheme of things I wonder > if that change is a step forward: for those kinds of procedures, we > could expect consumers to instead always properly handle the #f case > themselves, rather than baby-sitting them and systematically relying on > exceptions in the parent procedure, no? As a caricatural example: the > SRFI-1 `find` could raise an exception instead of returning #f, but I > don't think anyone would consider that proper behaviour. I share this sentiment in general (plus the fact that we should keep UI aspects, such as error reports, separate from core logic). Here there’s a precedent with other lookup procedures though (‘lookup-bootloader-by-name’, ‘lookup-compressor’, ‘lookup-image-type-by-name’, etc.), so I think it’s okay to keep it that way. Ludo’.
Hi, On mar., 17 janv. 2023 at 09:59, Ludovic Courtès <ludo@gnu.org> wrote: > Josselin Poiret <dev@jpoiret.xyz> skribis: > >> This looks good to me, although in the grand scheme of things I wonder >> if that change is a step forward: for those kinds of procedures, we >> could expect consumers to instead always properly handle the #f case >> themselves, rather than baby-sitting them and systematically relying on >> exceptions in the parent procedure, no? As a caricatural example: the >> SRFI-1 `find` could raise an exception instead of returning #f, but I >> don't think anyone would consider that proper behaviour. > > I share this sentiment in general (plus the fact that we should keep UI > aspects, such as error reports, separate from core logic). Here there’s > a precedent with other lookup procedures though > (‘lookup-bootloader-by-name’, ‘lookup-compressor’, > ‘lookup-image-type-by-name’, etc.), so I think it’s okay to keep it that > way. Well, from my small experience with other programming language, they barely do return a boolean when they fail. I think this way using a boolean is because some historical reasons when exceptions was not implemented in Scheme (or other languages). Exception allows the motto: «ask for forgiveness, not permission» while keeping under control the side effects. Well, IMHO exception is often a good practise for dynamically typed programming language; as Scheme (or Python). From my point of view, exception is not related to “should keep UI aspects, such as error reports, separated from core logic”. This is how the exception is handled. It is often easier to propagate exception until an handler than propagate a boolean (as with ’find’). And if the exception is not handled, then it just returns a backtrace, which is more informative, IMHO. For instance, Python returns an exception: --8<---------------cut here---------------start------------->8--- $ guix shell python python-ipython -- ipython Python 3.9.9 (main, Jan 1 1970, 00:00:01) Type 'copyright', 'credits' or 'license' for more information IPython 8.2.0 -- An enhanced Interactive Python. Type '?' for help. In [1]: lst = [1,2,3] lst = [1,2,3] In [2]: lst.index(2) Out[2]: 1 In [3]: lst.index(10) --------------------------------------------------------------------------- ValueError Traceback (most recent call last) Input In [3], in <cell line: 1>() ----> 1 lst.index(10) ValueError: 10 is not in list --8<---------------cut here---------------end--------------->8--- For other instances, OCaml implements two ’find’ [1], one using an exception and another using a Maybe monad (named ’option’). --8<---------------cut here---------------start------------->8--- val find : ('a -> bool) -> 'a list -> 'a Raises Not_found if there is no value that satisfies f in the list l. val find_opt : ('a -> bool) -> 'a list -> 'a option --8<---------------cut here---------------end--------------->8--- Haskell returns a Maybe monad [2]: --8<---------------cut here---------------start------------->8--- find :: Foldable t => (a -> Bool) -> t a -> Maybe a --8<---------------cut here---------------end--------------->8--- Well, from a signature point of view, ’find’ from SRFI-1 returns an union type (value or boolean) which can lead to hard to detect bugs: when composing functions, the error can be incorrectly pointed by the compiler to a totally unrelated place. Instead, the exception allows to keep an expected signature (say one value as with ’find’) but raises the error at the correct place. If there is no handler, it just raises the backtrace. From my point of view, code using exception cannot be worse* than without. That’s my general sentiment. :-) *worse: for sure, we could discuss some performance penalty depending on the context. 1: <https://v2.ocaml.org/api/List.html> 2: <https://hackage.haskell.org/package/base-4.17.0.0/docs/Data-List.html> Cheers, simon
Hi Simon, Simon Tournier <zimon.toutoune@gmail.com> writes: > Hi, > > On mar., 17 janv. 2023 at 09:59, Ludovic Courtès <ludo@gnu.org> wrote: >> Josselin Poiret <dev@jpoiret.xyz> skribis: >> >>> This looks good to me, although in the grand scheme of things I wonder >>> if that change is a step forward: for those kinds of procedures, we >>> could expect consumers to instead always properly handle the #f case >>> themselves, rather than baby-sitting them and systematically relying on >>> exceptions in the parent procedure, no? As a caricatural example: the >>> SRFI-1 `find` could raise an exception instead of returning #f, but I >>> don't think anyone would consider that proper behaviour. >> >> I share this sentiment in general (plus the fact that we should keep UI >> aspects, such as error reports, separate from core logic). Here there’s >> a precedent with other lookup procedures though >> (‘lookup-bootloader-by-name’, ‘lookup-compressor’, >> ‘lookup-image-type-by-name’, etc.), so I think it’s okay to keep it that >> way. > > Well, from my small experience with other programming language, they > barely do return a boolean when they fail. I think this way using a > boolean is because some historical reasons when exceptions was not > implemented in Scheme (or other languages). > > Exception allows the motto: «ask for forgiveness, not permission» while > keeping under control the side effects. Well, IMHO exception is often a > good practise for dynamically typed programming language; as Scheme (or > Python). > > From my point of view, exception is not related to “should keep UI > aspects, such as error reports, separated from core logic”. This is how > the exception is handled. It is often easier to propagate exception > until an handler than propagate a boolean (as with ’find’). > > And if the exception is not handled, then it just returns a backtrace, > which is more informative, IMHO. You've summarized well the reasons I think using an exception here makes sense (and why using exceptions rather than C-style booleans to propagate error conditions is preferable in general for languages where it's possible to do so -- luckily Scheme is one). I'll send a v4 reworking it to use srfi-34/35, so that the discussion to migrate to (ice-9 exceptions) can be kept separate.
diff --git a/gnu/packages/bootstrap.scm b/gnu/packages/bootstrap.scm index d2914fb5a7..9ea1a3e4d1 100644 --- a/gnu/packages/bootstrap.scm +++ b/gnu/packages/bootstrap.scm @@ -315,7 +315,8 @@ (define* (glibc-dynamic-linker (%current-system)))) "Return the name of Glibc's dynamic linker for SYSTEM." ;; See the 'SYSDEP_KNOWN_INTERPRETER_NAMES' cpp macro in libc. - (let ((platform (lookup-platform-by-system system))) + (let ((platform (false-if-platform-not-found + (lookup-platform-by-system system)))) (cond ((platform? platform) (platform-glibc-dynamic-linker platform)) diff --git a/guix/platform.scm b/guix/platform.scm index f873913fe0..4f4da002f7 100644 --- a/guix/platform.scm +++ b/guix/platform.scm @@ -21,6 +21,7 @@ (define-module (guix platform) #:use-module (guix memoization) #:use-module (guix records) #:use-module (guix ui) + #:use-module (ice-9 exceptions) #:use-module (srfi srfi-1) #:export (platform platform? @@ -29,6 +30,10 @@ (define-module (guix platform) platform-linux-architecture platform-glibc-dynamic-linker + &platform-not-found-error + platform-not-found-error? + false-if-platform-not-found + platform-modules platforms lookup-platform-by-system @@ -70,6 +75,19 @@ (define-record-type* <platform> platform make-platform (default #false)) (glibc-dynamic-linker platform-glibc-dynamic-linker)) + +;;; +;;; Exceptions. +;;; +(define-exception-type &platform-not-found-error &error + make-platform-not-found-error platform-not-found-error?) + +(define-syntax-rule (false-if-platform-not-found exp) + "Evaluate EXP but return #f if it raises a platform-not-found-error? +exception." + (guard (ex ((platform-not-found-error? ex) #f)) + exp)) + ;;; ;;; Platforms. @@ -94,23 +112,29 @@ (define platforms (platform-modules))))) (define (lookup-platform-by-system system) - "Return the platform corresponding to the given SYSTEM." - (find (lambda (platform) - (let ((s (platform-system platform))) - (and (string? s) (string=? s system)))) - (platforms))) + "Return the platform corresponding to the given SYSTEM. Raise +&PLATFORM-NOT-FOUND-ERROR when no platform could be found." + (or (find (lambda (platform) + (let ((s (platform-system platform))) + (and (string? s) (string=? s system)))) + (platforms)) + (raise-exception (make-platform-not-found-error)))) (define (lookup-platform-by-target target) - "Return the platform corresponding to the given TARGET." - (find (lambda (platform) - (let ((t (platform-target platform))) - (and (string? t) (string=? t target)))) - (platforms))) + "Return the platform corresponding to the given TARGET. Raise +&PLATFORM-NOT-FOUND-ERROR when no platform could be found." + (or (find (lambda (platform) + (let ((t (platform-target platform))) + (and (string? t) (string=? t target)))) + (platforms)) + (raise-exception (make-platform-not-found-error)))) (define (lookup-platform-by-target-or-system target-or-system) - "Return the platform corresponding to the given TARGET or SYSTEM." - (or (lookup-platform-by-target target-or-system) - (lookup-platform-by-system target-or-system))) + "Return the platform corresponding to the given TARGET or SYSTEM. Raise +&PLATFORM-NOT-FOUND-ERROR when no platform could be found." + (or (false-if-platform-not-found (lookup-platform-by-target target-or-system)) + (false-if-platform-not-found (lookup-platform-by-system target-or-system)) + (raise-exception (make-platform-not-found-error)))) (define (platform-system->target system) "Return the target matching the given SYSTEM if it exists or false