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

_* parse bug #3576

Open
pzinn opened this issue Mar 9, 2022 · 8 comments
Open

_* parse bug #3576

pzinn opened this issue Mar 9, 2022 · 8 comments
Labels

Comments

@pzinn
Copy link
Contributor

pzinn commented Mar 9, 2022

\:_* produces the bizarre error message:
KaTeX parse error: Got group of unknown type: 'internal'
same with \, or \!
in constrast LaTeX doesn't mind

@pzinn pzinn added the bug label Mar 9, 2022
@ronkok
Copy link
Collaborator

ronkok commented Mar 9, 2022

Well, the _ symbol indicates a subscript, of course. The KaTeX "supsub" builder is currently seeing an un-expanded macro as its base. Hence the error message. Digging deeper, \; relies on a \tmspace macro, which expands to a \TextOrMath{} macro to determine what value to use for the spacing.

A quick fix would be to wrap the macro for \; in braces. I.e. rewrite: \\tmspace+{5mu}{.2777em} to read {\\tmspace+{5mu}{.2777em}}. And similar wrapping for the other spacing macros.

@edemaine Do you see any drawback to that approach?

@edemaine
Copy link
Member

edemaine commented Mar 9, 2022

I agree that'd be the quick fix, but I'm a little worried because amsmath.sty doesn't do that:

\renewcommand{\,}{\tmspace+\thinmuskip{.1667em}}
\renewcommand{\!}{\tmspace-\thinmuskip{.1667em}}
\renewcommand{\:}{\tmspace+\medmuskip{.2222em}}

I think a more proper solution would be to allow internal nodes, which I assume is what \TextOrMath produces, as the base for a subscript. Or is this difficult for some reason? (What does _ need to know about its base?)

Even better would be to make \TextOrMath not produce its own parse nodes. This would rely on \ifmmode support via #3385. This got reviewed but is awaiting @ylemkimon's revision I think.

@ronkok
Copy link
Collaborator

ronkok commented Mar 9, 2022

The error occurs when supsub calls a buildGroup() on the base, because there is no group builder for an internal ParseType, aka an unexpanded macro. I tried to revise buildGroup in a quick but adequate way, but failed. I think you are correct; we need something more fundamental that ensures that macros are expanded before the supsub builder gets to see them.

So the two options on the table are to wait for #3385 or apply a (temporary?) quick fix with braces.

@edemaine
Copy link
Member

edemaine commented Mar 9, 2022

Actually, I misspoke. I forgot that we already made \TextOrMath work correctly as a macro. \ifmmode isn't relevant.

The issue seems to be the \relax in the definition of \tmspace. \kern3mu\relax_* fails, while \kern3mu_* and \TextOrMath{\kern3mu}{\kern.1667em}_* work fine.

So now I think this issue is a side effect of #3384 (see also #2138 which introduced internal nodes). I wonder if \relax is supposed to build a "null" group, which gets a subscript attached... or if the internal node should get discarded before subscript building.

Answer: it's the latter. For example, {a \over b}\relax_x renders as image, while {a \over b}{}_x renders as image in LaTeX.

@edemaine
Copy link
Member

edemaine commented Mar 9, 2022

I think at some level the issue is in the logic of Parser's parseAtom. This will treat \relax as the beginning of a new atom with its own subscripts etc., but shouldn't. A simple workaround would be to detect \relax in parseAtom and skip them here, but it'd be nicer to skip all internal nodes (as created e.g. by \def). I'm not quite sure how to code the latter, because we won't know about it until a second parseAtom call... @ylemkimon do you have ideas?

@rdong8
Copy link

rdong8 commented Nov 16, 2023

Having a similar error with $\implies^*$. Temporary workaround is \text{$\implies$}^*.

@edemaine
Copy link
Member

Thanks for the additional report! In all cases, a workaround is to add {} before the sub/superscript, as in \implies{}^*.

@pzinn
Copy link
Contributor Author

pzinn commented Feb 1, 2024

any progress on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants