latex3 / latex2e

The LaTeX2e kernel
https://www.latex-project.org/
LaTeX Project Public License v1.3c
1.93k stars 267 forks source link

Commands with optional argument as last argument cannot be used in argument of `array`'s `>{...}` #1468

Closed dbitouze closed 1 week ago

dbitouze commented 1 month ago

Brief outline of the bug

Commands with optional argument as last argument cannot be used in argument of array's >{...}

Minimal example showing the bug

\RequirePackage{latexbug}
\documentclass{article}
\NewDocumentCommand\foo{o}{x}
\usepackage{array}
\begin{document}
\begin{tabular}{>{\foo}l}
  Foo
\end{tabular}
\end{document}

Log file (required) and possibly PDF file

This is pdfTeX, Version 3.141592653-2.6-1.40.26 (TeX Live 2024) (preloaded format=latex 2024.9.10)  15 SEP 2024 21:18
entering extended mode
 restricted \write18 enabled.
 %&-line parsing enabled.
**tagpdf.tex
(./tagpdf.tex
LaTeX2e <2024-06-01> patch level 2
L3 programming layer <2024-08-30>
(/usr/local/texlive/2024/texmf-dist/tex/latex/latexbug/latexbug.sty
Package: latexbug 2024/06/17 v1.0o Bug-classification
)
(/usr/local/texlive/2024/texmf-dist/tex/latex/base/article.cls
Document Class: article 2024/02/08 v1.4n Standard LaTeX document class
(/usr/local/texlive/2024/texmf-dist/tex/latex/base/size10.clo
File: size10.clo 2024/02/08 v1.4n Standard LaTeX file (size option)
)
\c@part=\count194
\c@section=\count195
\c@subsection=\count196
\c@subsubsection=\count197
\c@paragraph=\count198
\c@subparagraph=\count199
\c@figure=\count266
\c@table=\count267
\abovecaptionskip=\skip49
\belowcaptionskip=\skip50
\bibindent=\dimen141
)
(/usr/local/texlive/2024/texmf-dist/tex/latex/tools/array.sty
Package: array 2024/06/14 v2.6d Tabular extension package (FMi)
\col@sep=\dimen142
\ar@mcellbox=\box52
\extrarowheight=\dimen143
\NC@list=\toks17
\extratabsurround=\skip51
\backup@length=\skip52
\ar@cellbox=\box53
)
(/usr/local/texlive/2024/texmf-dist/tex/latex/l3backend/l3backend-dvips.def
File: l3backend-dvips.def 2024-05-08 L3 backend support: dvips
\l__pdf_internal_box=\box54
\l__pdf_backend_content_box=\box55
\l__pdf_backend_model_box=\box56
\g__pdf_backend_annotation_int=\count268
\g__pdf_backend_link_int=\count269
\g__pdf_backend_link_sf_int=\count270
)
(./tagpdf.aux)
\openout1 = `tagpdf.aux'.

LaTeX Font Info:    Checking defaults for OML/cmm/m/it on input line 5.
LaTeX Font Info:    ... okay on input line 5.
LaTeX Font Info:    Checking defaults for OMS/cmsy/m/n on input line 5.
LaTeX Font Info:    ... okay on input line 5.
LaTeX Font Info:    Checking defaults for OT1/cmr/m/n on input line 5.
LaTeX Font Info:    ... okay on input line 5.
LaTeX Font Info:    Checking defaults for T1/cmr/m/n on input line 5.
LaTeX Font Info:    ... okay on input line 5.
LaTeX Font Info:    Checking defaults for TS1/cmr/m/n on input line 5.
LaTeX Font Info:    ... okay on input line 5.
LaTeX Font Info:    Checking defaults for OMX/cmex/m/n on input line 5.
LaTeX Font Info:    ... okay on input line 5.
LaTeX Font Info:    Checking defaults for U/cmr/m/n on input line 5.
LaTeX Font Info:    ... okay on input line 5.
LaTeX Font Info:    External font `cmex10' loaded for size
(Font)              <7> on input line 6.
LaTeX Font Info:    External font `cmex10' loaded for size
(Font)              <5> on input line 6.

! Missing { inserted.
<inserted text> 
                {
l.8 \end{tabular}

? s
OK, entering \scrollmode...
! Missing { inserted.
<inserted text> 
                {
l.8 \end{tabular}

I've put in what seems to be necessary to fix
the current column of the current alignment.
Try to go on, since this might almost work.

! Missing } inserted.
<inserted text> 
                }
l.8 \end{tabular}

I've inserted something that you may have forgotten.
(See the <inserted text> above.)
With luck, this will get me unwedged. But if you
really didn't forget anything, try typing `2' now; then
my insertion and my current dilemma will both disappear.

! Missing } inserted.
<inserted text> 
                }
l.8 \end{tabular}

I've inserted something that you may have forgotten.
(See the <inserted text> above.)
With luck, this will get me unwedged. But if you
really didn't forget anything, try typing `2' now; then
my insertion and my current dilemma will both disappear.

