Add support for plucking values with @ #89

hildjj · 2021-04-20T21:49:15Z

This still needs docs, and prototyping to make sure we can add other @-related things later.

hildjj · 2021-04-20T21:50:34Z

I back-ported the code from pegjs:HEAD, and got the tests to run. Figured I'd check in here so people could start to comment. I'm not 100% sold on this being the way forward, but having something to comment on felt like the next step.

lib/compiler/passes/generate-bytecode.js

hildjj · 2021-04-20T21:59:49Z

lib/compiler/passes/generate-js.js

@@ -3,7 +3,7 @@
 const asts = require("../asts");
 const js = require("../js");
 const op = require("../opcodes");
-const VERSION = require('../../version')
+const VERSION = require("../../version");


unrelated lint bug that needs to be landed even if this patch doesn't.

I think that this should be in the it's own PR or, at least, commit, together with other two unrelated changes in the package.json

hildjj · 2021-04-20T22:03:10Z

test/unit/compiler/passes/generate-bytecode.spec.js

+      ]));
+
+      expect(pass).to.changeAST("start = 'a' @'b' @'c'", bytecodeDetails([
+        // write a tool to generate comments


I decided to punt on making this pretty since we have #81 in the backlog.

hildjj · 2021-04-21T06:15:19Z

I've fiddled around with the grammar, and this will work just fine if we want to add decorators later:

@start   // current start rule
@memoize("first", {lru: 12}) // we can decide how interesting we want the parameters to be
first = @"foo" " "

This was the use case I was most worried about impacting. I'm also interested in how we're going to do things like importing other grammars. Do people have existing syntax for that in other forks?

Mingun

Also need to add an opcode description to the header comment of the generate-bytecode.js

examples/json.pegjs

test/unit/compiler/passes/generate-bytecode.spec.js

lib/compiler/passes/generate-bytecode.js

Mingun · 2021-04-21T07:01:38Z

lib/compiler/passes/report-incorrect-plucking.js

+          );
+        }
+
+        if (node.expression.type.startsWith("semantic_")) {


It is better to forbid that on the grammar level. I understand that you just picked up the existing implementation, so that can be done in the follow up PR, but personally I prefer to keep the commit history as clean as we can (without "fix" commits). So if it possible it is better to make now

The easiest fix is to just call error inside of LabeledExpression if the type of the expression is bad, but that feels like a cheat. It's important that @$[a-z]i+ works, which is also in PrefixedExpression. Refactoring the grammar to get all of that nuance in feels like it would be more confusing than the quick fix, though.

Opinions?

Mingun · 2021-04-21T07:14:53Z

Do people have existing syntax for that in other forks?

I have one, but I don't like it more (pegjs/pegjs#308). I tend to use JS-familiar syntax:

import rule_name from 'grammar';
import {rule1, rule2 as alias} from "grammar";

// usage
start
  = rule_name
  / rule1
  / alias
  ;

grammar are usually will be path (relative or absolute, according to JS rules of the import clause) to the grammar, but it also can be any identifier. Really it will depend on the import resolver.

hildjj · 2021-04-21T07:37:00Z

Also need to add an opcode description to the header comment of the generate-bytecode.js

@Mingun can you look at this please? I'm not familiar enough with the idioms used in those comments:

// [30] PLUCK
//
//        stack.pop(ip + 1);
//        stack.push(ip + 3);
//        ...
//        stack.push(ip + 3 + [ip + 2]);

the layout is [op.PLUCK, total_number_of_expression_items, number_of_plucks, pluck0, pluck1, ...]

Mingun · 2021-04-21T14:48:24Z

In order to make bytecode compatible with my other PRs, that I plan to port in the nearing future (ranges) it will make sense if you change PLUCK opcode number to 36, as in my original implementation. That will save me time when I prepare ranges PR :). We always can renumber opcodes later.

Then the comment will look that:

// [36] PLUCK n, k, p1, ..., pK
//
//        value = [stack[p1], ..., stack[pK]]; // when k != 1
//        -or-
//        value = stack[p1];                   // when k == 1
//
//        stack.pop(n);
//        stack.push(value);
//

(stack is the stack of values, parsed from the input, and positions in the input, and the bytecode manipulates it. This comment say, that opcode PLUCK firstly gets k values from specified positions in the stack, then replaced the last n values in the stack with the array of the selected elements/one specified element)

hildjj · 2021-04-21T17:24:00Z

change PLUCK opcode number to 36

Done. See if you're willing to live with my change to the semantic_ checks, please?

hildjj · 2021-04-21T17:24:58Z

I'm going to squash this commit once I have @Mingun 's review of my latest changes.

Mingun

Except the incorrect comment all good 👍 LGTM

Done. See if you're willing to live with my change to the semantic_ checks, please?

Yes, good change

Mingun · 2021-04-21T20:01:22Z

lib/compiler/passes/generate-bytecode.js

+// [30] PLUCK
+//
+//        stack.pop(ip + 1);
+//        stack.push(ip + 3);
+//        ...
+//        stack.push(ip + 3 + [ip + 2]);
+//


That is not the same thing as I described. In the end stack should contain one Array object with k elements, not the k elements, as you might think from the current description

Suggested change

// [30] PLUCK

//

// stack.pop(ip + 1);

// stack.push(ip + 3);

// ...

// stack.push(ip + 3 + [ip + 2]);

//

// [36] PLUCK n, k, p1, ..., pK

//

// value = [stack[p1], ..., stack[pK]]; // when k != 1

// -or-

// value = stack[p1]; // when k == 1

//

// stack.pop(n);

// stack.push(value);

//

oh. heh, i didn't save that buffer. willfix

hildjj · 2021-04-21T20:35:37Z

Squashed.

Mingun · 2021-04-22T13:57:48Z

Just remembered: it is good practice to update CHANGELOG.md in the PRs, so when you prepare release it will be easy to grab all the changes

Mingun · 2021-04-22T14:13:13Z

lib/compiler/passes/report-incorrect-plucking.js

+// Compiler pass to ensure the following are enforced:
+//
+//   - plucking can not be done with an action block
+//   - cannot pluck a semantic predicate


Suggested change

// - cannot pluck a semantic predicate

Checked in the grammar

Mingun · 2021-04-22T14:21:29Z

test/unit/compiler/passes/report-incorrect-plucking.spec.js

+  function reports(message, edgecases) {
+    it(message, () => {
+      edgecases.forEach(grammar => expect(pass).to.reportError(grammar));
+    });
+  }


Hadn't noticed before... need to check that location information in the error reports correct location. Here or whatever where similar checks is performed

Willfix by undoing this reports function and inlining the two tests.

hildjj · 2021-04-22T14:58:43Z

Just remembered: it is good practice to update CHANGELOG.md in the PRs, so when you prepare release it will be easy to grab all the changes

Good call, will do

hildjj · 2021-04-22T17:12:21Z

OK, @Mingun I've got all of your feedback in, I think. Please double-check before I merge?

hildjj · 2021-04-22T17:42:16Z

Fixes #38

Mingun

For the sake of code purity, could you make a little rebase?

fix lint errors (single quote to double quote, etc.)
change matrix
add changelog for not mentioned changes
implement plucking without regenerating parser
- actual implementation
- documentation
- changelog
- tests
- maybe examples
regenerate parser (examples also could be updated here)

In that case in the each point in the history project will able to build without errors and each commit will do the only one thing.

Fuhh... I think, that's all. Good work!

Mingun · 2021-04-22T17:33:23Z

test/unit/compiler/passes/report-incorrect-plucking.spec.js

+describe("compiler pass |reportIncorrectPlucking|", function() {
+  it("prevents \"@\" from being used with an action block", function() {
+    expect(pass).to.reportError("start1 = 'a' @'b' 'c' { /* empty action block */ }", {
+      message: "\"@\" cannot be used with an action block at line 1, column 14."


Suggested change

message: "\"@\" cannot be used with an action block at line 1, column 14."

message: "\"@\" cannot be used with an action block at line 1, column 14.",

location: {

start: { offset: 13, line: 1, column: 14 },

end: { offset: 17, line: 1, column: 18 }

}

Actually, text in the message should point to the beginning of the action block (opening {), but right now:

that information is unavailable (action block location covers expression and the action itself)

other check passes that work with actions also should be modified

So for now will live with that

test/unit/compiler/passes/report-incorrect-plucking.spec.js

hildjj · 2021-04-22T17:53:48Z

For the sake of code purity, could you make a little rebase?

I'm willing to do that, but don't think i've done a rebase in that direction before. Can you point me to docs, or give me quick set of bullets on the mechanics, please?

hildjj · 2021-04-22T17:55:22Z

nm, i found https://stackoverflow.com/a/6217314/8388 which has the info i need.

…ency

hildjj · 2021-04-22T18:22:14Z

Done. I removed the matrix change, since @StoneCypher is making that change in his PR in progress.

hildjj · 2021-04-22T19:06:06Z

@Mingun final-final approval? I want to get this landed and cut a release.

Mingun · 2021-04-22T19:13:31Z

Yes, final 👍

hildjj commented Apr 20, 2021

View reviewed changes

lib/compiler/passes/generate-bytecode.js Show resolved Hide resolved

hildjj commented Apr 20, 2021

View reviewed changes

hildjj marked this pull request as ready for review April 21, 2021 06:15

Mingun suggested changes Apr 21, 2021

View reviewed changes

examples/json.pegjs Outdated Show resolved Hide resolved

test/unit/compiler/passes/generate-bytecode.spec.js Outdated Show resolved Hide resolved

test/unit/compiler/passes/generate-bytecode.spec.js Outdated Show resolved Hide resolved

Mingun reviewed Apr 21, 2021

View reviewed changes

Mingun suggested changes Apr 21, 2021

View reviewed changes

hildjj force-pushed the pick branch from 122ca59 to f8a44d5 Compare April 21, 2021 20:32

hildjj mentioned this pull request Apr 21, 2021

Add support for picking fields with @ #38

Closed

Mingun reviewed Apr 22, 2021

View reviewed changes

Mingun mentioned this pull request Apr 22, 2021

Add grammarSource option and new source key to the location information #95

Merged

hildjj mentioned this pull request Apr 22, 2021

Make scripts work on Windows #94

Closed

Mingun suggested changes Apr 22, 2021

View reviewed changes

hildjj added 2 commits April 22, 2021 12:12

Lint fixes. Ensure package-lock is created. Remove nve as a devDepend…

3e12110

…ency

Update CHANGELOG.md for previous fixes

1110179

hildjj added 5 commits April 22, 2021 12:16

Implement @ plucking

75854c4

documentation

4316fcc

Update CHANGELOG.md

b360827

Add tests

24152b7

Update grammar and examples

4a1a4dc

hildjj force-pushed the pick branch from 5bdc64e to 4a1a4dc Compare April 22, 2021 18:21

hildjj mentioned this pull request Apr 22, 2021

Version floor conflict #100

Closed

Mingun mentioned this pull request Apr 22, 2021

Grammar action errors need to be more informative pegjs/pegjs#668

Closed

hildjj mentioned this pull request Apr 22, 2021

Release 1.1 #101

Closed

hildjj merged commit b5b0f39 into peggyjs:main Apr 22, 2021

hildjj deleted the pick branch April 22, 2021 19:16

hildjj mentioned this pull request Apr 27, 2021

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

Closed

Mingun mentioned this pull request May 24, 2022

Peggy vs PEG.js #249

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for plucking values with @ #89

Add support for plucking values with @ #89

hildjj commented Apr 20, 2021

hildjj commented Apr 20, 2021

hildjj Apr 20, 2021

Mingun Apr 22, 2021

hildjj Apr 20, 2021

hildjj commented Apr 21, 2021

Mingun left a comment

Mingun Apr 21, 2021

hildjj Apr 21, 2021

Mingun commented Apr 21, 2021 •

edited

Loading

hildjj commented Apr 21, 2021

Mingun commented Apr 21, 2021 •

edited

Loading

hildjj commented Apr 21, 2021

hildjj commented Apr 21, 2021

Mingun left a comment •

edited

Loading

Mingun Apr 21, 2021

hildjj Apr 21, 2021

hildjj commented Apr 21, 2021

Mingun commented Apr 22, 2021

Mingun Apr 22, 2021

Mingun Apr 22, 2021

hildjj Apr 22, 2021

hildjj commented Apr 22, 2021

hildjj commented Apr 22, 2021

hildjj commented Apr 22, 2021

Mingun left a comment

Mingun Apr 22, 2021

hildjj commented Apr 22, 2021

hildjj commented Apr 22, 2021

hildjj commented Apr 22, 2021

hildjj commented Apr 22, 2021

Mingun commented Apr 22, 2021

-      message: "\"@\" cannot be used with an action block at line 1, column 14."
+      message: "\"@\" cannot be used with an action block at line 1, column 14.",
+      location: {
+        start: { offset: 13, line: 1, column: 14 },
+        end: { offset: 17, line: 1, column: 18 }
+      }

Add support for plucking values with @ #89

Add support for plucking values with @ #89

Conversation

hildjj commented Apr 20, 2021

hildjj commented Apr 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hildjj commented Apr 21, 2021

Mingun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mingun commented Apr 21, 2021 • edited Loading

hildjj commented Apr 21, 2021

Mingun commented Apr 21, 2021 • edited Loading

hildjj commented Apr 21, 2021

hildjj commented Apr 21, 2021

Mingun left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hildjj commented Apr 21, 2021

Mingun commented Apr 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hildjj commented Apr 22, 2021

hildjj commented Apr 22, 2021

hildjj commented Apr 22, 2021

Mingun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hildjj commented Apr 22, 2021

hildjj commented Apr 22, 2021

hildjj commented Apr 22, 2021

hildjj commented Apr 22, 2021

Mingun commented Apr 22, 2021

Mingun commented Apr 21, 2021 •

edited

Loading

Mingun commented Apr 21, 2021 •

edited

Loading

Mingun left a comment •

edited

Loading