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

etoolbox.sty support #978

Merged
merged 35 commits into from Jul 11, 2018

Conversation

Projects
None yet
2 participants
@dginev
Collaborator

dginev commented Apr 29, 2018

Addresses #478

After a long time staring, there were only minor tweaks needed to get latexml to properly interpret the etoolbox.sty tex code and pass a basic test.

I have not tested on complex examples yet, and there are 4 warnings that indicate the begin/end hooks from etoolbox can't attach yet. But at the very least we're past the Fatal failures. I need to find myself a good torture test for etoolbox next.

my $message = "Input is empty";
if (my $token = $self->readToken) {
$message = "Next token is " . Stringify($token);
unshift(@{ $$self{pushback} }, $token);

This comment has been minimized.

@dginev

dginev Apr 29, 2018

Collaborator

I reworked this since it felt weird to unread an undef value in the empty input case.

@@ -60,7 +60,7 @@ sub readArguments {
my ($self, $gullet, $fordefn) = @_;
my @args = ();
foreach my $parameter (@$self) {
my $value = $parameter->read($gullet);
my $value = $parameter->read($gullet, $fordefn);

This comment has been minimized.

@dginev

dginev Apr 29, 2018

Collaborator

strange oversight, adding fordefn back in makes the parameter read errors much saner (as opposed to wrong argument of undef)

@@ -155,7 +155,7 @@ INVOKE:
# but it isn't expanded in the gullet, but later when digesting, in math mode (? I think)
elsif ($meaning->isExpandable) {
my $gullet = $$self{gullet};
$gullet->unread(@{ $meaning->invoke($gullet) });
$gullet->unread(@{ $meaning->invoke($gullet) || [] });

This comment has been minimized.

@dginev

dginev Apr 29, 2018

Collaborator

I hit a Perl die here while playing around, patched it.

@@ -2206,7 +2206,7 @@ DefPrimitiveI('\operator@font', undef, undef,
#======================================================================
DefMacro('\@tabacckludge {}', '\csname\string#1\endcsname');
DefMacro('\@argdef', '\newcommand'); # stopgap?

This comment has been minimized.

@dginev

dginev Apr 29, 2018

Collaborator

Technically, newcommand is defined in terms of argdef, not the other way around. etoolbox uses argdef directly, and I was surprised to see a simple aliasing to (latexml's) newcommand had great results.

sub make_message {
my ($cmd, @args) = @_;
my $stomach = $STATE->getStomach;
$stomach->bgroup;
Let('\protect', '\string');
my $message = join("\n", map { ToString(Expand($_)) } @args);
Let('\MessageBreak','\ltx@hard@MessageBreak'); # tricky, we need Expand() to execute it
my $message = join("", map { ToString(Expand($_,T_CS('\MessageBreak'))) } @args);

This comment has been minimized.

@dginev

dginev Apr 29, 2018

Collaborator

This is a bit of a perverse change. The warning messages of etoolbox have a trailing \@gobble that seems to intend to remove the trailing line break. So to avoid an argument error, I had to add the trailing newlines in. Meanwhile I also noticed \MessageBreak had to be locally bound to ^^J (and expandable), but wasn't, so fixed that too.

@brucemiller

This comment has been minimized.

Owner

brucemiller commented Apr 29, 2018

Awesome! Looks like we're working on this from both ends :> I'd given up (quite easily) reading in etoolbox.sty, but started writing (w/o testing) bindings for it. A lot of macros and similar patterns. If we can interpret the sty, some will still be useful, I think. The document & environment hooks will need special attention.

@dginev

This comment has been minimized.

Collaborator

dginev commented Apr 29, 2018

Nice! This PR certainly reads in the native package successfully, but whether it does it correctly - I'm not so sure! Especially when it comes to the extra hooks etoolbox introduces. So your work may definitely be the key to get things working in full 👍

I'm having trouble locating a great test case "for free" though, maybe I'll write my own - but perfectly happy to borrow yours if you have one? :>

@brucemiller

This comment has been minimized.

Owner

brucemiller commented Apr 29, 2018

Ha! That's the thing, I wrote no tests at all, just coding, coding, coding :>

@dginev dginev force-pushed the dginev:etoolbox-raw branch from 45ec5cd to 2df9f76 May 4, 2018

@dginev

This comment has been minimized.

Collaborator

dginev commented May 4, 2018

Pushing incremental changes here, as I expand the test coverage.

@dginev dginev changed the title from etoolbox.sty support to [WIP] etoolbox.sty support May 4, 2018

@dginev dginev referenced this pull request May 5, 2018

Closed

\the\value expansion #989

@@ -2700,7 +2700,7 @@ DefPrimitive('\@addtoreset{}{}', sub {
return; });
DefMacro('\value{}', sub {
ExplodeText(CounterValue(ToString(Expand($_[1])))->valueOf); });
T_CS("\\c@".ToString(Expand($_[1]))) });

This comment has been minimized.

@dginev

dginev May 5, 2018

Collaborator

A side-remark here in passing - I've seen a number of errors due to "No more Input" that have to do with the Expand technique of using a standalone Mouth for the expansion. Some of the low-level tricks in the sty files end up reaching beyond the argument, so Expand may need an upgrade that can grab the next Mouth from the gullet when its sandbox is not enough?

This comment has been minimized.

@dginev

dginev May 5, 2018

Collaborator

Not something to address now, I will eventually have a perfect example that shows this, which we can use to upgrade. But bookkeeping to remember.

@dginev

This comment has been minimized.

Collaborator

dginev commented May 5, 2018

@brucemiller I got to the hooks section of the documentation, but your bindings may be called for here. Here is a simple test that should get you a document that says "hello world"

\documentclass{article}
\usepackage{etoolbox}

% --- preamble/postamble document hooks
\AfterPreamble{\secret\newline\renewcommand{\secret}{world}}
\AfterEndPreamble{\secret\newline}
\AtEndPreamble{\def\secret{hello}}
\AfterEndDocument{never printed}

\begin{document}
\end{document}

The --includestyles run relies on the etoolbox \patchcmd and \appto macros to override all of \begin, \end, \document and \enddocument for the hooks. I'm wondering if it would be better to make the pool bindings resilient to such redefinitions, or instead to ensure they are locked in a way where raw TeX interpretation can't override them...

@dginev dginev force-pushed the dginev:etoolbox-raw branch from 75f4651 to 1f2b633 May 6, 2018

@dginev

This comment has been minimized.

Collaborator

dginev commented May 7, 2018

Note from exploring the showcase examples - turns out babel's italian.ldf requires etoolbox explicitly now, and uses its hooks. So all the more reason to think it's time to add support.

@dginev dginev force-pushed the dginev:etoolbox-raw branch 2 times, most recently from bc5941b to 66fc74e May 26, 2018

@dginev dginev force-pushed the dginev:etoolbox-raw branch 2 times, most recently from 8b68f4e to afd862c Jun 10, 2018

@dginev

This comment has been minimized.

Collaborator

dginev commented Jun 11, 2018

A summary comment in the PR on the status of this binding:

  • A lot of discussion happened in private email exchange with Bruce
  • While debugging, I stumbled on some fundamental limitations with latexml-defined macros, which expand to Perl CODE(...) blocks, and have no obvious analogue to the \meaning-based parser in etoolbox
  • This implies a weakened form of \patchcmd, and defining some of the patch-dependent commands directly in Perl
  • Nevertheless, the PR improves on the native interpretation of the .sty file definition, and Bruce has improved master to facilitate that along the way.

At this point I am going through the documentation and writing as many tests as I can, at which point this PR should be good to go.

@dginev dginev force-pushed the dginev:etoolbox-raw branch from 6469221 to cf48b87 Jun 11, 2018

@dginev dginev force-pushed the dginev:etoolbox-raw branch from 3c635ef to fefccc7 Jun 18, 2018

@dginev dginev changed the title from [WIP] etoolbox.sty support to etoolbox.sty support Jun 23, 2018

@dginev

This comment has been minimized.

Collaborator

dginev commented Jun 23, 2018

I now consider this PR with sufficient testing coverage, and welcome review and corrections. CC @brucemiller

DefConditional('\ifodd Number', sub { $_[1]->valueOf % 2; });
DefConditional('\ifnum Number Token Number SkipSpaces', sub { compare($_[1], $_[2], $_[3]); });
DefConditional('\ifdim Dimension Token Dimension SkipSpaces', sub { compare($_[1], $_[2], $_[3]); });
DefConditional('\ifodd Number SkipSpaces', sub { $_[1]->valueOf % 2; });

This comment has been minimized.

@dginev

dginev Jun 23, 2018

Collaborator

Something I would like to understand better is whether this behavior of skipping spaces after the conditional test is something that should be done for all conditional types.

In other words, while patching \ifnum was all it took to get the etoolbox machinery to work, I wonder if it isn't better to patch DefConditional itself centrally, so that you can never declare a conditional that doesn't skip spaces after its test.

Would like to read up on this in the texbook possibly... Just a note for now.

@brucemiller

This comment has been minimized.

Owner

brucemiller commented Jul 8, 2018

I've just pushed a commit that fixes \numexpr (and friends) so that they ignore spaces after expansion; I think this is the reason you were tempted to add the SkipSpaces to some of the \if commands; you should be able to revert that change.

I'm also bugged by the change to object::Equals; Without this change, it seems that the only affect this has on the test cases is that pdflatex considers \begin to be "patchable" (apparently that \patchcmd should succeed?), whereas LaTeXML does not. However, I have to wonder: is \begin patchable in LaTeXML?

@dginev

This comment has been minimized.

Collaborator

dginev commented Jul 8, 2018

Wonderful, rebasing the branch now and checking quickly...

@dginev dginev force-pushed the dginev:etoolbox-raw branch from f80c495 to c026770 Jul 8, 2018

@dginev

This comment has been minimized.

Collaborator

dginev commented Jul 9, 2018

Sadly this was a high difficulty rebase, but with some extra care I think I got it right - in particular the same test tex->xml pair succeeds, and reviewing the final diff looks correct. Removing the skipped spaces indeed keeps the tests passing with the latest master, which is excellent.

@dginev

This comment has been minimized.

Collaborator

dginev commented Jul 9, 2018

As to the patchable cases, the real win are binding-defined expansions that do not take arguments, such as \stop, which the master-branch Object.pm wouldn't successfully compare via the \meaning parsing.

A minimal example document is:

\documentclass{article}
  \usepackage{etoolbox}
\begin{document}
\ifpatchable{\stop}{}{}{not}  patchable
\end{document}

This type of patching will succeed with the latexml run as well, so we do in fact succeed in boosting the raw interpretation. The macros that are impossible to patch are those that have one or more arguments, as the current serialization won't show the expected pattern by the meaning parse.

pdflatex prints "patchable", and so does the current etoolbox branch. However, using the Object.pm from the master branch prints the opposite "not patchable", demonstrating the issue.

And my main claim is that this is solving a general discrepancy, which covers patchable macros only in the specific case of etoolbox. Other packages use \meaning-parsing for other types of techniques as well, so the upgrade could have long-term gain for raw interpretation.

@brucemiller

This comment has been minimized.

Owner

brucemiller commented Jul 9, 2018

@dginev

This comment has been minimized.

Collaborator

dginev commented Jul 9, 2018

I did not say that, no, I did not discuss the \begin case at all.

@dginev

This comment has been minimized.

Collaborator

dginev commented Jul 9, 2018

If I was to rephrase, I would prefer that Object.pm was capable of noticing a "tokenized" CODE(...) pointer returned by \meaning, as a general point of enhancing the technique I dubbed "\meaning-parsing", that is common in raw tex bindings.

If, separately from that, you would prefer that \ifpatchable should be reimplemented in a way that is completely accurate with the latexml implementation of patching, rather than aim for pdflatex-proximity, I can do that as well. But I see it as a higher level concern that is specific to etoolbox.

My perspective from the start was to try and get as much as possible of etoolbox.sty operational as raw tex, and I'm quite happy I stumbled on a couple of generic sticking points that will improve the raw interpretation for a potential number of additional packages.

@brucemiller

This comment has been minimized.

Owner

brucemiller commented Jul 9, 2018

It's always a dilemma; sometimes pretending to be (say) pdflatex is best, but we can't do everything pdflatex can. Likewise depends on what \ifpatchable might be used for; if it's used to check whether to use \patchcmd, then \ifpatchable should do false when we can't patch.

@dginev

This comment has been minimized.

Collaborator

dginev commented Jul 9, 2018

Great, I'm on the same page w.r.t the dilemma between following pdflatex behavior and being honest about what latexml can do. For the binding, I had - and just slightly enhanced - the \patchcmd macro, which is the user-facing feature in question, honestly produce Info messages that it can not patch non-expandable, as well as CODE-expandable, definitions. And return the false case.

Since \patchcmd is the user-facing macro, I am hoping this is both honest and provides the right messages to avoid confusion.

Meanwhile, the \ifpatchable macro is a bit lower-level, and has no further uses in etoolbox itself, which is why I leaned towards keeping it with its "raw tex" interpretation. Let me know if that is acceptable, for now, or if I can add any further upgrades.

I also added a lint pass to my last commit, so it is best seen with the ?w=1 flag on, as in the following link:
ce62b29?w=1

@brucemiller

This comment has been minimized.

Owner

brucemiller commented Jul 10, 2018

Great, cause now I'm having second thoughts! :>
Basically, I'm still inclined to think that we should claim something's patchable, but I'm also guessing that somewhere below here is effectively asking "are these two things the same definition". And even if one's been mangled by \meaning, one might expect LaTeXML to recognize them as still the "same", right?

@dginev

This comment has been minimized.

Collaborator

dginev commented Jul 10, 2018

And even if one's been mangled by \meaning, one might expect LaTeXML to recognize them as still the "same", right?

That's it! Exactly my point, yes. Could you be implying you're coming closer to liking the Object patch?

@brucemiller

This comment has been minimized.

Owner

brucemiller commented Jul 10, 2018

@brucemiller brucemiller merged commit 4e718de into brucemiller:master Jul 11, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment