Re: [PATCH] add SRFI-119 / language/wisp to Guile? (new patch, squashed)

[Ludovic Courtès]

Hi!

"Dr. Arne Babenhauserheide" <arne_bab@web.de> skribis:

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

[...]

>> Given the complexities in changing the way languages are handled (the
>> required discussions, as we’ve seen in the not yet resolved discussion),
>> would you be OK with keeping the question about adding support for
>> SRFI-119 to Guile separate from the general discussion about language
>> handling?
>>
>> Are there improvements needed besides the ones I did thanks to the
>> review by Maxime or is this good to go?
>
> I created a new (squashed) version of the patch to simplify reviewing:

It does, thank you!

Overall I’m confident the code has been battle-tested (I suppose there
are minimal changes compared to Wisp, right?), so I’ll comment mostly on
cosmetic issues.

> From c7a50f632293cf88f241d45d1fd52fa2f58ce772 Mon Sep 17 00:00:00 2001
> From: Arne Babenhauserheide <arne_bab@web.de>
> Date: Fri, 3 Feb 2023 22:20:04 +0100
> Subject: [PATCH] Add language/wisp, wisp tests, and srfi-119 documentation
>
> * doc/ref/srfi-modules.texi (srfi-119): add node
> * module/language/wisp.scm: New file.
> * module/language/wisp/spec.scm: New file.
> * test-suite/tests/srfi-119.test: New file.

Please add the new files to the relevant ‘Makefile.am’ files.

> +* SRFI-119::                    Wisp: simpler indentation-sensitive scheme.

Please capitalize “Scheme” (in general we like to pay attention to
typography, spelling, and language in the manual.)

> +@subsection SRFI-119 Wisp: simpler indentation-sensitive scheme.
> +@cindex SRFI-119
> +@cindex wisp

Same: “Scheme”, “Wisp”.

> +The languages shipped in Guile include SRFI-119 (wisp), an encoding of

s/(wisp)/, also referred to as @dfn{Wisp} (for ``Whitespace …'')/

> +Scheme that allows replacing parentheses with equivalent indentation and
> +inline colons. See

Two spaces after end-of-sentence periods, to facilitate navigation in
Emacs.

> +++ b/module/language/wisp.scm
> @@ -0,0 +1,761 @@
> +;;; Wisp
> +
> +;; Copyright (C) 2013, 2017, 2018, 2020 Free Software Foundation, Inc.
> +;; Copyright (C) 2014--2023 Arne Babenhauserheide.
> +;; Copyright (C) 2023 Maxime Devos <maximedevos@telenet.be>

I see you don’t have a copyright assignment on file for Guile.  If you
wish, you can assign copyright to the FSF:

  https://lists.gnu.org/archive/html/guile-devel/2022-10/msg00008.html

Let us know what you’d like to do.

> +(define-module (language wisp)
> +   #:export (wisp-scheme-read-chunk wisp-scheme-read-all 
> +               wisp-scheme-read-file-chunk wisp-scheme-read-file
> +               wisp-scheme-read-string))

Please remove tabs from this file and indent it “the usual way”.  You
can do that by passing it through Emacs and using ‘M-x indent-region’,
or possibly using ‘guix style -f’.

> +; use curly-infix by default

Use two leading colons for comments, except for margin comments.

