latex3 / latex2e

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

ltkeys: Global options starting surrounded by spaces in the input are not removed from the unused option list #1238

Closed Skillmon closed 3 days ago

Skillmon commented 9 months ago

Brief outline of the bug

The current code used to remove keys from the unused options list doesn't first strip spaces from the key name, which results in false-positives in the unused options list.

Minimal example showing the bug

\RequirePackage{latexbug}       % <--should be always the first line (see CONTRIBUTING)!

\begin{filecontents}{bugreport.sty}
\DeclareKeys{foo.store=\myfoo, bar.store=\mybar, baz.store=\mybaz}
\ProcessKeyOptions
\ProvidesPackage{bugreport}
\end{filecontents}

\documentclass[foo =bar, bar=baz, baz = foo]{article}

  % Any preamble code goes here
\usepackage{bugreport}

\typeout{%
  foo: \myfoo^^J%
  bar: \mybar^^J%
  baz: \mybaz^^J%
}

\begin{document}
\end{document}

Log file (required) and possibly PDF file

bugreport.log

Skillmon commented 9 months ago

First found thanks to https://tex.stackexchange.com/questions/706935/why-do-i-get-unused-global-options-warning-for-options-forwarded-to-a-package

josephwright commented 8 months ago

Fixed in dev?

Skillmon commented 8 months ago

It is fixed in main see https://github.com/latex3/latex2e/pull/1239. The ltnews39 entry is still missing. Also, currently the fix wasn't merged into develop, so the -dev formats still throw the warning, while the normal formats don't.

tweh commented 3 months ago

As far as I understood this issue is fixed however I came across this while implementing an expl3 class and the following MWE still show this issue (with TeXLive 2024, up-to-date on macOS, up-to-date)

\RequirePackage{latexbug}       % <--should be always the first line (see CONTRIBUTING)!

\begin{filecontents}{democlass.cls}
\ProvidesExplClass {democlass} {2024/07/22} {1} {demo}
\LoadClassWithOptions { scrbook }
\tl_new:N \l_demo_word_tl
\keys_define:nn { democlass } {
  word .tl_set:N = \l_demo_word_tl,
}
\ProcessKeyOptions [ democlass ]
\newcommand{\myword}{\tl_use:N \l_demo_word_tl}
\end{filecontents}

\documentclass[word =xx]{democlass}

% Any preamble code goes here
\usepackage{bugreport}

\begin{document}
Test: \myword
\end{document}

Am I doing something wrong or is this bug still persistent?

muzimuzhi commented 3 months ago

@tweh It seems the problem only reproduces with Koma-Script document classes like scrbook or scrartcl, but not with standard ones like article.

\documentclass[foo =xx]{scrbook}
\makeatletter
% article.cls, unused options: <foo>
% scrbook.cls, unused options: <foo >, note the trailing space
\PackageError{debug}{unused options: <\@unusedoptionlist>}{}
\makeatother

\begin{document}
content
\end{document}
muzimuzhi commented 3 months ago

Looks like \OptionNotUsed is the culprit. Here's a reproducing example with article:

\RequirePackage{latexbug}       % <--should be always the first line (see CONTRIBUTING)!
\begin{filecontents}[force]{democlass.cls}
\LoadClass{article}
\DeclareKeys {word .code = \OptionNotUsed}
\ProcessKeyOptions
\end{filecontents}

\documentclass[word =xx]{democlass}

\begin{document}
\end{document}

In log

LaTeX Warning: Unused global option(s):
    [word ].

\OptionNotUsed only removes =<value> from \CurrentOption whose value may have form <key>=<value>, but doesn't trim spaces from <key>. https://github.com/latex3/latex2e/blob/22375e7b5e90cc9638f62806f69a11aa532430e5/base/ltclass.dtx#L1522-L1527

muzimuzhi commented 3 months ago

One way is to add space-trimming to \@remove@eq@value, which is only used in two fully-expansion places.

\ExplSyntaxOn
% \def\@remove@eq@value#1=#2\@nil{#1} % before
\def\@remove@eq@value#1=#2\@nil{\tl_trim_spaces:n{#1}}
\ExplSyntaxOff

https://github.com/latex3/latex2e/blob/22375e7b5e90cc9638f62806f69a11aa532430e5/base/ltclass.dtx#L1524-L1526 https://github.com/latex3/latex2e/blob/22375e7b5e90cc9638f62806f69a11aa532430e5/base/ltclass.dtx#L1744-L1745

josephwright commented 3 months ago

@muzimuzhi Make a PR?