l3keys isn't align safe

[Skillmon]

The underlying \keyval_parse:NNn of l3keys isn't alignment safe if the key=value list contains an alignment character outside of braces.

\documentclass[]{minimal}

\ExplSyntaxOn
\NewDocumentCommand \foo { m } { \keys_set:nn { foo } {#1} }
\keys_define:nn { foo } { a .tl_set:N = \l_tmpa_tl }
\ExplSyntaxOff

\begin{document}
\begin{tabular}{c}
  \foo{a={a&b}} \\ % works
  \foo{a=a&b} \\ % doesn't work
\end{tabular}
\end{document}

[Skillmon]

There are a few possible fixes for this. The following implements a few of them as an MWE and runs some test to show in which cases the respective fix would actually work.

\documentclass[]{article}

\ExplSyntaxOn
\cs_new:Npn \test:nn #1#2
  {
    \typeout{}
    \typeout { while~ searching~ for~ noalign/omit }
    \tex_halign:D { \typeout{0} ## \typeout{1} \tex_cr:D
      \keyval_parse:nnn {#1} {#2} { a, a=b&c }
      \tex_cr:D
    }
    \typeout { with~ relax }
    \tex_halign:D { \typeout{0} ## \typeout{1} \tex_cr:D
      \relax \keyval_parse:nnn {#1} {#2} { a, a=b&c }
      \tex_cr:D
    }
    \typeout{}
  }
\ExplSyntaxOff

\begin{document}
\ExplSyntaxOn

% no fix applied
\test:nn \use_none:n \use_none:nn % fails with relax
\test:nn { \relax \use_none:n } { \relax \use_none:nn } % fails in both cases

% Fix #1: only one pair of group_align_safe...
% This attempted fix will wrap the whole parsing in a single align-safe group
% starting just before the first step of parsing and closing it when the loop is
% done. The `\exp_args:Nno \use:n` construct is used to expand the actual
% meaning of \keyval_parse:nnn so that I don't have to set up the active
% category codes used there.
\group_begin:
\exp_args:Nno \use:n { \cs_set:Npn \keyval_parse:nnn #1#2#3 }
  {
    \exp_after:wN \group_align_safe_begin:
    \keyval_parse:nnn {#1} {#2} {#3}
  }
\cs_set:Npn \__keyval_end_loop_active:w
    \s__keyval_tail
    \__keyval_loop_other:nnw #1 \s__keyval_mark \s__keyval_tail , \s__keyval_tail ,
  { \group_align_safe_end: }

\test:nn \use_none:n \use_none:nn % works in both cases
\test:nn { \relax \use_none:n } { \relax \use_none:nn } % fails without relax

% Fix #2: also put group_align_safe... around the user code
% This fix uses the same align-safe group, but closes and reopens it on every
% user code being inserted.
\cs_set:Npn \__keyval_pair:nnnn #1 #2 #3 #4
  {
    \__keyval_if_blank:w \s__keyval_mark #2 \s__keyval_nil \s__keyval_stop \__keyval_blank_key_error:w
      \s__keyval_mark \s__keyval_stop
    \group_align_safe_end:
    \exp_not:n { #4 { #2 } { #1 } }
    \group_align_safe_begin:
    \__keyval_loop_other:nnw {#3} {#4}
  }
\cs_set:Npn \__keyval_key:nn #1 #2
  {
    \__keyval_if_blank:w \s__keyval_mark #1 \s__keyval_nil \s__keyval_stop \__keyval_blank_key_error:w
      \s__keyval_mark \s__keyval_stop
    \group_align_safe_end:
    \exp_not:n { #2 { #1 } }
    \group_align_safe_begin:
    \__keyval_loop_other:nnw {#2}
  }

\test:nn \use_none:n \use_none:nn % works in both cases
\test:nn { \relax \use_none:n } { \relax \use_none:nn } % works in both cases

% undo the fixes above
\group_end:

% Fix #3: using tex_expanded:D
% This fix should be quite obvious, during each step of parsing the entire code
% is surrounded by the brace groups of \tex_expanded:D and \__kernel_exp_not:w.
\group_begin:
\exp_args:Nno \use:n { \cs_set:Npn \keyval_parse:nnn #1#2#3 }
  {
    \exp_after:wN \__kernel_exp_not:w \exp_after:wN \tex_expanded:D
    \exp_after:wN { \exp_after:wN { \keyval_parse:nnn {#1} {#2} {#3} } }
  }

\test:nn \use_none:n \use_none:nn % works in both cases
\test:nn { \relax \use_none:n } { \relax \use_none:nn } % works in both cases
\group_end:

\group_begin:
% Fix #4: delay output until after parsing is done
% This fix stores all the user supplied code in a braced group which will only
% be used after the entire parsing is done. The parsing itself is also
% surrounded by a align-safe group.
\exp_args:Nno \use:n { \cs_set:Npn \keyval_parse:nnn #1#2#3 }
  {
    \exp_after:wN \group_align_safe_begin:
    \keyval_parse:nnn {#1} {#2} {#3}
    \group_align_safe_end:
    \__keyval_result:n { }
  }
\cs_new_eq:NN \__keyval_result:n \__kernel_exp_not:w
\cs_new:Npn \__keyval_output:nw #1 #2 \__keyval_result:n #3
  { #2 \__keyval_result:n { #3 #1 } }
\cs_set:Npn \__keyval_pair:nnnn #1 #2 #3 #4
  {
    \__keyval_if_blank:w \s__keyval_mark #2 \s__keyval_nil \s__keyval_stop \__keyval_blank_key_error:w
      \s__keyval_mark \s__keyval_stop
    \__keyval_output:nw { #4 { #2 } { #1 } }
    \__keyval_loop_other:nnw {#3} {#4}
  }
\cs_set:Npn \__keyval_key:nn #1 #2
  {
    \__keyval_if_blank:w \s__keyval_mark #1 \s__keyval_nil \s__keyval_stop \__keyval_blank_key_error:w
      \s__keyval_mark \s__keyval_stop
    \__keyval_output:nw { #2 { #1 } }
    \__keyval_loop_other:nnw {#2}
  }

\test:nn \use_none:n \use_none:nn % works in both cases
\test:nn { \relax \use_none:n } { \relax \use_none:nn } % works in both cases

\group_end:

\ExplSyntaxOff
\end{document}

I'm wary which of these is the way to go, and I honestly don't understand why the first fix (only one align-safe group around everything) fails if the user provided code (in this case \relax \use_none:n(n)) contains unexpandable material (if someone could explain this to me, I'd be very grateful), and only while TeX is searching for noalign/omit.

When it is decided to use the third fix (using \tex_expanded:D), we'd have to also implement one of the other fixes to support old engines which don't have the \expanded primitive yet.

[Skillmon]

Also, if the fourth fix (delaying output) is chosen, we could reorder much of the internals of \keyval_parse:nnn to not hand down the user-provided code, but instead put that after the output marker, this way we would gain some speed in that code (but the code would be considerably slower in total if the key=val list contains a few elements).

[Skillmon]

Explanation why the first proposed fix doesn't work is no longer needed, that's actually contained in a footnote of the TeXbook:

\footnote*{The
token list $\alpha$ should not be empty, however, because \TeX\ expands
the first token of an alignment entry before looking at the template,
in order to see if the entry begins with |\noalign| or |\omit|. The master
counter value that is considered to be present at the beginning of an
entry is the value in the counter just after the ``$u$~part'' of the
template has been entirely read.}

[davidcarlisle]

I'm not sure that we have to fix this, we could just document it. This variant of the original test document runs without error

\documentclass[]{minimal}

\ExplSyntaxOn
\NewDocumentCommand \foo { m } {\iffalse{\fi\keys_set:nn { foo } {#1} \iffalse}\fi }
\keys_define:nn { foo } { a .tl_set:N = \l_tmpa_tl }
\ExplSyntaxOff


\begin{document}
\begin{tabular}{c}
\foo{a={a&b}} \\ % works
x\foo{a=a&b} \\ % works
\foo{a=a&b} \\ % works
\end{tabular}
\end{document}

Here the calling code has already used a \iffalse{\fi guard before parsing, and so there is no need to make the key parse alignment safe. Here I don't try to use expansion contortions to remove the training \iffalse}\fi as it is harmless here.

I think in most cases where a keyval parser is used in an alignment the same will be true, it will be vanishingly rare that a document level command occurring in an alignment needs to expand straight into key parsing with no possibility of adding any guards. Perhaps the main example would be a version of \multicolumn taking a kv argument but if such a command has to take special precautions I don't think that's too bad, if making the general fix has an adverse effect on the speed and maintainability of the parsing in all the normal cases.

[blefloch]

I disagree with David here. The typical package writer will not think of putting \iffalse{\fi constructions in the right places. (sorry, typing with 1 hand only)

[Skillmon]

I agree with @davidcarlisle in part. It is trivial to make \keys_set:nn alignment safe, because it already is a protected macro one can just put a \group_align_safe_begin: before it calls \keyval_parse:NNn, and the matching ..._end: after it.

The typical package writer will use \keys_set:nn, anyone who's creating a key=value interface that should be used inside an alignment with \keyval_parse:nnn is not the typical package writer.

So it might really be a good idea to document \keyval_parse:nnn as not being alignment safe while \keys_set:nn is.

[Skillmon]

Also the use case with an expandable key=value macro that should expand to \multicolumn or similar doesn't need to be alignment safe anyway, as the parsing would be done while TeX scans for \noalign/\omit, during which process alignment tokens aren't replaced by the preamble anyway. So that one use case we could think of is already supported.

Does anyone have another good use case that really requires \keyval_parse:nnn to be alignment safe?

[davidcarlisle]

they might think of it if we documented it was needed, but my point is that in most cases where it matters they probably need to have done this already, the keyval parsing is typically embedded in some command definition and you need to have the guards higher up when you grab the arguments. If in the end we come up with a fix that isn't over contorted or have too great an impact in the common case where it isn't needed then yes we should add it but otherwise it isn't clearly a win, I think.

[blefloch]

@david: "probably need to have done this already [...] need to have the guards higher up when you grab the arguments" → That's not true because in \def\mycommand#1{\keyval_parse:NNn \my:n \my:nn {#1}} \halign{#\cr \mycommand{foo=&,bar}} the & is properly hidden by the braces, until we reach the internals of \keyval_parse:NNn. From the package author's point of view it seems that #1 is always properly hidden by braces so things should be align-safe. I quite like the third fix \unexpanded\expanded{{...}}. In engines without \expanded I like the second fix (opening/closing the group_align_safe at each user code). Is there a reason to prefer the fourth fix (collecting everything until the end) compared to the second?

[Skillmon]

We could reduce code complexity with the fourth (no forwarding of the user code through all levels needed). It is guaranteed to be stable. And it is guaranteed to be f-expandable just like the \unexpanded\expanded one.

I'll run a few small benchmarks next week with the three candidates. Then we can make an educated decision.

[blefloch]

I don't think you should spend time on the fix number four because it will be prohibitively slow (quadratic in the number of keyval entries) and we have many other restricted-expandable commands of the same kind: I view \keyval_parse:NNn as a cousin of \tl_map_function:nN.

Maybe when \expanded becomes required (in a few years?) we can have a sweep through all restricted-expandable functions and mostly (fully?) retire the notion by making all of them completely expandable.

[blefloch]

I've gone for fix number 2 even when \expanded is available: it would have been quite messy to distinguish the two cases and do fixes number 2 and 3 accordingly.

[Skillmon]

@blefloch I don't think it would've been too messy to implement both fixes in parallel, and the \unexpanded\expanded fix is faster than the other. I'll open a pull request.

[blefloch]

Thanks. Sorry, I was a bit lazy about that.