Mathieu Othacehe schreef op di 06-07-2021 om 19:29 [+0200]:
> Hey Maxime,
> 
> > +  "Try to find the body of the procedure defined inline by EXPRESSION.
> > +If it was found, call EXPRESSION with its body. If it wasn't, call
>                                ^
>                                FOUND
> 
> > +    (`(,(or 'let 'let*) . ,_)
> > +     (find-procedure-body (car (last-pair expression)) found
> > +                          #:not-found not-found))
> 
> You can use "last" from (srfi srfi-1) here.

No, I can't -- unless a backtrace in case of invalid code is acceptable.
The problem is that the procedure 'last' expects its argument to be a list.
E.g., try from a REPL:

scheme@(guile-user)> ((@ (srfi srfi-1) last) '("a" . 0))
$1 = 0

... wait, why didn't this raise an exception?
Looking at the definition of 'last' in (srfi srfi-1) in guile, I see

(define (last pair)
  "Return the last element of the non-empty, finite list PAIR."
  (car (last-pair pair)))

So, you're correct that "last" from (srfi srfi-1) can be used here,
but this still seems rather fragile to me and an implementation detail
of (srfi srfi-1).

> What's the point of stripping the let clause by the way?

Because 'find-procedure-body' is supposed to find the body of the procedure,
but sometimes the lambda is wrapped in a 'let'. (I don't have an example
currently in mind, but I've definitely seen things like

  (modify-phases %standard-phases
    (replace 'something
      (let ((three (+ 1 2))
        (lambda _
          (format #t "~a~%" tree)))))


in package definitions).  You could ask, why do we need to extract the
procedure body, can't we just pass the whole expression as-is to
'check-procedure-body'?

That's a possiblity, but would lead to some false negatives: consider the
following phases definition:

  (modify-phases %standard-phases
    (replace 'check
      (let ((three (+ 1 2)))
        (lambda* (#:key tests? #:allow-other-keys)
          (invoke "stuff" (number->string bla-bla))))))

In this case, the parameter tests? is ignored, even though the symbol 'tests?'
appears in the expression.  So we shouldn't look at the parameter.

--- wait, this is a patch review for the 'wrapper-inputs' linter, not the
'optional-tests' linter!  Indeed, for the 'wrapper-inputs' linter, stripping
the 'let' (or 'let*') is pointless.  It might even lead to false negatives!

Consider the case

    (add-after 'install 'wrap
      (let ((wrap (lambda (x) (wrap-program x [...]))))
        (lambda _
          (wrap "stuff")
          (wrap "other-stuff"))))

That usage should ideally be detected by 'wrap-program'.  But as no
current package definition seems to do such a thing, and using
'find-procedure-body' seems marginally ‘cleaner’ to me (YMMV?),
I would use 'find-procedure-body' anyways.

> > +  (list (make-warning package
> > +                      ;; TRANSLATORS: 'modify-phases' is a Scheme syntax
> > +                      ;; and should not be translated.
> > +                      (G_ "incorrect call to ‘modify-phases’")
> > +                      #:field 'arguments)))
> 
> Maybe you could return a plain object here.

Yes, but then

(define* (find-phase-deltas package found
                            #:key (not-found (const '()))
                            (bogus (cut report-bogus-phase-deltas package <>)))

would need to become

(define* (find-phase-deltas package found
                            #:key (not-found (const '()))
                            (bogus (lambda (bogus-deltas)
                                     (list (report-bogus-deltas package bogus-deltas)))))

which is rather verbose. (The 'bogus-deltas' argument could be dropped I suppose?
But 'find-phase-deltas' is supposed to be generic, such that its callers can have
easy access to the bogus (dotted) list of phases ...)


> > +  (list (make-warning package
> > +                      ;; TRANSLATORS: See ‘modify-phases’ in the manual.
> > +                      (G_ "invalid phase clause")
> > +                      #:field 'arguments)))
> 
> and here.

Likewise.

Greetings,
Maxime.