Move all JS codegeneration to the `generate-js` pass #117

Mingun · 2021-04-27T15:06:30Z

Port of my PR to solve that problem: pegjs/pegjs#450.

Besides solving the original incapsulation problems, this PR also opens doors for the source map support (#105) by introducing place where original position information can be stored.

I'm not sure, should this be considered as breaking change or not. Strictly speaking, that it is, because it:

removes the consts member from the grammar node, which some third-party passes can expect (I can restore that, but if third-party passes modifies it, that modifications will not have the effect)
changes the name of the opcode MATCH_REGEXP to MATCH_CLASS (not strictly necessary, can be removed).

So if that changes is unacceptable now, can be held that until release 2.0.

Fixes #106

StoneCypher · 2021-04-27T16:12:43Z

changes the name of the opcode MATCH_REGEXP to MATCH_CLASS (not strictly necessary, can be removed).

It seems like this would introduce confusion with regards to es6 classes?

StoneCypher · 2021-04-27T16:13:39Z

removes the consts member from the grammar node, which some third-party passes can expect (I can restore that, but if third-party passes modifies it, that modifications will not have the effect)

I would like to understand what this means better, please

I assume this doesn't prevent peggy from parsing const, so, what's the goal here? Was it just previously under the wrong part of the grammar, or something like that?

Mingun · 2021-04-27T16:19:40Z

I would like to understand what this means better, please

I mean, that if you wrote a plugin that attached a pass to the generate stage between generateBytecode and generateJs and that pass does something like that:

let myPass = visitor.build({
  grammar(node) {
    let consts = node.consts;// Uhh... where it now!?
    // use consts
  }
});

your plugin in a risk group.

StoneCypher · 2021-04-27T19:57:13Z

There's almost no peg plugins. This is in some way a different product. This doesn't appear to interfere with the coffeescript or typescript plugins.

I'm mostly in support of this, despite that it is a breaking change, because it's a very minor breakage in a feature that's mostly unused. I think I'd be in total support of this when I knew:

How hard would it be to update a plugin to your new structure?
Why is it actually being done?

Mingun · 2021-04-27T20:38:43Z

If you not use opcodes.js and generate-bytecode.js, then nothing to do for update.
Why I done that in the first time? I tried to write generator for other languages, Java, to be honest. And I realized that I need to decode JS regexps and JS code to make a port, or write my own copy of generate-bytecode.js, or not use bytecode at all. Then David create an issue where he want to remove excess evals and that change does exactly that.

StoneCypher · 2021-04-27T20:41:20Z

yeah the eval is a problem, it makes a lot of build tools angry (and rightfully so)

in addition to being a security win, it'll also be a pretty big speed win, i suspect

StoneCypher · 2021-04-27T20:41:38Z

i support this pr

hildjj · 2021-04-28T18:01:24Z

changes the name of the opcode MATCH_REGEXP to MATCH_CLASS (not strictly necessary, can be removed).

It seems like this would introduce confusion with regards to es6 classes?

We already have a bunch of bits that refer to character classes [^abc] but it would be consistent to always say "character class" whenever we're referring to those. I'll admit to having been confused a few times looking through the code in the places where we didn't do that.

hildjj

Look at the suggested optimizations, please. They are not mandatory, but I'd like you to at least rule them out before we merge. I want to land #107 before this, and #116 after this.

lib/compiler/passes/generate-bytecode.js

lib/compiler/passes/generate-js.js

hildjj · 2021-05-02T21:35:02Z

@Mingun if you rebase this, I'd be happy to merge it next.

Mingun · 2021-05-03T11:54:16Z

I've rebased and change MATCH_CLASS to MATCH_CHAR_CLASS (I still can drop that commit completely if you wish). All other remained unchanged (justifications inline)

hildjj · 2021-05-18T15:01:58Z

Is this the next patch to land? It needs a rebase onto main at least, to resolve the conflicts, but I'll go through it with a closer eye if we're about ready.

…on by content types into: * `literals` -- for literal expressions, like `'a'` * `classes` -- for character class expressions, like `[a]` * `expectations` -- for constants describing expected values when parse failed * `functions` -- for constants with function code

…rating to `generateJs` pass from `generateBytecode` pass. That allows to generate parser on other languages and more easy replace actions. Also, this is helpful for better hiding of the peggy internals from an initializers and functions code, which is needed for the import feature. After this commit, a function code (semantic predicates and actions) is stored in the `functions` field of the grammar node instead of the `consts` field. This is also opens doors for supporting source maps, because we get a place for the storing information about the original positions of the codes

…eneration to `generateJs` pass from `generateBytecode` pass.

…ting to `generateJs` pass from `generateBytecode` pass.

…ation to pass `generateJs` from pass `generateBytecode`.

…pdate CHANGELOG.md

Mingun · 2021-05-18T17:25:16Z

Yes, it is ready for review. I've made a rebase so please review.

hildjj

Other than finishing the backward-compat discussion for plugins, this is ready to go.

Note for myself later: the escaping routines are not new, they were just moved from a now-unnecessary separate file. I think we should discuss how we're doing regexp and string escaping in #15, since we'll need to touch this code then anyway.

hildjj · 2021-05-18T17:29:58Z

lib/compiler/opcodes.js

@@ -4,52 +4,52 @@
 const opcodes = {
  // Stack Manipulation

-  PUSH:             0,    // PUSH c


Doesn't this (as an example of several opcode changes) conflict with the typescript plugin? Is @metadevpro following the work here yet? How could they support both peggy and pegjs if they wanted?

It will. Instead of renaming that opcode we can just leave it unused and introduce a new opcode for PUSH_EMPTY_STRING. Renaming MATCH_REGEXP also should be reverted. Also we could add an alias instead of renaming or just do nothing. Then in version 2.0 we could completely remove the unused opcode.

On the other hand, @metadevpro could create a local copy of opcodes, he anyway is not needed to use exactly the same opcodes as peggy used because they used only as a glue between generate-bytecode-ts and generate-ts passes.

OK, my suggestion from your choices above:

Leave PUSH as 0. Mark it as deprecated with a comment.

Create a new opcode for PUSH_EMPTY_STRING

Leave MATCH_CHAR_CLASS and MATCH_REGEXP both as opcode 20, but mark MATCH_REGEXP as deprecated with a comment.

I think this will allow plugins to support both peggy and pegjs at the same time adequately. If you expect that plugins would need to take different actions for MATCH_CHAR_CLASS and MATCH_REGEXP, then don't alias them together, and instead leave MATCH_REGEXP as opcode 20, create a new opcode for MATCH_CHAR_CLASS.

All done as you said. I've make a rebase , changed only the last and the pre-pre-last commits. MATCH_REGEXP and PUSH marked @deprecated, noted this in the CHANGELOG.md

…GEXP` -> `MATCH_CHAR_CLASS`. New name reflects the opcode purpose, not implementation.

…te-js.js

…h PUSH_EMPTY_STRING opcode because it only used with empty strings

hildjj requested changes Apr 28, 2021

View reviewed changes

hildjj mentioned this pull request Apr 28, 2021

Typescript, Rollup, Jest, Terser, Rimraf, announcer, de-binarying, CI platforming #107

Closed

Mingun mentioned this pull request Apr 30, 2021

Optimize redundant fail checks #118

Merged

Mingun force-pushed the bc-cleanup branch from b32e750 to b027048 Compare May 3, 2021 11:49

Mingun force-pushed the bc-cleanup branch 3 times, most recently from 652b4e0 to 1ce2528 Compare May 4, 2021 19:41

Mingun added 6 commits May 18, 2021 22:13

Cleanup bytecode generator (pegjs/pegjs#450): Move expectation code g…

f3c6d0c

…eneration to `generateJs` pass from `generateBytecode` pass.

Cleanup bytecode generator (pegjs/pegjs#450): Move regexp code genera…

3d51dd9

…ting to `generateJs` pass from `generateBytecode` pass.

Cleanup bytecode generator (pegjs/pegjs#450): Move literal code gener…

8318d04

…ation to pass `generateJs` from pass `generateBytecode`.

Cleanup bytecode generator (pegjs/pegjs#450): Regenerate parser and u…

ed88b84

…pdate CHANGELOG.md

Mingun force-pushed the bc-cleanup branch from 1ce2528 to 75c088c Compare May 18, 2021 17:21

hildjj reviewed May 18, 2021

View reviewed changes

hildjj approved these changes May 18, 2021

View reviewed changes

Mingun and others added 2 commits May 21, 2021 00:26

Cleanup bytecode generator (pegjs/pegjs#450): Rename opcode `MATCH_RE…

e8557cc

…GEXP` -> `MATCH_CHAR_CLASS`. New name reflects the opcode purpose, not implementation.

Cleanup bytecode generator (pegjs/pegjs#450): Merge js.js into genera…

dcbe046

…te-js.js

Cleanup bytecode generator (pegjs/pegjs#450): Replace PUSH opcode wit…

aa44ec3

…h PUSH_EMPTY_STRING opcode because it only used with empty strings

Mingun force-pushed the bc-cleanup branch from 75c088c to aa44ec3 Compare May 20, 2021 19:45

hildjj merged commit be21fa2 into peggyjs:main May 20, 2021

Mingun deleted the bc-cleanup branch May 21, 2021 13:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move all JS codegeneration to the `generate-js` pass #117

Move all JS codegeneration to the `generate-js` pass #117

Mingun commented Apr 27, 2021

StoneCypher commented Apr 27, 2021

StoneCypher commented Apr 27, 2021

Mingun commented Apr 27, 2021

StoneCypher commented Apr 27, 2021

Mingun commented Apr 27, 2021

StoneCypher commented Apr 27, 2021

StoneCypher commented Apr 27, 2021

hildjj commented Apr 28, 2021

hildjj left a comment

hildjj commented May 2, 2021

Mingun commented May 3, 2021

hildjj commented May 18, 2021

Mingun commented May 18, 2021

hildjj left a comment

hildjj May 18, 2021

Mingun May 18, 2021 •

edited

Loading

hildjj May 19, 2021 •

edited

Loading

Mingun May 20, 2021

hildjj May 20, 2021

Move all JS codegeneration to the generate-js pass #117

Move all JS codegeneration to the generate-js pass #117

Conversation

Mingun commented Apr 27, 2021

StoneCypher commented Apr 27, 2021

StoneCypher commented Apr 27, 2021

Mingun commented Apr 27, 2021

StoneCypher commented Apr 27, 2021

Mingun commented Apr 27, 2021

StoneCypher commented Apr 27, 2021

StoneCypher commented Apr 27, 2021

hildjj commented Apr 28, 2021

hildjj left a comment

Choose a reason for hiding this comment

hildjj commented May 2, 2021

Mingun commented May 3, 2021

hildjj commented May 18, 2021

Mingun commented May 18, 2021

hildjj left a comment

Choose a reason for hiding this comment

hildjj May 18, 2021

Choose a reason for hiding this comment

Mingun May 18, 2021 • edited Loading

Choose a reason for hiding this comment

hildjj May 19, 2021 • edited Loading

Choose a reason for hiding this comment

Mingun May 20, 2021

Choose a reason for hiding this comment

hildjj May 20, 2021

Choose a reason for hiding this comment

Move all JS codegeneration to the `generate-js` pass #117

Move all JS codegeneration to the `generate-js` pass #117

Mingun May 18, 2021 •

edited

Loading

hildjj May 19, 2021 •

edited

Loading