latex3 / latex2e

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

[amsmath] Using \Umathaccent can cause "missing \begin{document}" error #727

Open ckosowsky opened 2 years ago

ckosowsky commented 2 years ago

Brief outline of the bug

Outline:

Once amsmath redefines one of the standard accent macros by calling \@tempc on line 765 of amsmath.sty, the package cannot use \@tempd correctly on line 768 to print a warning if it sees another accent macro that was (re)defined using \Umathaccent. Instead, the result is a missing \begin{document} error.

Problem:

On line 776, amsmath.sty defines \@tempd to print a message, so this is presumably the intended behavior of the macro throughout lines 780-794. However, when amsmath redefines one of the accent commands/arguments of \@tempa, it calls \@tempc, which redefines \@tempd using \chardef on line 775. At this point, \@tempd will no longer print a message, which means that if amsmath tries to call \@tempd on line 768 when it sees another accent defined with \Umathaccent, \@tempd is an unexpandable \char value rather than the original \PackageWarningNoLine behavior.

Suggested solutions:

Any of the following will work.

  1. Put the definition of \@tempc on line 773 inside a group because \set@mathaccent is \global. But you'd have to make the accent command robust outside the group, so this would take a little bit of rewriting. Alternatively, you could say \protected\xdef inside \set@mathaccent instead of making it robust, and then you wouldn't need to change any of the code to accommodate grouping. But perhaps this would be bad for compatibility with other packages? I'm not sure. Using \protected\xdef instead of \MakeRobust is potentially related to this (now closed) bug report.
  2. Rename both instances of \@tempd on line 775.
  3. Rename both instances of \@tempd on lines 768 and 776.

Minimal example showing the bug

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

%% The following is the definition of \tilde:

\show\tilde

%% The following code replaces \mathaccent 
%% with \Umathaccent:

