Handle \relax and frozen \relax in tikz path

[muzimuzhi]

Motivation for this change

As suggested in #966 (comment), this PR makes \relax and the frozen \relax handled in tikz path parsing. An intro to frozen \relax can be found in this TeX-SX answer and texdoc interface3, sec. XVI.7.

PS: Perhaps \tikz@frozen@relax@token could be put in pgfutil-common.tex.

Fixes #966 (or, implements)

Checklist

Please check the boxes to explicitly state your agreement to these terms:

• Code changes are licensed under GPLv2 + LPPLv1.3c
• Documentation changes are licensed under FDLv1.2

[hmenke]

Let's see what the CI thinks about this.

[muzimuzhi]

Let's see what the CI thinks about this.

CI triggered by (isomorphic) commit hmenke@cb425a5 just gets passed.

Discussions about code style here can still continue and I am willing to do a rebasing followed by force pushing at the end.

[muzimuzhi]

CI / build (dvisvgm) fails when setting up texlive environment, see https://github.com/pgf-tikz/pgf/pull/967/checks?check_run_id=1591472106#step:4:10

/tmp/texlive/bin/x86_64-linux/tlmgr: TLPDB::from_file could not initialize from: http://mirrors.ibiblio.org/pub/mirrors/CTAN/systems/texlive/tlnet/tlpkg/texlive.tlpdb
/tmp/texlive/bin/x86_64-linux/tlmgr: Maybe the repository setting should be changed.
/tmp/texlive/bin/x86_64-linux/tlmgr: More info: https://tug.org/texlive/acquire.html
Error: Process completed with exit code 1.

The CI for the same commit on my forked branch is ok. Guess this failure is caused by network problems.

[ilayn]

restarted just to double check

[hmenke]

I had to revert this. It broke circuitikz.

[muzimuzhi]

Could you provide a specific broken example?

I locally compiled the latest manual of circuitikz with pgf 3.1.8 (by removing the -interaction=nonstopmode in its Makefile and then executing make manual-git), the compilation passed. The to path issue circuitikz/circuitikz#460 you commented also seemed to be irrelevant to this PR.

[hmenke]

Sure, sorry. The MWE was only communicated privately by @Rmano.

\documentclass[border=10pt]{standalone}
\usepackage[siunitx, RPvoltages]{circuitikz}
\begin{document}
\pgfcircversion{} on \pgfversion
\begin{circuitikz}[american]
    \draw (0,0) to [
        R=R,
        v=V,
        i=I,
        f=F,
        ] (3,0);
\end{circuitikz}
\end{document}

bisection

$ git bisect start
$ git bisect bad 3.1.8
$ git bisect good 3.1.7a
$ TEXMFHOME=/path/to/pgf git bisect run pdflatex --interaction=nonstopmode test.tex
[...]
79e613ae139f4e16f1f991114fe1a001d0dd9dab is the first bad commit
commit 79e613ae139f4e16f1f991114fe1a001d0dd9dab
Author: muzimuzhi <muzimuzhi@gmail.com>
Date:   Tue Dec 22 05:24:18 2020 +0800

    tikz: handle \relax and frozen \relax on path #966

 tex/generic/pgf/frontendlayer/tikz/tikz.code.tex | 27 +++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)
bisect run success

[Rmano]

@muzimuzhi @hmenke
If you check 1.2.7 it does not break anymore, but my solution is more handwaving than other thing. The change that fixed the breakage was circuitikz/circuitikz@14a8f3a - adding groups around the node-putting instructions. Using expandable ifs as suggested by @hmenke did not solve the problem.

@muzimuzhi if you check out circuitikz 1.2.6, you can see the thing that puzzled me: if in the MWE you comment out just one of the decorations it will work. Just using the four of them (l= which is the same as giving the parameter to R, i=, f=, and v=) breaks. Comment one of them and it runs through.

I tried to debug the core problem but it exceeds my TeX skills, and found almost by chance that an extra scope solved (or masked?) the problem.

I am a bit busy today but I can help debugging.

[hmenke]

@Rmano No, I don't want you to add workarounds for PGF regressions. Please always report them, because you are most likely not the only one who is affected.

[Rmano]

Ok, let's see if I can understand why it broke, will remove the workaround in the next minor. My hunch, probably wrong, is that it was hitting some expansion limit, like the famous \tikz \node{\boldmath $x$}; (see https://tex.stackexchange.com/questions/487777/why-boldmath-fails-in-a-tikz-node); due to that hunch I tried to add groups --- and it worked.

[muzimuzhi]

The MWE was only communicated privately by @Rmano.

@hmenke If you referred to the chats on TeX-SX, they're public. See https://chat.stackexchange.com/transcript/message/56560034#56560034.

