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.

Sign up for GitHub

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

Jump to bottom

Handler config 'full or existing' can set non-existent keys when '.search also' is in effect. #1143

Open
fritzw opened this issue Mar 8, 2022 · 3 comments · May be fixed by #1144
Open

Handler config 'full or existing' can set non-existent keys when '.search also' is in effect. #1143

fritzw opened this issue Mar 8, 2022 · 3 comments · May be fixed by #1144
Labels
pgfkeys

Comments

@fritzw
Copy link

fritzw commented Mar 8, 2022

Brief outline of the bug

With /handler config=full or existing, an assignment like key/.style={stuff} should only set the style if key exists in the current path or somewhere in the search paths set with .search also. However, when one or more paths are passed to .search also, and key does not exist in the current path, key/.style={stuff} will always be executed it in the first search path, regardless of whether it exists there or not. It will never fail, like it should when the key on which the handler is called does not exist.

In the example below, the first case (with color=green) appears to work. The style every foo is set in /main path, because the style did not exist in /second path. However, in the second example you can see it fail: every foo is set in /third path although it did not exist there before, despite the handler config.

Cause of the bug

If you look at the log file, you can see the cause of the issue (thanks to the patched commands):

  1. .search also executes \pgfqkeys{/third path}{every foo/.style/.try/.expand once=value} (defined in \pgfkeys@searchalso@appendentry).
  2. \pgfkeys@add@path@as@needed prepends /third path and sets \ifpgfkeysaddeddefaultpath=\iftrue (in \pgfkeys@addpath).
  3. \pgfkeys@ifexecutehandler@handlefullorexisting executes .expand once because it exempt from /handler config.
  4. .expand once executes \pgfkeysalso{/third path/every foo/.style/.try=value}.
  5. \pgfkeysalso calls \pgfkeys@add@path@as@needed, which sees an absolute path and sets \ifpgfkeysaddeddefaultpath=\iffalse (in \pgfkeys@nevermind).
  6. .try calls \pgfkeys@ifexecutehandler@handlefullorexisting with \pgfkeyscurrentpath=/third path/every foo/.style, which succeeds because \ifpgfkeysaddeddefaultpath is false.

Log file excerpt:

[...]
pgfkeys@addpath: every foo/.style
pgfkeys@addpath: every foo/.style/.try/.expand once
pgfkeysalso: /third path/every foo/.style/.try={color=blue}
pgfkeys@nevermind: /third path/every foo/.style/.try
/third path/every foo/.style={color=blue}
pgfkeys@nevermind: /third path/every foo/.code
[...]

Minimal working example (MWE)

\documentclass{article}
\usepackage{pgfkeys}

