Fix incorrect handling of user-defined environments introduced in #856 (#1137)

[dpvc]

This PR fixes the problem with user-defined environments that was reported in #1137 that was introduced by PR #856, that was trying to fix a potential infinite recursion. It changed how the environments end macros were processed by inserting them into the parser string followed by second \end{...} macro. A parser.stack.env value (processing) was used to track when the second \end{...} was processed and skip the insertion of the end macros.

The problem with that is that the environment where processing was added might be cleared by the ed macros. For the example in #1137, the \begin{array} in the begin macros and the \end{array} in the end macros for the boxed environment means that the processing environment value is in the environment in effect within the array environment, and that environment is ended and removed when \end{array} is processed. So the processing value is also lost, and the end macros are inserted again, leading to a new \hline being inserted outside the array environment, and the associated error about a misplaced \hline.

Because of this, and other similar situations, the parser.stack.env value can't be used to track the second \end{...}. So I've gone back to the original process of parsing the end macros, but needed a new way to avoid the potential recursion that #856 was meant to fix.

The infinite recursion was possible when the end macros included \end{...} for the user-defined environment. This was a problem because when processing the end macros that contain \end{...} that \end would cause the end macros to be inserted again, and then that is related over and over. This is because the testing for a matching \begin{...} for the \end{...} isn't done until after the end macros are processed (so that the user-defined environment can include \begin and \end as in the example from #1137), when the end stack item is pushed.

The solution introduced here is use the beginEnv stack items to form a linked list of the open (user-defined) environments, with the the top-most beginEnv stack item recorded in a parser.stack.global variable. We only insert the end macro string if there is an active beginEnv on the stack, and we remove that from the linked list once we add the end macros the first time. That way, we only add the end macros once for any \begin{...} that is on the stack, so even if the end macros include \end{...}, they can't be inserted infinitely, and eventually we get the end stack item being pushed, producing the mismatched begin/end message.

This allows

\newenvironment{boxed}
    {\begin{array}{|c|c|}\hline}
    {\\\hline\end{array}}
\begin{boxed}a&b\\c&d\end{boxed}

and also

\newenvironment{boxed}
    {\begin{array}{|c|c|}\hline}
    {\\\hline\end{array}}
\begin{boxed}
\begin{boxed}a&b\\c&d\end{boxed}
& X
\end{boxed}

to work, while having

\newenvironment{a}{x}{y\end{a}}
\begin{a} ... \end{a}

produce an appropriate error message.

Other test cases include

\newenvironment{a}{x}{y}
\newenvironment{b}{p}{q}
\begin{a} \begin{b} ... \end{b} \end{a}

that should work while

\newenvironment{a}{x}{y}
\newenvironment{b}{p}{q}
\begin{a} \begin{b} ... \end{a} \end{b}

and

\newenvironment{a}{x}{y}
\begin{a} \begin{matrix} ... \end{a}

and

\newenvironment{a}{x}{y}
\begin{a} ... \end{matrix}

and

\newenvironment{a}{x}{y}
... \end{a}

and

\newenvironment{a}{a}{b\end{a}}
\newenvironment{b}{x}{y}
\begin{a} \begin{b} ... \end{b} \end{a}

and

\newenvironment{a}{a}{b\end{a}}
\newenvironment{b}{x}{y}
\begin{b} \begin{a} ... \end{a} \end{b}

should all produce errors.

The definitions

\newenvironment{a}{\begin{b}}{\end{b}}
\newenvironment{b}{x}{y}
\begin{a} ... \end{b}\begin{b}\end{a}

should produce x...yxy,

while

\newenvironment{a}{\begin{b}}{\end{b}}
\newenvironment{b}{x}{y\end{b}}
\begin{a} ... \end{a}

should error.

[dpvc]

It turns out we don't need the linked list, just a count of the open user-defined environments, so I've made a commit that changes that. I had originally tried to use the beginItem to store information about what \end was being processed, but that didn't work in some cases, and so removed it, but didn't realize the linked list was no longer needed until I had set it aside for a while.

[zorkow]

It turns out we don't need the linked list, just a count of the open user-defined environments, so I've made a commit that changes that. I had originally tried to use the beginItem to store information about what \end was being processed, but that didn't work in some cases, and so removed it, but didn't realize the linked list was no longer needed until I had set it aside for a while.

Isn't it a bit early for you?
I had a go at this yesterday morning as well. My solution involves a list of previously opened custom environments and an error that flags recursion. While it only adds one new property, its drawback is that it would need to change the property type allowing for arrays.

But it allowed me to put together quite a number of test cases. Most work with your solution. Here is one, where the error message is not quite what is expected.

\newenvironment{c}{x}{\end{a}y}
\newenvironment{b}{\begin{c}x}{\end{c}y}
\newenvironment{a}{\begin{b}x}{\end{b}y}
\begin{a} ... 

But these are quite pathological cases, so I would not be too fuzzed about them.

[dpvc]

Isn't it a bit early for you?

Yes, I couldn't sleep, and thought I might as well get some work done.

I had a go at this yesterday morning as well. My solution involves a list of previously opened custom environments and an error that flags recursion. While it only adds one new property, its drawback is that it would need to change the property type allowing for arrays.

It sounds like you were doing essentially the same as mine. There have been several times I'd have liked to change the type for EnvList to include more complex objects, I went with the linked list to avoid that. But, as I found out, neither is actually needed for this situation.

But it allowed me to put together quite a number of test cases. Most work with your solution. Here is one, where the error message is not quite what is expected.

\newenvironment{c}{x}{\end{a}y}
\newenvironment{b}{\begin{c}x}{\end{c}y}
\newenvironment{a}{\begin{b}x}{\end{b}y}
\begin{a} ...
But these are quite pathological cases, so I would not be too fuzzed about them.

I had a similar example as the last one in my pull request description, and I was not so happy with the error message there as well. But actual LaTeX recurses infinitely for this one (and the simpler variants from my PR), so I feel better that we are at least giving a message. Also, LaTeX complains about all three definitions because \c, \b, and \a are already defined. I haven't looked closely at how the internals for LaTeX really work, but I guess it creates macros to handle the environment begin and end pieces and uses those when \begin{...} and \end{...} are reached.

In any case, if you want to use a different solution, let me know.

[zorkow]

lgtm.
Once merged I will add all the tests we have for it.

[dpvc]

Once merged I will add all the tests we have for it.

Thanks!