vincentb1 / fmtcount

Source for LaTeX fmtcount package
11 stars 5 forks source link

Delimit if/else branches with braces instead of \else #36

Closed FrnchFrgg closed 4 years ago

FrnchFrgg commented 6 years ago

When cleveref 0.21.4 is loaded together with fmtcount 3.05 with the french language, e.g with

\documentclass[12pt,french]{article}
\usepackage{babel}
\usepackage{cleveref}
\usepackage{fmtcount}
\begin{document}
\ordinalstringnum{1}
\end{document}

the result is

Incomplete \ifnum; all text was ignored after line ...

This is probably due to a command like \@tempa or similar that cleveref redefines to \iftrue or \iffalse somewhere for its usage, which leads the fast conditional skipping of TeX astray. Use braced arguments and \@firstoftwo/\@secondoftwo instead, which fixes the problem.

vincentb1 commented 6 years ago

@FrnchFrgg Bonjour Julien, Désolé, mais je n'arrive pas à reproduire le problème. Chez moi l'exemple minimal que vous donnez compile normalement… Pourriez-vous clarifier quelles versions de fmtcount et de cleveref vous utilisez ?

Ci-joint l'essai que j'ai fait. Pour fmtcount à dire vrai je n'ai pas utilisé le HEAD de master, mais le HEAD de version_3.05+, je doute toutefois que cela fasse une différence, parce que fc-french.def et fmtcount.sty sont sensiblement identiques entre les deux. french-frog-bug.zip

vincentb1 commented 6 years ago

Ce commentaire contient les instructions pour récupérer et installer le HEAD de version_3.05+, histoire qu'on soit exactement sur la même version…

FrnchFrgg commented 6 years ago

I can reproduce the bug with the minimal example zip you gave. I use LuaLaTeX, cleveref 0.21.4 2018/03/27, fmtcount v3.05 2017/12/24. I also tested with the version_3.05+ branch and I get the same results. Similarly with straight pdfTeX instead of LuaTeX.

vincentb1 commented 6 years ago

Sorry, I thought that you were a French speaking person. That is why I answered in French.

My cleveref version is as follows: « cleveref 2013/12/28 v0.19 Intelligent cross-referencing », that is probably why I could not reproduce the problem, I will update it and see what happens.

FrnchFrgg commented 6 years ago

En effet, je suis français. Sauf que je parle tellement anglais sur le net que par réflexe je réponds en anglais... Et en effet, le problème s'est déclaré avec la mise à jour de TeXLive et donc de cleveref. Avec un ancien cleveref, pas de problème, c'est pour cela que j'ai précisé la version dans la pull request initiale.

vincentb1 commented 6 years ago

OK, j'arrive à reproduire le bogue avec la dernière cleveref (la 0.21.5, dispo sur CTAN), et vous avez vu juste c'est bien le fait que cleveref redéfinisse à ìftrue ou \iffalse des noms de macro temporaire du genre \@tempf sans le faire au sein d'un groupe, ou sans ràzer \@tempf à la fin — par ex. avec un petit \let\@tempf\relax, qui pose le problème.

Je pense que le souci est plutôt dans cleveref. J'ai fait une modif que je vous joins…

vincentb1 commented 6 years ago

cleveref.tar.gz

Et voilà…

FrnchFrgg commented 6 years ago

Il y a un problème dans cleveref sans doute, mais il est aussi certain que les constructions avec des booléens primitifs \if \else \fi` de TeX sont très très fragiles dès lors qu'il peut y avoir du contenu non maitrisé à l'intérieur. Plusieurs solutions donc:

  1. Ne pas utiliser de variables « publiques » comme \@tempxxx dans le code mais uniquement des variables dont le nom fait qu'elles ont très très peu de probabilité d'être modifiées par un autre package. LaTeX3 (l3kernel, expl3) résoud en partie ce problème en fournissant des variables de type temporaire qui sont « typées » c'est-à-dire qui ont vocation à toujours recevoir le même type de contenu (et donc pas \iffalse ou \iftrue).
  2. Remplacer les structures conditionnelles primitives par des structures qui prennent des arguments. C'est globalement le choix de LaTeX2e avec les tests du genre \@ifdefined ou \@ifnextchar et cela a été formalisé par LaTeX3 avec les arguments de type TF. Ce choix est le plus logique. Bien sûr, cleveref devrait utiliser (comme dans LaTeX3) des variables définies à 0ou 1 au lieu d'utiliser \iffalse et \iftrue comme valeur booléenne, et remplacer l'appel direct au booléen par \if\@tempa1 par exemple. Mais rendre fmtcount robuste contre les idées farfelues de tous les autres concepteurs de packages semble justifié.
vincentb1 commented 6 years ago

J'avoue que je ne suis pas complètement d'accord avec vos recommandations. Le problème c'est que cleveref fait un \let d'une variable temporaire \@tempf sur un conditionel \iftrue ou \iffalse. Forcément ça pose problème, et je suis sûr qu'il y a plein d'autres paquetages que fmtcount qui seront impactés. Donc je ne vais pas changer mon code pour contourner le problème causé par qq'un d'autre, en cachant ce problème, ce qui revient à retarder sa correction par son auteur.

Vous proposez d'utiliser des noms très peu probables, mais pour moi ça revient à encombrer inutilement la table de hashage de TeX, je préfère utiliser des noms communs pour les variables temporaires pour recycler les ressources mémoire déjà prises.

Par ailleurs, je n'étais pas satisfait de la 1re correction de cleveref que j'ai proposée, parce que ça revenait à admettre le \let de \@tempf sur un conditionnel comme qqchose de licite, en réparant le truc a posteriori par un \let\@tempf\@undefined. Je pense qu'il est plus propre d'utilser \if@tempswa. Voir le code macros2e sur le CTAN. Ci-joint donc la solution avec \if@tempswa: cleveref.tar.gz

vincentb1 commented 4 years ago

@FrnchFrgg Bonsoir, Je m'aperçois que vous n'avez jamais répondu à mon dernier message. Êtes-vous satisfait de ma proposition de correction ? — si oui, vous pouvez vous l'approprier et la poster sur la forge de cleveref, personnellement je n'utilise pas ce paquetage. Si non, alors vos commentaires sont les bien venus.

FrnchFrgg commented 4 years ago

Je suis d'accord que le bug vient de cleveref. Cela dit, les concepteurs de LaTεX 2ε et surtout LaTεX 3 ont décidé consciemment d'éviter un maximum les constructions \if \else primitives, justement à cause de ces problèmes.

Une bonne manière d'être robuste contre les autres packages mal élevés est d'utiliser des techniques comme \@firstoftwo en LaTεX2ε ou comme \bool_if:nTF en LaTεX3, qui s'affranchissent de ces problèmes graces aux arguments délimités par accolades.

Tel était le but de cette PR. Feel free to ignore it.

vincentb1 commented 4 years ago

@FrnchFrgg J'ai pris ta modif, mais comme master est en plein chantier, j'ai mis le changement vers la branche de maintenance version_3.05+ pour l'instant. Je refusionnerai tout ça dans master au moment de sortir la v4.00 qui comprend l'arabe.

vincentb1 commented 4 years ago

@FrnchFrgg Merci pour la contrib.

vincentb1 commented 4 years ago

@FrnchFrgg J'ai versé aujoiurd'hui sur le CTAN une version v3.07 qui prend en compte votre correction.

FrnchFrgg commented 4 years ago

Thanks. I am still waiting for an answer from the cleveref maintainer (because making fmtcount more robust doesn't preclude from fixing the bug in the original package)