\pgfkeys{
    /main path/foo/.code={\{\pgfkeysalso{/main path/every foo} foo\}},
    /main path/every foo/.style={color=red}, % It even works if this is commented out
    /main path/color/.code=color\{#1\},
    /handler config=full or existing,
}

\usepackage{etoolbox}
\makeatletter
\pretocmd\pgfkeys@nevermind{
    \immediate\write16{pgfkeys@nevermind: \detokenize{#1}}
}{}{}
\pretocmd\pgfkeys@addpath{
    \immediate\write16{pgfkeys@addpath: \detokenize{#1}}
}{}{}
\pretocmd\pgfkeysalso{
    \immediate\write16{pgfkeysalso: \expandafter\expandafter\expandafter\detokenize\expandafter\expandafter\expandafter{#1}}
}{}{}
\pgfkeys{/handlers/.style/.code=\immediate\write16{\pgfkeyscurrentpath/.style={#1}}\pgfkeys{\pgfkeyscurrentpath/.code=\pgfkeysalso{#1}}}
\makeatother

\begin{document}
\pgfkeys{
    /second path/.cd,
    /second path/.search also={/main path},
    every foo/.style={color=green}, % no 'every foo' in /second path
    foo
}
This worked, foo is green, not red

\pgfkeys{
    /second path/.cd,
    /second path/.search also={/third path, /main path},
    every foo/.style={color=blue},
    foo
}
This failed, foo is still green, not blue
\end{document}
@hmenke hmenke added the pgfkeys label Mar 8, 2022
@fritzw
Copy link
Author

fritzw commented Mar 9, 2022

My first idea to fix this is the following. I'm not sure whether it is viable, because I imagine it could lead to searching the search paths multiple times (maybe even O(n^2) ?) if the key does not exist in any search path.

I think I'll just try to compile the pgfmanual with this and see where it goes.

\long\def\pgfkeys@exp@call#1{%
  \ifpgfkeysaddeddefaultpath% If the original key was a relative path, do the expanded assignment also with a relative path
    \pgfkeys@pathtoks{}%
    \expandafter\pgfkeys@splitter\pgfkeyscurrentkeyRAW//% Removes the /.expand* component of the original relative key.
  \fi%
  \pgfkeysalso{\pgfkeyscurrentpath={#1}}%
}

@fritzw
Copy link
Author

fritzw commented Mar 9, 2022 •
edited

Okay, so I compiled both the pgf manual and the pgfplots manual (which makes heavier use of .search also) using the modified version of \pgfkeys@exp@call shown above. I also added an \immediate\write16 inside the \ifpgfkeysaddeddefaultpath block in order to count the number of invocations with a relative path.

Findings:

  • The compiled pgfmanual is 100% identical (visually, according to diffpdf -a) between the original and the modified version of \pgfkeys@exp@call proposed above. Compiling the pgf manual invokes \pgfkeys@exp@call 5863 times with a relative key path.
  • The compiled pgfplots manual (which makes heavy use of .search also) is also identical, except for plots which include rand or rnd data (which are 6 pages in the version I compiled from the master branch). Compiling the pgfplots manual invokes \pgfkeys@exp@call 11788 times with a relative key path.

However, this does not guarantee that there is not some legacy code out there that would not break if this is changed. Especially the fact that setting /.style can now fail if a search path was in effect, whereas before it would never fail, could cause problems.

On the other hand, /handler config is a rarely used feature, so the probability of things breaking is not that high. Even pgfplots only has a single occurrence where /handler config is changed (in the definition of \pgfplots@define@currentplotstyle@as). And grepping for /handler config in a full TeX Live 2021 installation yields no results besides those in pgfkeys and the aforementioned one in pgfplots.

EDIT: Also, my fears of quadratic run time were not confirmed, at least in the MWE above, because .try never invokes the .unknown handler, so there is no recursion. Even infinite .search also loops do not happen.

fritzw added a commit to fritzw/pgf that referenced this issue Mar 9, 2022
@fritzw
Fix '.search also' with /handler config=full or existing
d5b4c09 
Make .expand* handlers use relative paths if the original path was relative.
Fixes pgf-tikz#1143
@fritzw fritzw linked a pull request Mar 9, 2022 that will close this issue
Open
2 tasks
@fritzw
Copy link
Author

fritzw commented Mar 16, 2022

In case anyone needs it, here is a (hopefully robust) approach which applies the fix proposed above, but only if it can reproduce the behavior of the bug. It will complain with a warning if something goes wrong.

% This file enables a hotfix for https://github.com/pgf-tikz/pgf/issues/1143,
% but only if the bug can be reproduced. To use it, save this file as
% pgfkeys-fix1143.sty somewhere where LaTeX can find it (e.g. current folder).
% Then write \usepackage{pgfkeys-fix1143} somewhere in your preamble.

\RequirePackage{pgfkeys}

\def\pgfkeys@HotfixForSearchAlsoReproduceBug{%
    \begingroup% Keep all side effects of checking local to this group
    \pgfkeys{
        /handler config=full or existing,
        /other path/.unknown/.code={\gdef\pgfkeys@HotfixForSearchAlsoFixed{}},
        /test path/.unknown/.code={\gdef\pgfkeys@HotfixForSearchAlsoFixed{}},
        /test path/.search also={/other path},
        /test path/.cd,
        non existent key/.style={},% If the bug is fixed, this should trigger one of the .unknown handlers
    }%
    \endgroup% End of hotfix group
}%

\pgfkeys@HotfixForSearchAlsoReproduceBug
\ifx\pgfkeys@HotfixForSearchAlsoFixed\@undefined% If bug is not yet fixed in pgfkeys...
    \long\def\pgfkeys@exp@call@old@definition#1{\pgfkeysalso{\pgfkeyscurrentpath={#1}}}% The original definition before the hotfix.
    \ifx\pgfkeys@exp@call\pgfkeys@exp@call@old@definition% only apply the fix if the original definition was not modified
        \PackageInfo{pgfkeys}{issues 1143 hotfix done}%
        \long\def\pgfkeys@exp@call#1{% Define the fixed version of the macro (globally, to escape the current group)
            \ifpgfkeysaddeddefaultpath% If the original key was a relative path, do the expanded assignment also with a relative path
                \pgfkeys@pathtoks{}%
                \expandafter\pgfkeys@splitter\pgfkeyscurrentkeyRAW//% Removes the /.expand* component of the original relative key.
            \fi%
            \pgfkeysalso{\pgfkeyscurrentpath={#1}}%
        }%
    \else
        \PackageWarning{pgfkeys}{issue 1143 hotfix FAILED. The definition of \detokenize{\pgfkeys@exp@call} is not as expected, but the bug is still there.}%
    \fi
    \pgfkeys@HotfixForSearchAlsoReproduceBug% Check if bug is actually fixed after applying the fix
    \ifx\pgfkeys@HotfixForSearchAlsoFixed\@undefined% bug is still not fixed, something went wrong!
        \PackageWarning{pgfkeys}{issue 1143 hotfix did not work.}%
    \else
        \PackageInfo{pgfkeys}{issue 1143 hotfix works}%
    \fi
\else
    \PackageInfo{pgfkeys}{issue 1143 fixed, skipping hotfix}%
\fi

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
pgfkeys
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix '.search also' with /handler config=full or existing fritzw/pgf
2 participants
@hmenke @fritzw