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.

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

Extra \endgroup in \AddToHook #1430

Open
ysalmon opened this issue Aug 8, 2024 · 6 comments
Open

Extra \endgroup in \AddToHook #1430

ysalmon opened this issue Aug 8, 2024 · 6 comments
Labels
bug (improve documentation) nothing really wrong but documentation could be better category base (latex) stale

Comments

@ysalmon
Copy link

ysalmon commented Aug 8, 2024

Brief outline of the bug

The command \AddToHook causes an error in special circumstances.

Minimal example showing the bug

\RequirePackage{latexbug}
\documentclass{article}
\ExplSyntaxOn
\NewDocumentCommand{\test}{m}{
    \group_begin:
    \cs_set_eq:cN { ~ } \space
    \group_end:
}

\AddToHook{cmd/test/before}{\relax}
\ExplSyntaxOff

\begin{document}
nothing
\end{document}

Note the error goes away if the {m} spec is replaced with {}. Also note I have no idea about \cs_set_eq:cN { ~ } \space, this is minimised from a function in a package I did not write (Piton, function \__piton_piton_standard).

Log file (required) and possibly PDF file

texstudio_VuFlvC.log
No PDF file is produced.

@muzimuzhi
Copy link
Contributor

Similar: #1099.

@u-fischer
Copy link
Member

unrelated but you should not patch internal commands of other packages.

@FrankMittelbach
Copy link
Member

I think this ends up being an unfortunate restriction. It is documented that it is not possible to use generic command hooks with every command. Normally the code that inserts the hook tries to identify whether or not it is possible and provide the developer with a sensible error message, but in edge cases like this you can end up with a low-level error.

Trying to avoid this at the cost of making each and every patching noticeably slower is not really viable approach as it doesn't change the situation (other than producing a better error message (it will still not be possible to patch in a generic hook into such commands).

So I think the only thing that should be done here is to strengthen the documentation and stating that in a few cases one might get low-level errors instead of a decent error message when using a generic hook is not possible.

@FrankMittelbach FrankMittelbach added category base (latex) bug (improve documentation) nothing really wrong but documentation could be better labels Aug 8, 2024
@ysalmon
Copy link
Author

ysalmon commented Aug 9, 2024

I see this is tricky.

Is there a way to do the same thing as \cs_set_eq:cN { ~ } \space without using this syntax ?
My naïve idea would be \cs_set_eq:NN \ \space but I guess that would have been used if possible.

The definition is using the L3 catcode table, and assumes that the standard tex/L2 catcode table is in force when its execution starts.

@FrankMittelbach
Copy link
Member

To be honest I have no idea what this \cs_set_eq:cN { ~ } is trying to achieve or why it shouldn't be possible to use \ ` instead. In your simplified MWE it obviously doesn't make any sense, but that is probably due to other code being removed.

Copy link

github-actions bot commented Oct 8, 2024

This issue has been automatically marked as stale because it has not had recent activity.

@github-actions github-actions bot added the stale label Oct 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug (improve documentation) nothing really wrong but documentation could be better category base (latex) stale
Projects
Status: Pool (unscheduled issues)
Development

No branches or pull requests

4 participants