Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

\dots fails if followed by commands involving primitive \if #1448

Open
davidcarlisle opened this issue Aug 25, 2024 · 13 comments
Open

\dots fails if followed by commands involving primitive \if #1448

davidcarlisle opened this issue Aug 25, 2024 · 13 comments

Comments

@davidcarlisle
Copy link
Member

davidcarlisle commented Aug 25, 2024

Brief outline of the bug

The code added at #1424 to expand past a \protect definition may expose a primitive \if... token
this reacts badly when nested in the testing done by \dots.

Minimal example showing the bug

\RequirePackage{latexbug}       % <--should be always the first line (see CONTRIBUTING)!

\documentclass{article}

\usepackage{amsmath}

\newcommand\test{\iftrue x\fi}

\DeclareRobustCommand\testr{\DOTSB+}
\protected\def\testp{\DOTSB-}
\def\testd{\DOTSB*}

\begin{document}

$ x \dots \testr$

$ x \dots \testp$

$ x \dots \testd$

$x \dots \iffalse a\else b\fi $

$x \dots \test$

$x \dots \boldsymbol{y} $


\end{document}

gives

Runaway argument?
\fi \fi \thedots@ \iffalse a\else b\fi $ 
! Paragraph ended before \stripprotect@ was complete.
<to be read again> 
                   \par 
l.21 

I think the best solution in the end would be to f-expand before testing so that primitve \ifs, \protect and \protected and \long definitions would all be resolved in advance and the testing simplified, however it would be a disruptive change as the current \DOTSI, \DOTSB, .. definitions wouldn't be distinguishable (as they are all \ifx equal to \relax).

So the suggestion here is more in the spririt of the existing code, check after the first \meaning if the token is a primitive \if.. (because meaning starts \if) and if so skip the test that expands and checks for \protect.

*** amsmath.sty~	Sun Aug 25 14:24:19 2024
--- amsmath.sty	Sun Aug 25 14:44:14 2024
***************
*** 443,448 ****
--- 443,452 ----
    \@xp\Umathch@\meaning@\Umathch@
  }
  \fi
+ {\uccode`7=`\\ \uccode`(=`i \uccode`)=`f
+   \uppercase{\gdef\testif@#1#2#3#4\testif@{%
+     \ifx7#1\ifx(#2\ifx)#3\@tempswafalse\fi\fi\fi}
+ }}
  \newcount\classnum@
  \def\getmathch@#1.#2\getmathch@{\classnum@#1 \divide\classnum@4096
   \ifcase\number\classnum@\or\or\gdef\thedots@{\dotsb@}\or
***************
*** 522,531 ****
         \ifgtest@ % if \keybin@ test
           \gdef\thedots@{\dotsb@}%
         \else
         \begingroup
           \def\protect{\protect}% % make it a quark
           \xdef\meaning@{\@xp\stripprotect@\@let@token.........\stripprotect@. .........}%
!        \endgroup
           \xdef\meaning@@{\@xp\striplong@\meaning@\relax\meaning@}%
           \xdef\meaning@@{\@xp\stripprotected@\meaning@@\relax\meaning@@}%
           \@xp\math@\meaning@\math@
--- 526,540 ----
         \ifgtest@ % if \keybin@ test
           \gdef\thedots@{\dotsb@}%
         \else
+          \xdef\meaning@{\meaning\@let@token. .........}%
+          \@tempswatrue
+          \@xp\testif@\meaning@....\testif@
+          \if@tempswa
         \begingroup
           \def\protect{\protect}% % make it a quark
           \xdef\meaning@{\@xp\stripprotect@\@let@token.........\stripprotect@. .........}%
!          \endgroup
!          \fi
           \xdef\meaning@@{\@xp\striplong@\meaning@\relax\meaning@}%
           \xdef\meaning@@{\@xp\stripprotected@\meaning@@\relax\meaning@@}%
           \@xp\math@\meaning@\math@

Full .sty attached for ease of testing. (I could make a PR against the dtx source later, but possibly not this weekend)

amsmath.txt

@davidcarlisle davidcarlisle self-assigned this Aug 25, 2024
@josephwright josephwright added this to the Release 2025 Spring milestone Oct 22, 2024
@github-project-automation github-project-automation bot moved this to Pool (unscheduled issues) in upcoming LaTeX2e releases Oct 22, 2024
davidcarlisle added a commit that referenced this issue Oct 30, 2024
josephwright added a commit that referenced this issue Oct 30, 2024
* add extra meaning test for #1448

* forgot to add test file to git

* chmod

* luatex (l3build could perhaps have normalised this)

* eol eof

* document use of special uccodes for catcode 12 tokens

* Correct a missing "%"

---------

Co-authored-by: Joseph Wright <[email protected]>
@davidcarlisle
Copy link
Member Author

The fix above was released in the 2024-11-01 release but unfortunately misses one case using \protected in combination with a primitive \if.