[1

] (./tagpdf.aux)
 ***********
LaTeX2e <2024-06-01> patch level 2
L3 programming layer <2024-08-30>
 ***********
 ) 
Here is how much of TeX's memory you used:
 1875 strings out of 473580
 44600 string characters out of 5728260
 413935 words of memory out of 5000000
 24847 multiletter control sequences out of 15000+600000
 558069 words of font info for 36 fonts, out of 8000000 for 9000
 1141 hyphenation exceptions out of 8191
 35i,6n,50p,163b,1346s stack positions out of 10000i,1000n,20000p,200000b,200000s

Output written on tagpdf.dvi (1 page, 268 bytes).
FrankMittelbach commented 1 month ago

Not claiming I understand the issue here yet, but it looks as if it has to do with the way \NewDocumentCommand is doing the parsing, if you use \newcommand\foo[1][]{x} instead it does through, so I suspect it has to do with how this tries to be align-safe (and that doesn't quite work if it happens as part of the array preamble code).

@josephwright / @davidcarlisle can you perhaps take a look?

davidcarlisle commented 1 month ago

I can look, no promises though, as I said in the original issue it's a tricky area:-)

FrankMittelbach commented 1 month ago

Technically, it might be enough if we put a \relax at the end of the preamble tokens so that scanning is stopped, i.e., before the \ignorespaces but I would like really to understand first what is going on and for now I don't

davidcarlisle commented 1 month ago

You can't scan the end template while inside the "safe" group, this works forcing the endtemplate to be seen earlier, but the trailing optional argument can not easily be used even then as there is a lot of backage to lose before the [ at the start of the content is seen.

Given that the optional argument can't easily work, I'm not sure we should try too hard to avoid the error.

\documentclass{article}

\NewDocumentCommand\foo{O{default}}{x with #1}
\usepackage{array}
\begin{document}

%\begin{tabular}{>{\foo}l}
\begin{tabular}{>{\expandafter\foo}l}
  Foo\\[0pt]
  [option]Foo2
\end{tabular}
\end{document}
FrankMittelbach commented 1 month ago

What I don't quote comprehend right now is why it hits \endtemplate during parsing for [ and not the content of the cell and then stop at the F there. What am I not seeing here?

davidcarlisle commented 1 month ago

hmm I tried to make an \halign example but failed so it's not quite as I thought, will repost something later

Skillmon commented 1 month ago

It can be fixed using collcell (I know, not really a solution, but a valid workaround):

\documentclass{article}

\NewDocumentCommand\foo{O{default}}{x with #1}
\NewDocumentCommand\fooAux{m}{\foo#1}
\usepackage{array,collcell}
\begin{document}

\begin{tabular}{>{\collectcell\fooAux}l<{\endcollectcell}}
  Foo\\[0pt]
  [option]Foo2
\end{tabular}
\end{document}
FrankMittelbach commented 1 month ago

@Skillmon that moves the execution of \foo into the cell body where it doesn't hit \endtemplate. But a far simpler workaround is >{\foo\relax}.

davidcarlisle commented 1 month ago

@FrankMittelbach the difference is that @Skillmon's version actually allows the optional argument (if the cell content starts [option] whereas \relax avoids the error but also ensures the option is never taken. ( as would the \ignorespaces that normally follows the >{...} tokens actually). So it depends what we think should happen

FrankMittelbach commented 1 month ago

well I don't think supporting that directly makes much sense (it is not as if we support picking up any data through an argument of a command placed at the end of >{...} so why doing that for an optional arg? So I'm for putting \relax in front of \ignorespacesand be done with it.

davidcarlisle commented 1 month ago

OK with me (and it works) but now I'm confused, \ignorespaces is a non expandable primitive so why doesn't it stop any lookahead in the same way as \relax

josephwright commented 1 month ago

OK with me (and it works) but now I'm confused, \ignorespaces is a non expandable primitive so why doesn't it stop any lookahead in the same way as \relax

\ignorespaces is part of the template, and one has to be careful with that.

josephwright commented 1 month ago

A couple of notes:

  1. The reason we have the code to cope with & at all is that it allows [...&...] - see https://github.com/latex3/latex3/issues/839
  2. I have similar issues in siunitx, where I have to manually pick up \ignorespaces when looking ahead
  3. With hindsight, siunitx's S-column would simply take a mandatory argument - and I suggest that's the better steer here - 'don't use ending optional args in tabular preambles'
davidcarlisle commented 1 month ago

OK with me (and it works) but now I'm confused, \ignorespaces is a non expandable primitive so why doesn't it stop any lookahead in the same way as \relax

\ignorespaces is part of the template, and one has to be careful with that.

Yes but adding relax to the preamble just before the ignorespaces prevents the lookahead hitting the endtemplate but current I don't see why.

josephwright commented 1 month ago

OK with me (and it works) but now I'm confused, \ignorespaces is a non expandable primitive so why doesn't it stop any lookahead in the same way as \relax

\ignorespaces is part of the template, and one has to be careful with that.

Yes but adding relax to the preamble just before the ignorespaces prevents the lookahead hitting the endtemplate but current I don't see why.

It stops the look ahead picking up anything from the template, that's the point: if you hit the \ignorespaces you have to handle differently (again, see inside siunitx ...)

davidcarlisle commented 1 month ago

@josephwright I think you are describing >{\foo\relax] that is the easy case, my question was why adding \relax to the template stops lookahead when \ignorespaces does not.

davidcarlisle commented 1 month ago

as it is, eventually you end up at

~...\@xarraycr ->\@ifnextchar [\@argarraycr {\ifnum 0=`{}\fi \cr }

but the \cr triggers the end cell template and you get as next step

~...\@ifnextchar #1#2#3->\let \reserved@d =#1\def \reserved@a {#2}\def \reserve
d@b {#3}\futurelet \@let@token \@ifnch 
#1<-[
#2<-\@argarraycr 
Runaway argument?
{\ifnum 0=`{}\fi \textonly@unskip \relax \UseTaggingSocket {tbl/cell/\ETC.
! Forbidden control sequence found while scanning use of \@ifnextchar.
<inserted text> 
                \par 
<to be read again> 
                   \endtemplate 

but with \relax you have one extra layer of safe group protection and the \cr does not fire and the next step is

~...\@ifnextchar #1#2#3->\let \reserved@d =#1\def \reserved@a {#2}\def \reserve
d@b {#3}\futurelet \@let@token \@ifnch 
#1<-[
#2<-\@argarraycr 
#3<-\ifnum 0=`{}\fi \cr 
{\let}

ah, the diff is

$ diff array.sty~ array.sty
99c99
<    \the@toks \the \@tempcnta
---
>    \the@toks \the \@tempcnta\relax

so I don't think it is \relax before \ignorespaces so much as \relax after \the \@tempcnta which is reducing the lookahead.

dbitouze commented 1 month ago

3. and I suggest that's the better steer here - 'don't use ending optional args in tabular preambles'

But what about commands whose final argument is an optional one, used without an optional argument as their final argument one, just as \fontspec{⟨font⟩}[⟨font features⟩]?

See e.g. https://github.com/latex3/tagging-project/issues/707.

josephwright commented 1 month ago
  1. and I suggest that's the better steer here - 'don't use ending optional args in tabular preambles'

But what about commands whose final argument is an optional one, used without an optional argument as their final argument one, just as \fontspec{⟨font⟩}[⟨font features⟩]?

See e.g. latex3/tagging-project#707.

Doesn't change what I said, really

davidcarlisle commented 1 month ago
  1. and I suggest that's the better steer here - 'don't use ending optional args in tabular preambles'

But what about commands whose final argument is an optional one, used without an optional argument as their final argument one, just as \fontspec{⟨font⟩}[⟨font features⟩]?

See e.g. latex3/tagging-project#707.

they will work after the change. try adding \relax on line 99 of array.sty as in the above diff

davidcarlisle commented 1 month ago

@dbitouze or rather than modify the package, after loading array do

\makeatletter
\def\insert@column{%
  \UseTaggingSocket{tbl/cell/begin}%
   \the@toks \the \@tempcnta\relax
   \ignorespaces \@sharp \textonly@unskip
   \the@toks \the \count@ \relax
   \UseTaggingSocket{tbl/cell/end}%
}
\makeatother
u-fischer commented 1 month ago

@davidcarlisle and probably something similar for \insert@pcolumn?

davidcarlisle commented 1 month ago

@u-fischer ooh probably, yes

FrankMittelbach commented 1 month ago

I made the PR already last night but didn't upload yet

Skillmon commented 1 month ago

That change (adding \relax) breaks collcell (just pointing it out). Well, breaking is not really correct, collcell now "just" needs to strip the extra \relax, still requires a change by collcell.

davidcarlisle commented 1 month ago

@Skillmon hmm

\def\insert@column{%
  \UseTaggingSocket{tbl/cell/begin}%
   \the@toks \the \@tempcnta
   \ignorespaces\space \@sharp \textonly@unskip
   \the@toks \the \count@ \relax
   \UseTaggingSocket{tbl/cell/end}%
}

?

u-fischer commented 1 month ago

That change (adding \relax) breaks collcell (just pointing it out)

@Skillmon Do you have an example? For me it still seems to work fine.

Skillmon commented 1 month ago

@u-fischer just take a look at the contents, it starts with the \relax that was injected. So it doesn't outright break, but depending on what you intend to do with it, it might break a document.

josephwright commented 1 month ago

@Skillmon I need to sort a patch for collcell to send to Martin - I could cover as part of that

Skillmon commented 1 month ago

@josephwright yes, I think that would be great (sorry I didn't make it, didn't find the time yesterday evening -- not because I was busy or something just got distracted)