> +(read-enable 'curly-infix)

This needs to be:

  (eval-when (expand load eval)
    (read-enable 'curly-infix))

> +(use-modules
> +  (srfi srfi-1)
> +  (srfi srfi-11); for let-values
> +  (ice-9 rw); for write-string/partial
> +  (ice-9 match))

Please make them #:use-module clauses in the ‘define-module’ form above.

> +;; Helper functions for the indent-and-symbols data structure: '((indent 
> token token ...) ...)
> +(define (line-indent line)
> +         (car line))
> +
> +(define (line-real-indent line)
> +         "Get the indentation without the comment-marker for unindented 
> lines (-1 is treated as 0)."
> +         (let (( indent (line-indent line)))
> +             (if (= -1 indent)
> +               0
> +               indent)))
> +
> +(define (line-code line)
> +         (let ((code (cdr line)))
> +             ; propagate source properties
> +             (when (not (null? code))
> +                    (set-source-properties! code (source-properties line)))
> +             code))

Instead of using lists or pairs for “lines”, I suggest defining a proper
record type, like:

  (define-record-type <input-line>
    (input-line indent content)
    input-line?
    (indent  input-line-indent)
    (content input-line-content))

This will make the code easier to read and type-clear.

> +; literal values I need
> +(define readcolon 
> +       (string->symbol ":"))
> +
> +(define wisp-uuid "e749c73d-c826-47e2-a798-c16c13cb89dd")
> +; define an intermediate dot replacement with UUID to avoid clashes.
> +(define repr-dot ; .
> +       (string->symbol (string-append "REPR-DOT-" wisp-uuid)))
> +
> +; allow using reader additions as the first element on a line to prefix the 
> list
> +(define repr-quote ; '
> +       (string->symbol (string-append "REPR-QUOTE-" wisp-uuid)))
> +(define repr-unquote ; ,
> +       (string->symbol (string-append "REPR-UNQUOTE-" wisp-uuid)))
> +(define repr-quasiquote ; `
> +       (string->symbol (string-append "REPR-QUASIQUOTE-" wisp-uuid)))
> +(define repr-unquote-splicing ; ,@
> +       (string->symbol (string-append "REPR-UNQUOTESPLICING-" wisp-uuid)))
> +
> +(define repr-syntax ; #'
> +       (string->symbol (string-append "REPR-SYNTAX-" wisp-uuid)))
> +(define repr-unsyntax ; #,
> +       (string->symbol (string-append "REPR-UNSYNTAX-" wisp-uuid)))
> +(define repr-quasisyntax ; #`
> +       (string->symbol (string-append "REPR-QUASISYNTAX-" wisp-uuid)))
> +(define repr-unsyntax-splicing ; #,@
> +       (string->symbol (string-append "REPR-UNSYNTAXSPLICING-" wisp-uuid)))

Instead of using these UUIDs, I suggest using known unique objects,
“unique” in the sense of ‘eq?’.

So it can be:

  (define repr-dot (make-symbol "repr-dot"))  ;uninterned symbol

> +(define (wisp-scheme-read-chunk-lines port)
> +         (let loop
> +           ((indent-and-symbols (list)); '((5 "(foobar)" "\"yobble\"")(3 
> "#t"))
> +             (inindent #t)
> +             (inunderscoreindent (equal? #\_ (peek-char port)))
> +             (incomment #f)
> +             (currentindent 0)
> +             (currentsymbols '())
> +             (emptylines 0))

I’d encourage following the usual naming convention, so ‘in-indent?’,
‘in-comment?’, etc.

> +                 ((and inunderscoreindent (and (not (equal? #\space 
> next-char)) (not (equal? #\newline next-char))))
> +                   (throw 'wisp-syntax-error "initial underscores without 
> following whitespace at beginning of the line after" (last 
> indent-and-symbols)))

I suggest using exception objects or SRFI-35 error conditions (they’re
interchangeable) carrying information about the king of parsing error
and its location.  That way, the user interface, such as the compiler,
can produce helpful error messages (and internationalized, too).

> +(define (wisp-replace-paren-quotation-repr code)
> +         "Replace lists starting with a quotation symbol by
> +         quoted lists."
> +         (match code
> +             (('REPR-QUOTE-e749c73d-c826-47e2-a798-c16c13cb89dd a ...)
> +                (list 'quote (map wisp-replace-paren-quotation-repr a)))

You need to write these clauses like:

  (match code
    (((? wisp-quote?) body ...)
     …))

where:

  (define (wisp-quote? obj) (eq? obj repr-quote))

On the use of source properties: they’re not ideal (they rely on a side
hash table that’s quite memory-hungry) and it would be nice to consider
also implementing the “annotated read” interface (info "(guile)
Annotated Scheme Read").  That can come in a subsequent patch, though.

> +++ b/module/language/wisp/spec.scm
> @@ -0,0 +1,85 @@
> +;; Language interface for Wisp in Guile
> +
> +;;; adapted from guile-sweet: 
> https://gitorious.org/nacre/guile-sweet/source/ae306867e371cb4b56e00bb60a50d9a0b8353109:sweet/common.scm
> +
> +;;; Copyright (C) 2005--2014 by David A. Wheeler and Alan Manuel K. Gloria
> +;;; Copyright (C) 2014--2023 Arne Babenhauserheide.
> +;;; Copyright (C) 2023 Maxime Devos <maximedevos@telenet.be>
> +
> +;;; Permission is hereby granted, free of charge, to any person
> +;;; obtaining a copy of this software and associated documentation
> +;;; files (the "Software"), to deal in the Software without
> +;;; restriction, including without limitation the rights to use, copy,
> +;;; modify, merge, publish, distribute, sublicense, and/or sell copies
> +;;; of the Software, and to permit persons to whom the Software is
> +;;; furnished to do so, subject to the following conditions:
> +;;;
> +;;; The above copyright notice and this permission notice shall be
> +;;; included in all copies or substantial portions of the Software.
> +;;;
> +;;; THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> +;;; EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> +;;; MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
> +;;; NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
> +;;; BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
> +;;; ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
> +;;; CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
> +;;; SOFTWARE.

Can you make it LGPLv3+?  It’s a small file anyway.

> +(define-language wisp
> +  #:title "Wisp Scheme Syntax. See SRFI-119 for details."
> +  ; . #:reader read-one-wisp-sexp
> +  #:reader read-one-wisp-sexp ; : lambda (port env) : let ((x 
> (read-one-wisp-sexp port env))) (display x)(newline) x ;
> +  #:compilers `((tree-il . ,compile-tree-il))
> +  #:decompilers `((tree-il . ,decompile-tree-il))
> +  #:evaluator (lambda (x module) (primitive-eval x))
> +  #:printer write ; TODO: backtransform to wisp? Use source-properties?
> +  #:make-default-environment
> +  (lambda ()
> +    ;; Ideally we'd duplicate the whole module hierarchy so that `set!',
> +    ;; `fluid-set!', etc. don't have any effect in the current environment.
> +    (let ((m (make-fresh-user-module)))
> +      ;; Provide a separate `current-reader' fluid so that
> +      ;; compile-time changes to `current-reader' are
> +      ;; limited to the current compilation unit.
> +      (module-define! m 'current-reader (make-fluid))
> +      ;; Default to `simple-format', as is the case until
> +      ;; (ice-9 format) is loaded. This allows
> +      ;; compile-time warnings to be emitted when using
> +      ;; unsupported options.
> +      (module-set! m 'format simple-format)

I’d remove this last statement, I think we should avoid fiddling with
bindings here.

> +(define-module (test-srfi-119)
> +  #:use-module (test-suite lib)
> +  #:use-module (srfi srfi-1)
> +  #:use-module (language wisp))

The test suite looks rather short; it would be nice if it would cover
more code.

In particular, I think source location info is important for front-ends
like this, because it determines the quality of error messages and thus
its ease of use.  Perhaps you could add test in that direction?

Overall I feel we should be able to merge this rather soon.  Do ping me
or Andy when needed!

Thanks,
Ludo’.