\seq_shuffle:N disturbs an outer seq mapping

[frougon]

Hello,

Please consider this MWE:

\documentclass{article}
\usepackage{expl3}

\ExplSyntaxOn
\seq_new:N \l_test_seq
\seq_set_from_clist:Nn \l_test_seq { a, b, c }

\seq_map_inline:Nn \l_test_seq
  {
    \iow_term:n { XXX }
    \seq_shuffle:N \l_tmpa_seq
  }
\ExplSyntaxOff

\begin{document}
\end{document}

The XXX is only printed once, because \__seq_shuffle:NN uses \cs_set_eq:NN \__seq_item:n \__seq_shuffle_item:n inside a group instead of, .e.g., \__seq_push_item_def:n { \__seq_shuffle_item:n {##1} } ... \__seq_pop_item_def:.

AFAIUI, what happens precisely is that the \seq_gset_from_inline_x:Nnn in the replacement text of \__seq_shuffle:NN uses \__seq_push_item_def:n (plus the corresponding pop), which (properly) does a global assignment to \__seq_item:n before the \endgroup in \__seq_shuffle:NN. This prevents the local assignment to \__seq_item:n from being cancelled when this \endgroup is executed, thus the value of \__seq_item:n the \seq_map_inline:Nn mapping relies on hasn't been restored when the end of the inline loop code is reached. I'll provide a fix in the next message (hit Enter too early by mistake! :-/).

[frougon]

One possible fix is:

\cs_new_protected:Npn \__seq_shuffle:NN #1#2
  {
    \int_compare:nNnTF { \seq_count:N #2 } > \c_max_register_int
      {
        \__kernel_msg_error:nnx { kernel } { shuffle-too-large }
          { \token_to_str:N #2 }
      }
      {
        \group_begin:
          \int_zero:N \l__seq_internal_a_int
          \__seq_push_item_def:n { \__seq_shuffle_item:n {##1} }
          #2
          \__seq_pop_item_def:
          \seq_gset_from_inline_x:Nnn \g__seq_internal_seq
            { \int_step_function:nN { \l__seq_internal_a_int } }
            { \tex_the:D \tex_toks:D ##1 }
        \group_end:
        #1 #2 \g__seq_internal_seq
        \seq_gclear:N \g__seq_internal_seq
      }
  }

By inlining the \__seq_shuffle_item:n {##1} call added in \__seq_push_item_def:n, one can save one macro expansion per item in the sequence. The fix can be tested by simply replacing \cs_new_protected:Npn with \cs_set_protected:Npn.

I can file a merge request if you say whether you prefer this fix or the inlined variant (but maybe you'll want to do something else).

[frougon]

In case you received the previous “comments” by email, please just read #687 instead: as I accidentally sent the bug report too early (I have a... special keyboard), it wasn't ready yet and I had to make a few edits before being satisfied. Sorry!

[frougon]

The shuffled sequence is not the mapped sequence.

[blefloch]

Thanks for the detailed bug report! I'm running the test suite right now, with a variant of what you wrote:

          \@@_push_item_def:
          \cs_gset_eq:NN \@@_item:n \@@_shuffle_item:n
          #2
          \@@_pop_item_def:

instead of using directly \@@_push_item_def:n and having to decide whether to expand or not.

[blefloch]

Even if it were the same sequence, all of our current mappings simply expand the given list before running through it, so shuffling the seq variable will have no effect on the mapping. AFAIK this is undocumented.

[frougon]

Thanks @blefloch, that is indeed a neat idea!