\makeatletter
\xdef\tilde{%
  \Umathaccent"\mathchar@type\mathalpha"\number\symoperators"7E\relax}
\show\tilde % definition is same as above

%% But loading amsamth produces a "missing
%% \begin{document}" error as well as a bunch
%% of other secondary issues:

\usepackage{amsmath}

\begin{document}

\end{document}

Log file (required) and possibly PDF file

ams-bug.log

FrankMittelbach commented 2 years ago

Your log shows that your LaTeX version

This is LuaTeX, Version 1.10.0 (TeX Live 2019)  (format=lualatex 2019.5.8)  11 DEC 2021 20:40
 restricted system commands enabled.
 file:line:error style messages enabled.
**ams-bug.tex
(./ams-bug.tex
LaTeX2e <2018-12-01>

is 3 years behind the current release which is LaTeX2e <2021-11-15> patch level 1 and the latter doesn't show an error. (but your redefinition is not identical with just \mathaccent replaced with \Umathaccent)

ckosowsky commented 2 years ago

Sorry about the out-of-date version! Here is code that shows the same bug in the most recent release. In this case, the problem arises when we redefine the command that actually produces the accent, e.g. \tilde<space>.

Minimal Example

\RequirePackage{latexbug}
\documentclass{article}

%% The following is the definition of \tilde

\show\tilde

%% The command \tilde<space> is responsible
%% for actually typesetting the accent 
%% character

\expandafter\show\csname tilde \endcsname

%% We can replace \mathaccent in \tilde<space> 
%% with \Umathaccent

\makeatletter
\expandafter\xdef\csname tilde \endcsname{%
  \Umathaccent"\mathchar@type\mathalpha"\number\symoperators"7E\relax}

%% The definition is otherwise the same as before

\expandafter\show\csname tilde \endcsname

%% But loading amsmath produces a "missing
%% \begin{document}" error as well as a bunch
%% of other secondary issues

\usepackage{amsmath}

\begin{document}

\end{document}

Log File

ams-bug-1.log

davidcarlisle commented 2 years ago

thanks for your report the idiom of re-using \@temp... scratch macro names really comes from a previous era when we needed to avoid filling the csname hash table, so as you comment we could simply rename them, however \@tempd here is really just called in one place , in the Umathaccent branch so I think it would make sense not to rely on a definition made at load time at all and just locally define it before use something like

\def\@tempb#1>#2#3 #4\@nil#5{%
  \@xp\ifx\csname#3\endcsname\mathaccent
    \@tempc#4?"7777\@nil#5%
  \else
  \@xp\ifx\csname#3\endcsname\Umathaccent
     \def\@tempd##1\@nil##2{%
    \PackageWarningNoLine{amsmath}{%
      Unable to redefine \string\Umathaccent\space\string##2}%
     }%
    \@tempd#4\@nil#5%
  \else
    \PackageWarningNoLine{amsmath}{%
      Unable to redefine math accent \string#5}%
  \fi\fi}
ckosowsky commented 2 years ago

Yeah, that makes sense. Looking at your code, I'm wondering would it work to get rid of \@tempd for the \Umathaccent message altogether? Then the second \ifx branch would look like the third branch

\def\@tempb#1>#2#3 #4\@nil#5{%
  \@xp\ifx\csname#3\endcsname\mathaccent
    \@tempc#4?"7777\@nil#5%
  \else
    \@xp\ifx\csname#3\endcsname\Umathaccent
      \PackageWarningNoLine{amsmath}{%
        Unable to redefine \string\Umathaccent\space\string#5}%
    \else
      \PackageWarningNoLine{amsmath}{%
        Unable to redefine math accent \string#5}%
    \fi
  \fi}
davidcarlisle commented 2 years ago

I need to check the notes from when that was added, as it is, really we could skip the test for \Umathaccent altogether and just give the generic "unable to redefine math accent" warning.

I think the intention (clearly not implemented) was that the \Umathaccent branch mimiced the \mathaccent branch and did something other than warn, the dtx file hints that these definitions are somewhat incomplete

% \changes{v2.15}{2016/02/20}{Detect \cs{Umathaccent} on package load}
% Adjust the test made at package load to recognise
% |\Umathaccent|. Although currently it is just used to give a
% modified warning that the accents will not be redefined.
%
% Note that the engines behave quite differently here, luatex
% even without these definitions using the OpenType accent set up by
% unicode-math  stacks |\hat{hat{f}}| correctly but xetex acts like
% classic tex and needs this adjustment. This difference is not
% addressed here at all.
ckosowsky commented 2 years ago

So the original intention was to write, say, \UmathaccentV for XeTeX that would work the same way as \mathaccentV except with unicode? I could see that being useful, although I mostly use LuaTeX, so it's not much of an issue for me personally. But in the absence of a way to implement this, I think skipping the test for \Umathaccent makes sense too. I'm not familiar enough with \mathaccentV to have an idea about how to adapt it for unicode.

ckosowsky commented 2 years ago

Well, assuming it's more complicated than replacing each \mathaccent with \Umathaccent and adding double quote marks before the math family and character codes. If it is that simple, then it's just a matter of parsing the \Umathaccent definition and putting your numeric inputs for the new \Umathaccent in the right place syntactically.

But it looks like \mathaccentV has some sort of nested accent evaluation happening that I don't completely understand. That could get messy if the macro is written in such a way that \mathaccent and \Umathaccent could get substituted for one another inside the nested evaluation. If that's not a worry, then creating a \UmathaccentV would be doable without too much trouble.

I'm happy to leave the conversation here or keep discussing \mathaccentV if you'd like.

davidcarlisle commented 2 years ago

Thanks for the report, will leave this open, and hope to find time to investigate over the holiday period.

stale[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity.

FrankMittelbach commented 2 years ago

next holiday period will come eventually :-)

stale[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity.

github-actions[bot] commented 10 months ago

This issue has been automatically marked as stale because it has not had recent activity.