I think the real cause is a hidden problem that \tikz@expandcount is not reset in cases handled by \tikz@handle, which was made much more easier to trigger by introducing \tikz@handle@evenmore. See a possible fix in muzimuzhi@0f0b925, which still contains some bit of debugging code. (And I'm not sure if the beginning \let\pgfutil@next\tikz@expand in \tikz@handle is needed, though CI didn't complain about the deletion.)

The (expected) logic in \tikz@handle is,

• if the collected token is handled, then reset \tikz@expandcount to 100 and continue with specific commands;
• otherwise, reduce that count by 1, put back the collected token and do one-step expansion on it, re-collect it, and finally try \tikz@handle on it again.
• if the count is lower than zero after reducing, throw an error.

By introducing \tikz@handle@more, the cases handled by \tikz@handle never reset the count. This has already buried a small hidden danger. By introducing \tikz@handle@evenmore, almost every handled cases, except for the two newly added ones, will not reset the count. The difference gets accumulated by occasional one-step expansions, and eventually explode in a complex (as of its internal expansions, not the user input) enough circuitikz example. Since the change to\tikz@expandcount is local, adding an extra group to some part of the tikz path happens to solve the problem.

Some intermediate results that helped me when digging the problem:

• circuitikz does not generates either forms of \relax.
• Adding new \ifx...\else...\fis in \tikz@handle@more is ok, but introducing \tikz@handle@evenmore is buggy.
• The value of \tikz@expandcount at the beginning of \tikz@expand is frequently reset before, but never/seldom gets reset when this PR is applied.

[hmenke]

@muzimuzhi If you want we can try again with your new patch. However, after thinking a bit more about this, encountering \relax or frozen \relax on the path is usually a sign of a different problem in the code that the user wrote. I'm not so sure whether we should really handle it in this way. It's probably better to error out with a more specific error in these cases. Currently it throws Did you forget a semicolon? which often is not the case at all.

[Rmano]

So, if I have understood correctly: that mechanism you describe is a safety mechanism to avoid infinity recursion --- but the counter was not reset so that using more than three "decorations" hit the limit value. So, adding the extra grouping made the changes to the counter local, and so it did not trigger the limit. Am I correct?

[hmenke]

@Rmano That describes it pretty accurately. You skipped the point where I fucked it up, though.

[Rmano]

@Rmano That describes it pretty accurately. You skipped the point where I fucked it up, though.

:-; you can try to blame somebody else like in the chat, seems quite standard :-P

[muzimuzhi]

Two discussions are mixed up. One for questioning why pgf 3.1.8 + circuitikz 1.2.6 is broken for some example, and another one for whether tikz should silently skipping \relax and frozen \relax in path handling.

The first one is what @Rmano is interested/involved in, if not the only one. And its cause is already uncovered. There is nothing wrong in circuitikz 1.2.6, but something wrong in pgf, especially 3.1.8. And that cause could have nothing to do with current PR if we adjusted the way to implement, for example the one originally posted in #966 (comment). Relax, it's ok if the second one didn't attract you.

The second one is what this PR wanted to address.

@hmenke My original purpose was to i) make the similar logic written in \foreach ... {\draw ...;} and \draw foreach ...; either both work or both not work, and ii) make the normal texnique \if(num|dim)\x=\y\relax works in foreach path operation, because as I could imagine, explaining frozen \relax or adding \dimexpr to users is more difficult than just \relax. The remained question is, as you pointed out, whether auto ignoring \relax will cover any real problems, rather than ease minor ones. This also relates to the error message concern. Another consideration is, should the complex/nested foreach path operation (not the \foreach command used outside tikz path) be encouraged or discouraged. (I may have encountered the \relax problem before, and resolved by rewriting codes to \foreach ... {\draw ...}.)

I don't have a strong opinion on both of them (about second one).

[muzimuzhi]

However, after thinking a bit more about this, encountering \relax or frozen \relax on the path is usually a sign of a different problem in the code that the user wrote.

In short, need some examples.

[hmenke]

I think we can handle \relax in a reasonable way by stepping over it without removing it. It just came to my mind that with the original patch \relax was actually discarded. That might or might not be a problem. From my point of view it's also really hard to argue what's the correct behavior here, because we're somewhere in the middle of a complex expansion chain. Another big problem here is actually \expandafter\tikz@scan@next@command\pgf@let@token which will happily expand \protected material. Maybe you could open a new PR with your revised patch and then we can work on it together.

[muzimuzhi]

Another big problem here is actually \expandafter\tikz@scan@next@command\pgf@let@token which will happily expand \protected material.

Aha, seems another call for latex3 expandable macros \token_if_xxx implemented in pgf. Not sure if I correctly got your point. To solve the problem piece by piece, firstly I opened PR #970 that solely addresses the \tikz@expandcount issue.