An extended test file with an extra case (\testpi) that fails:

\documentclass{article}

\usepackage{amsmath}

\newcommand\test{\iftrue x\fi}

\protected\def\testpi{\iftrue x\fi}

\DeclareRobustCommand\testr{\DOTSB+}
\protected\def\testp{\DOTSB-}
\def\testd{\DOTSB*}

\begin{document}

$ x \dots \testr$

$ x \dots \testp$

$ x \dots \testd$

$x \dots \iffalse a\else b\fi $

$x \dots \test$

$x \dots \boldsymbol{y} $

$ x \dots \testpi$


\end{document}

It remains to be seen whether to re-adjust the \meaning tests or to bring forward the code planned for #1521

@cfr42
Copy link
Contributor

cfr42 commented Nov 3, 2024

Possibly related: #1531 (comment)

@guger
Copy link

guger commented Nov 4, 2024

The same error just occurred to me, when using \dots in a matrix environment.

Extra \fi.
\mdots@@ ...haracter@ \fi \fi \fi \fi \fi \fi \fi
\thedots@
l.61 \var{X_1} & \cov{X_1}{X_2} & \dots &
\cov{X_1}{X_N} \\

The cause was not immediately clear to me, as my custom commands \var and \cov do not include any \if:

\NewDocumentCommand\var{m}{\mathrm{var}\left(#1\right)}
\NewDocumentCommand\cov{mm}{\mathrm{cov}\left(#1,#2\right)}

Apparently, there have to be some nested \ifs. I hope, this will be ressolved with the same fix.

@davidcarlisle
Copy link
Member Author

@guger there is an \ifhmode inserted by the array package tabular code, which then fails, which is why it's enough to put anything between \dots and & so these are all the same issue. A fix will be posted shortly, but (especially as the fix that was posted with the release proved incomplete) it needs a bit more testing, but we'll push a fix (or temporarily revert the change) assoon as possible.

@guger
Copy link

guger commented Nov 4, 2024

@davidcarlisle Very interesting, thank you for the explanation!

@davidcarlisle
Copy link
Member Author

@guger after loading array if you add

\makeatletter
\protected\def\textonly@unskip{\relax\ifhmode\unskip\fi}
\makeatother

to your preamble could you confirm the problem goes (this isn't the real fix, which should be in amsmath but this adjusts array not to trigger the problem

@guger
Copy link

guger commented Nov 4, 2024

@davidcarlisle Yes, this solves the issue! Thanks!

@davidcarlisle
Copy link
Member Author

davidcarlisle commented Nov 4, 2024

@guger thanks for confirming. That change (adding \relax) is harmless so it would be Ok to leave it in your document even after an update to amsmath is rolled out, but sorry for the inconvenience.

@clason
Copy link

clason commented Nov 5, 2024

I have observed a related bug -- should I open a new issue about it? The following minimal code

\documentclass{article}
\usepackage{amsmath}

\begin{document}
  $A=\{1,2,\dots\}$
\end{document}

triggers

! TeX capacity exceeded, sorry [input stack size=10000].
\GenericWarning ...tchoice@ \else 4\fi \endcsname 
                                                  \protect \GenericWarning  
l.5 $A=\{1,2,\dots\}
                    $
!  ==> Fatal error occurred, no output PDF file produced!

(Using \ldots works fine.)

josephwright added a commit that referenced this issue Nov 5, 2024
* revert to 066ad1d and re-apply 3c68c56

* remove test file for #1448

* today's date and changes.txt entry

* Update date/version

---------

Co-authored-by: Joseph Wright <[email protected]>
josephwright added a commit that referenced this issue Nov 5, 2024
* revert to 066ad1d and re-apply 3c68c56

* remove test file for #1448

* today's date and changes.txt entry

* Update date/version

---------

Co-authored-by: Joseph Wright <[email protected]>
@cfr42
Copy link
Contributor

cfr42 commented Nov 6, 2024

@clason That looks like the same issue. Does the code @davidcarlisle posted above help?

@clason
Copy link

clason commented Nov 6, 2024

It does not, unfortunately.

@davidcarlisle
Copy link
Member Author

@clason yes sorry, that example was posted to https://tex.stackexchange.com/questions/730161/recently-added-bug-to-amsmath yesterday, it's the same issue in amsmath but as it isn't in a table the above array patch doesn't avoid it. There is a quick fix posted at the above site, In these github sources amsmath has been reverted to the June release see #1543 and we'll re-issue amsmath today. We will bring forward new code implementing the intended feature of supporting protected macros after testing with the test cases that have been raised, but we are first reverting the amsmath release to allow time for testing.

@clason
Copy link

clason commented Nov 6, 2024

Thanks, the tex.se post is indeed exactly the same. (Cue lament on the declining usefulness of Google...)

As the workaround is trivial and harmless (just add an l in those cases), it's not urgent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Pool (unscheduled issues)
Development

No branches or pull requests

5 participants