Move \global and \def to functions by ylemkimon · Pull Request #2138 · KaTeX/KaTeX

ylemkimon · 2019-10-27T01:53:20Z

TeX primitive commands are more like functions, so move to functions and return new parseNode type internal, which is skipped during parseExpression.

codecov-io · 2019-10-27T01:56:15Z

Codecov Report

Merging #2138 into master will increase coverage by <.01%.
The diff coverage is 93.33%.

@@            Coverage Diff             @@
##           master    #2138      +/-   ##
==========================================
+ Coverage   94.75%   94.75%   +<.01%     
==========================================
  Files          82       83       +1     
  Lines        5315     5316       +1     
  Branches      927      927              
==========================================
+ Hits         5036     5037       +1     
  Misses        256      256              
  Partials       23       23

Flag	Coverage Δ
#screenshotter	`89.26% <44.82%> (+0.02%)`	⬆️
#test	`88.48% <93.33%> (ø)`	⬆️

Impacted Files	Coverage Δ
src/parseNode.js	`80% <ø> (ø)`	⬆️
src/macros.js	`97.08% <ø> (+0.26%)`	⬆️
src/functions.js	`100% <ø> (ø)`	⬆️
contrib/render-a11y-string/render-a11y-string.js	`89.18% <0%> (-0.35%)`	⬇️
src/Parser.js	`96.85% <100%> (+0.01%)`	⬆️
src/functions/def.js	`96.29% <96.29%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0d8830a...6694bee. Read the comment docs.

Move \newcommand back to macros for now

edemaine · 2019-11-29T19:06:32Z

@ylemkimon Could you clarify the sense in which \def is more like a function than a macro? I assumed functions were a match for things that produced output, but \defs do not. Could you show an example that parses more correctly this way? (Apologies if this is mentioned in another issue/PR -- I'm far from caught up.) Would also make sense to add this as a test.

ylemkimon · 2019-12-01T12:26:10Z

@edemaine \def is a primitive TeX command, which is unexpandable and consumed in the TeX's "stomach", whereas macros are expandable and consumed in the TeX's "gullet". Currently in KaTeX, \def is defined as a macro that expands into an empty string before reaching the "stomach". So, for instance, 2^\def\foo{1}2 expands into 2^2 in KaTeX, but doesn't work in TeX.

Furthermore, this is required in two pending PRs:

Add support for \expandafter, \noexpand, \edef, \let, and \long #2122: \def should not be affected by \noexpand or \expandafter
feat(parser): add integer, dimen, glue group parsers #2139: Register (variable) command will be defined using functions and \global should work with them.

k4b7 · 2019-12-02T00:21:58Z

test/katex-spec.js

    it("should prevent expansion", () => {
-        expect`\gdef\foo{1}\foo`.toParse();
-        expect`\gdef\foo{1}\foo`.toParse(new Settings({maxExpand: 2}));
-        expect`\gdef\foo{1}\foo`.not.toParse(new Settings({maxExpand: 1}));


It looks like the behavior was wrong before this PR. It it safe to say that the following will pass now?

expect`\gdef\foo{1}\foo`.toParse(new Settings({maxExpand: 1}));

@kevinbarabash That seems better!

k4b7 · 2019-12-02T00:26:31Z

contrib/render-a11y-string/render-a11y-string.js

+        case "internal": {
+            break;
+        }


In reality we should never have to handle this case since all all internal nodes in the AST should get expanded, right?

I believe the \def function returns an internal node (at least that's what the PR main text says), and this throws it away.

Edit: Oops, I see: yes, maybe not possible in render-a11y-string.js, given that they're eaten by Parser.js. This might just be to handle lint?

k4b7 · 2019-12-02T00:28:03Z

src/functions/assignment.js

@@ -0,0 +1,79 @@
+//@flow


The name of this file feels a bit misleading. Could you renamed it to internal.js?

I think assignment refers to \def and \gdef which is the purpose of this file. (The idea is that other commands might also generate internal.) I could see calling it def.js. The name assignment.js is presumably future-proofing for an eventual \let definition? But I'd still lean toward def.js.

The reason why I suggested internal.js was that it also handles \global, but if \global can only be used with \def then def.js should be fine as well.

Changed to def.js.

k4b7

Requesting a filename change from assignment.js to internal.js. Other than that, this looks great. The fact that all existing tests pass and it fixed one of the test cases gives me confidence in this change.

edemaine · 2019-12-02T01:25:34Z

@ylemkimon Thanks for the explanation! I see now and agree.

Ideally we should mention in a comment somewhere that KaTeX macros correspond to TeX gullet and KaTeX functions correspond to TeX stomach. I was (mostly) aware of this, but wasn't really aware of how \def fit in.

2^\def\foo{1}2 is a great example to think about, and I think we should add it as a test too (unless you already did and I missed it).

ylemkimon · 2019-12-04T02:07:33Z

test/katex-spec.js

        expect`\global\def\foo{}\foo`.toParseLike``;
-        // TODO: This doesn't work yet; \global needs to expand argument.
-        //expect`\def\DEF{\def}\global\DEF\foo{}\foo`.toParseLike``;
+        expect`\def\DEF{\def}\global\DEF\foo{}\foo`.toParseLike``;


It turns out this fixes another test case.

k4b7

LGTM. Thanks for improving the behavior of \def.

ylemkimon added 3 commits October 27, 2019 10:50

Add parseNode type internal

25c14b0

Move \def and \newcommand to functions

2184694

Fix Flow error

7d438c7

ylemkimon added a commit that referenced this pull request Oct 29, 2019

Port #2138: add parseNode type internal

986e240

ylemkimon requested review from edemaine and k4b7 November 1, 2019 15:30

ylemkimon added the GH Review: review-needed label Nov 1, 2019

Separate \global

462a719

Move \newcommand back to macros for now

ylemkimon added the high priority label Nov 2, 2019

ylemkimon changed the title ~~Move \def and \newcommand to functions~~ Move \global and \def to functions Nov 3, 2019

k4b7 reviewed Dec 2, 2019

View reviewed changes

k4b7 mentioned this pull request Dec 2, 2019

Add support for \expandafter, \noexpand, \edef, \let, and \long #2122

Merged

k4b7 added GH Review: needs-revision and removed GH Review: review-needed labels Dec 2, 2019

k4b7 requested changes Dec 2, 2019

View reviewed changes

Merge branch 'master' into internal

a1c643b

ylemkimon added GH Review: review-needed and removed GH Review: needs-revision labels Dec 3, 2019

ylemkimon added 4 commits December 4, 2019 01:00

Rename assignment.js to def.js

4470b19

Update test cases

0d75823

Add comments

83e08de

Update a test case

6694bee

ylemkimon commented Dec 4, 2019

View reviewed changes

k4b7 approved these changes Dec 5, 2019

View reviewed changes

k4b7 added GH review: accepted and removed GH Review: review-needed labels Dec 5, 2019

k4b7 merged commit b1eeeec into master Dec 5, 2019

ylemkimon deleted the internal branch July 11, 2020 19:19

edemaine mentioned this pull request Mar 9, 2022

_* parse bug #3576

Open

Uh oh!

Conversation

ylemkimon commented Oct 27, 2019

Uh oh!

codecov-io commented Oct 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

edemaine commented Nov 29, 2019

Uh oh!

ylemkimon commented Dec 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edemaine Dec 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k4b7 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edemaine commented Dec 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k4b7 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-io commented Oct 27, 2019 •

edited

Loading

ylemkimon commented Dec 1, 2019 •

edited

Loading

edemaine Dec 2, 2019 •

edited

Loading

k4b7 left a comment •

edited

Loading

edemaine commented Dec 2, 2019 •

edited

Loading