[bug#42146] [PATCH core-updates 1/?] build: substitute: Don't fail silently.

[Maxim Cournoyer <maxim.cournoyer@gmail.com>]

Hi Ludovic,

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

> Hi Maxim,
>
> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
>
>> Ludovic Courtès <ludo@gnu.org> writes:
>
> [...]
>
>>>> +;;; Extend regexp objects with a pattern field.
>>>> +;;;
>>>> +(define-record-type <regexp*>
>>>> +  (%make-regexp* pat flag rx)
>>>> +  regexp*?
>>>> +  (pat regexp*-pattern)                 ;the regexp pattern, a string
>>>> +  (flag regexp*-flag)                   ;regexp flags
>>>> +  (rx regexp*-rx))                      ;the compiled regexp object
>>>> +
>>>> +;;; Work around regexp implementation.
>>>> +;;; This record allows to track the regexp pattern and then display it.
>>>> +(define* (make-regexp* pat #:optional (flag regexp/extended))
>>>
>>> I’m skeptical about the concrete benefits.  I would not include it in
>>> (guix build utils), or at least not in this patch series.
>
> [...]
>
>> The benefit is concrete: it makes it possible to show which regexp
>> pattern failed to match (its textual representation), instead of
>> something much less useful such as a generic placeholder such as
>> "<unknown> (regexp)".
>
> Yes, okay.
>
> Can we keep the <regexp*> interface private, then?  To me it’s an
> implementation detail and perhaps a bit of a hack that I’d rather not
> commit to maintaining.

If you can think of a more elegant way to define it, I'm all ears.
Ideally, as Simon pointed out, that's something that Guile would support
natively (the ability to pull back the raw pattern from a regexp
object).  In Python for example, the re.Pattern object has a 'pattern'
property [0]

[0]  https://docs.python.org/3/library/re.html?highlight=regex#re.Pattern.pattern

Otherwise I don't think "the bit of a hack" is substantiated enough :-).

> The cost might be to duplicate it in teams.scm, but that’s an acceptable
> cost IMO.

I dislike copying bits around just to "hide" them from the "official"
interface, and have no fear maintaining that simple piece of code, so
I'd rather keep it at one place and expose it as a public API.

>> It's in actual use if you look at the definition of substitute:
>>
>>
>>   (let ((rx+proc (map (match-lambda
>>                         (((or (? regexp? pattern) (? regexp*? pattern)) . proc)
>>                          (cons pattern proc))
>>                         ((pattern . proc)
>>                          (cons (make-regexp* pattern regexp/extended) proc)))
>>                       pattern+procs)))
>>
>> The previous version followed a different approach, annotating the
>> rx+proc list with the raw pattern; I think the approach here is a bit
>> cleaner, and it should also enable users to pass pre-computed regexp*
>> objects to substitute* and have useful error messages produced.
>
> Note that ‘substitute*’ only takes string literals as patterns, not
> regexp objects.  The pattern part of clauses has sometimes been abused
> to pass code, typically ‘string-append’ calls, but that’s definitely not
> the spirit.
>

It's true that 'substitute*' only accepts literal patterns, but that's
not the case with 'substitute' itself, which is also exposed in the API
of (guix build utils): it accepts regexp objects as well, and thus
there's value in this change -- users could in theory use make-regexp*
objects and pass that to 'substitute' and benefit from more precises
error messages.

> Another approach would have been, in ‘substitute*’, to capture the
> regexp-as-string as well as other useful debugging info, such as its
> source location.

That'd be a nice solution, but it doesn't take into account
'substitute', as mentioned above.  As it has exactly zero (!) users
outside of substitute*, perhaps it could be yanked from the public API
and then we could consider doing the above for substitute* ?

-- 
Thanks,
Maxim