latex3 / latex2e

The LaTeX2e kernel
https://www.latex-project.org/
LaTeX Project Public License v1.3c
1.94k stars 267 forks source link

Fix for `\KeyValue` in templates defined `\fp_(g)set:NV` to `\relax` #1523

Closed muzimuzhi closed 2 weeks ago

muzimuzhi commented 3 weeks ago

Brief outline of the bug

193bfa8d4 (Expand variable contents when passing \KeyValue content (fixes #1486), 2024-10-07) added something like \exp_not:c { #1 V } to \__template_assign_variable:n, in which #1 has the form <type>_(g)set:N. See line 2127 below.

https://github.com/latex3/latex2e/blob/19854823731d3c1ec932bab9223a3cb576852f19/base/lttemplates.dtx#L2120-L2138

Among the possible <type> values, at least for fp, \fp_(g)set:NV are (previously) undefined, but are now wrongly defined to \relax.

Release release-2024-11-01 is affected.

(First reported in https://github.com/CTeX-org/ctex-kit/issues/723, by myself.)

Minimal example showing the bug

% needs latex-dev
\RequirePackage{latexbug}
\documentclass{article}

\NewTemplateType{foo}{0}    % current LaTeX version

\def\foocode{}
\DeclareTemplateInterface{foo}{FOO}{0}
  {
    foo.code : real ,
    code.foo : real = \KeyValue{foo.code} ,
  }
\DeclareTemplateCode{foo}{FOO}{0}
  {
    foo.code = \foocode ,
    code.foo = \foocode ,
  }
  {\AssignTemplateKeys (\fpeval\foocode)}

\DeclareInstance{foo}{i1}{FOO}{ foo.code=1 }
\DeclareInstance{foo}{i2}{FOO}{ code.foo=1 }

\begin{document}
\UseInstance{foo}{i1} % errors
\UseInstance{foo}{i2} % errors
\end{document}

The first error

! LaTeX Error: A floating point with value '1' was misused.

Log file (required) and possibly PDF file

latex-KayValue-2.log

josephwright commented 3 weeks ago

Sigh - I guess the package wasn't tested against dev - drat. I see you went for

\cs_gset_protected:Npn \fp_set:NV
  { \exp_args:NNV \fp_set:Nn }

but shouldn't

\cs_generate_variant:Nn \fp_set:Nn { NV }

work equally well? I could do that in expl3 today and we could avoid a patch release!

muzimuzhi commented 3 weeks ago

but shouldn't

\cs_generate_variant:Nn \fp_set:Nn { NV }

work equally well?

True. At the time I was not sure about whether \cs_generate_variant:Nn works like \cs_new... or \cs_(g)set....

Update: Oh, \relax is treated as non-existent in expl3.

I'll update the workaround posted in https://github.com/CTeX-org/ctex-kit/issues/723#issuecomment-2451472859.

muzimuzhi commented 3 weeks ago

The \__template_assign_variable: is responsible for processing 7 types: tl, clist, fp, int, dim, skip, and muskip. Among them only the first two have \<type>_(g)set:NV defined.

josephwright commented 3 weeks ago

Hmm, lets see what everyone else thinks - a test for the existence of a V-type variant would add steps to every key assignment - perhaps easier to to quickly add the V-type variants even though they are not all needed.

muzimuzhi commented 3 weeks ago

Or, use \exp_args:NNV \<type>_(g)set:Nn in replace of \<type>_(g)set:NV.

josephwright commented 3 weeks ago

Or, use \exp_args:NNV \<type>_(g)set:Nn in replace of \<type>_(g)set:NV.

Sure, that would work - I was thinking not only about a 'quick fix' but also that we perhaps should make sure this doesn't come up elsewhere, and adding a few variants to expl3 addresses that

FrankMittelbach commented 2 weeks ago

Sure, that would work - I was thinking not only about a 'quick fix' but also that we perhaps should make sure this doesn't come up elsewhere, and adding a few variants to expl3 addresses that

I would add the V-type variants and make a note in the template code that (if ever) this part gets extended with further variable types then it is necessary to ensure that the corresponding V-type is implemented.

josephwright commented 2 weeks ago

@FrankMittelbach Cool - I'll shortcut the normal main/dev cycle for expl3 and upload both at the same time (may be tomorrow) ...

josephwright commented 2 weeks ago

Should be fixed: if everyone is happy with that being done at the expl3 end, I'll close (and mark hotfixed).

muzimuzhi commented 2 weeks ago

Maybe add some tests (to lttemplates005.lvt)?

josephwright commented 2 weeks ago

Doing tests as suggested, there is still an issue - I will need to look again at this. Fix hopefully later today - with tests :)

josephwright commented 2 weeks ago

Sorted - I'll close