[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[bug#49315] [PATCH]: Lint usages of 'wrap-program' without a "bash" inpu
From: |
Maxime Devos |
Subject: |
[bug#49315] [PATCH]: Lint usages of 'wrap-program' without a "bash" input. |
Date: |
Tue, 06 Jul 2021 22:38:03 +0200 |
User-agent: |
Evolution 3.34.2 |
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.
signature.asc
Description: This is a digitally signed message part