Source map support by Mingun · Pull Request #163 · peggyjs/peggy

Mingun · 2021-06-01T18:41:11Z

Draft PR for getting feedback about general conception.

Probably will fix #105, but I need to learn how I can test that. I've never working with source maps before so I need learn how to use it for debugging. If anyone have a link to a good course, please let me known.

Notes about implementation: I'm not sure that introducing a new output type is a good idea. Maybe a new option sourceMap: true will be better?

humanchimp · 2021-06-25T00:03:31Z

thanks for working on this. as far as grokking sourcemaps goes, I've seen a bunch of different visualization tools, but this is pretty groovy, IMO: https://evanw.github.io/source-map-visualization/

Mingun · 2021-06-26T07:36:48Z

Thanks for the link! Visualization looks great, but unusable, unfortunately. The author, it seems, forgot to provide a way to upload source files. The generated file and it map, obviously, is not enough. But I found another same-named project -- https://sokra.github.io/source-map-visualization.

o-alexandre-felipe · 2021-07-08T15:20:24Z

@Mingun,

Thank for the for working in the much better approach, looking forward to having this feature. When do you think this will be ready?

Looking into your implementation I have a few doubts.

The SourceNode will be able to map the statements when you give a block of code? (I imagined you would have to tokenize it before)

    push(new SourceNode(
      ast.topLevelInitializer.codeLocation.start.line,
      node.codeLocation.start.column - 1,
      options.grammarSource,
      ast.topLevelInitializer.code,
      node.type
    ));

It seems that your function definition

function buildFunc(a, i) {
      return new SourceNode(
        null, null, options.grammarSource,
        [
          "\n  var " + f(i) + " = function(" + a.params.join(", ") + ") {",
          a.source,
          "};"
        ],
        f(i)
      );
    }

Will align incorrectly the action code with the beginning of the line (before var), the intention wasn't something like this?

new SourceNode(null, null, null, [
    "\n var " + f(i) + " = function(" a.params.join(", ") + ") {",
    new SourceNode(null, null, options.grammarSource, a.source),
    "};"
  ],
  a.source
);

I see that you are assigning source maps to the javascript (precisely the part I ignored in my solution), but where are you mapping the rests? I can't see where you set the source maps for Rules/Literals and classes (regular expressions).

Mingun · 2021-07-08T16:45:08Z

The SourceNode will be able to map the statements when you give a block of code? (I imagined you would have to tokenize it before)

Yes, that's what it's designed for.

Will align incorrectly the action code with the beginning of the line (before var), the intention wasn't something like this?

As you can see in the regenerated parser.js, indentation is correct.

I can't see where you set the source maps for Rules/Literals and classes (regular expressions).

Only code blocks need source map because all other pieces of grammar you cannot debug.

I plan to finish work on this after #167 will merge in order to avoid merge conflicts in CLI, but @hildjj taken a little vacation, I think. Actually, locally I've already done some job and fixed some moments after testing with visualizer. Actually, this work can be finished in couple days. I think I'll do it this weekend.

hildjj · 2021-07-08T17:05:50Z

Yes, sorry, I've had other things I was working on. I'll circle back around to 167 in the next few days.

o-alexandre-felipe · 2021-07-08T17:15:10Z

The SourceNode will be able to map the statements when you give a block of code? (I imagined you would have to tokenize it before)
Yes, that's what it's designed for.

Well, it was designed to receive one Node (token), and the corresponding position in the source file, if you send a big chunk only the first token will be mapped, and in this case the first token is not an identifier, so it is like if it was not mapped.

Will align incorrectly the action code with the beginning of the line (before var), the intention wasn't something like this?

As you can see in the regenerated parser.js, indentation is correct.

Sorry if that was ambiguous, when I said alignment I meant the association between the position in the generated code and the position in the source code.

It seems you are aligning var with the start of the action.

I can't see where you set the source maps for Rules/Literals and classes (regular expressions).

Only code blocks need source map because all other pieces of grammar you cannot debug.

What do you mean with I cannot debug? By debugging I mean to be able to run step by step over the pieces that may consume some input.

All you have to do is to choose one token of the generated code as the anchor for the rule/literal/class.

I plan to finish work on this after #167 will merge in order to avoid merge conflicts in CLI, but @hildjj taken a little vacation, I think. Actually, locally I've already done some job and fixed some moments after testing with visualizer. Actually, this work can be finished in couple days. I think I'll do it this weekend.

Thank you!

o-alexandre-felipe · 2021-07-19T16:50:33Z

Hi @Mingun and @hildjj I am looking forward to use this feature. I closed my pull request after you mentioned this work, since I think is not the best use of my time to work on code that someone else is already doing. Are you still working on this or should I resume what I started, or would you prefer to setup a branch that I can work from this work as a starting point.

Thanks

Mingun · 2021-07-20T16:58:54Z

Sorry for the delay, the new old interesting project captured my attention :). I will try to mend.

Well, it was designed to receive one Node (token), and the corresponding position in the source file, if you send a big chunk only the first token will be mapped, and in this case the first token is not an identifier, so it is like if it was not mapped.

Large parts of the generated parser code have not any representation in the source grammar. I've tested my mapping on https://sokra.github.io/source-map-visualization and it seems it works quite well. There are some errors in the mapping but it seems unavoidable because source-map by design maps only start coordinates of chunks, but not their length, so you can observe artifacts as on that image:

If you has thoughts how this can be fixed, you are welcome!

Sorry if that was ambiguous, when I said alignment I meant the association between the position in the generated code and the position in the source code.

It seems you are aligning var with the start of the action.

In my tests on https://sokra.github.io/source-map-visualization it seems to working correctly.

What do you mean with I cannot debug? By debugging I mean to be able to run step by step over the pieces that may consume some input.

I mean that this is not an executable code. You cannot put breakpoint in it. You shows me an interesting way to use the map, I not thinking about it. But I do not really understand how this can be done now because lengths of original and generated strings is different (rule vs peg$rule) and SourceMap allow only mapping of the same-length strings (at least I do not see any clear way how to map strings with non-identical lengths). So this will be outside of scope of my PR, at least for now, but you can start prepare your own PR based on mine :).

o-alexandre-felipe · 2021-07-21T14:16:24Z

Sorry for the delay, the new old interesting project captured my attention :). I will try to mend.

That's fine, many projects out there

If you has thoughts how this can be fixed, you are welcome!

Yes I have an idea about how to do this, the javascript should be the easy part. Also it would be fine if we extended the javascript code rule in the peggy parser, so that we could build the JS AST and also report errors properly. But that is a big change.

I mean that this is not an executable code. You cannot put breakpoint in it. You shows me an interesting way to use the map, I not thinking about it. But I do not really understand how this can be done now because lengths of original and generated strings is different (rule vs peg$rule) and SourceMap allow only mapping of the same-length strings (at least I do not see any clear way how to map strings with non-identical lengths). So this will be outside of scope of my PR, at least for now, but you can start prepare your own PR based on mine :).

Thanks, when I have the opportunity I will prepare a PR with the source maps only for the embedded source maps.

Mingun · 2021-08-02T19:32:33Z

I've finished this and it is ready for final review.
I've implemented two variants of API:

the first one in this PR: new boolean option sourceMap. When it is true and output is "source", compiler returns a SourceNode object. Usage:

let node = peggy.generate(grammar, {
  output: "source",
  // Required, see below
  grammarSource: "my-file",
  sourceMap: true,
});
let { code, map } = node.toStringWithSourceMap();
map.toJSON();// Get source map as JSON object
map.toString();// Get source map as a string (serialized JSON object)

the second one in the branch https://github.com/Mingun/peggy/tree/alternative-source-map-api: new output variant "source-and-map". Usage quite similar:

 let node = peggy.generate(grammar, {
-  output: "source",
+  output: "source-and-map",
   // Required, see below
   grammarSource: "my-file",
-  sourceMap: true,
 });
 let { code, map } = node.toStringWithSourceMap();
 map.toJSON();// Get source map as JSON object
 map.toString();// Get source map as a string (serialized JSON object)

So you can compare and choose what your like better. If an alternative API is more preferable, I add this commit to that PR.

Because of the mozilla/source-map#444 you can't use null, undefined and "" as a source file names. Because when we create a source map sources taken from the grammarSource option, it should be defined and not equal to the null, undefined, or "". source-map library automatically converts all other objects into strings, so be sure that your grammarSource.toString() returns a string representation that will be used in the source map.

o-alexandre-felipe · 2021-08-03T13:37:39Z

Nice work. I like the first option you propose.
I'm glad that you ended before I started (well I was off last week)

Mingun · 2021-08-17T17:06:24Z

@hildjj , I forgot to mention: the first commit should also include changes in package-lock.json, but I have npm 7.20.6 which produces only version 2 file, but repository contains version 1 file. Is there any reason why version 1 is used? Version 2 is backward-compatible and can be read by older npm.

hildjj · 2021-08-17T17:09:41Z

We had some issue testing on node10 with the newer package-lock format at some point. I've been generating the package-lock file with docker run --rm -it -v $PWD:/app -v ~/.npm/:/root/.npm --workdir /app node:12 npm install --package-lock-only since.

I continue to prefer solving this problem by not checking in the package-lock file, since we have no runtime dependencies.

hildjj · 2021-08-19T19:02:58Z

If you rebase, I'll prioritize reviewing this over the next few days. I'd like to get this in soon, have a few people try it, then do a release next week if possible.

Mingun · 2021-08-20T17:13:49Z

I plan to finish this this weekend.

Mingun · 2021-08-20T20:13:09Z

Rebased: new changes in those 2 commits: https://github.com/peggyjs/peggy/pull/163/files/e5c3522eeb8e05f694b72dbe09f10223db3c9d5f..74dbbfa62e203f9b862151ce63ca0b010e968a1a

hildjj

Mostly minor stuff, except that we need to get rollup to work cleanly, and have the output work in the browser. I like the follow-on patch with the API change, so let's go ahead and work from that point.

hildjj · 2021-08-22T18:19:06Z

lib/compiler/passes/generate-js.js

+      return toSourceNode(node.code, node.codeLocation, "$" + node.type);
+    }
+
+    return node.code;


should be able to reach this with a plugin

Could you expand your thought? If you think about something like ts-pegjs which originally will have a TypeScript code here, it should already translate it to the JS, but I suspect it have their own generate-ts pass. Or you talk about extracting this function into utils?

sorry i was terse. :) This line is missed in code coverage, and i think it should not be too difficult to write a small plugin in the test that would reach this line of code.

After we merge this, I'm going to do a PR that adds a bunch of code coverage, so I can fix it up then if you like.

I spent some time to understand that nyc command in the package.json used only for debug CLI and not required for ordinally run, because jest has built-in support for coverage, but the results are not showing in the console. I think we need to fix that. I can prepare a PR for that if you not already handle that in your upcoming PR.

Cover this line, will update PR soon

I think this should be a separate issue. Assign it to me, and I'll fix it when I touch the CLI.

bin/peggy.mjs

hildjj · 2021-08-22T18:51:38Z

package.json

    "rimraf": "^3.0.2",
    "rollup": "^2.56.2",
    "sinon": "^11.1.2",
+    "source-map": "^0.7.3",


This needs to be "^0.8.0-beta.0", which includes a fix so that rollup doesn't pull in fs and path. Then there's a change needed to rollup.config.js, add json(), to line 31 so that the mapping table included by that version of source-map will get pulled in correctly..

...and then there are a couple of other rollup issues behind that which I'm not sure why we haven't seen before. I'm going to put those aside for the moment and review the rest of this.

Ok. Actually, I doubted should I depend on the beta version of the project that not seems to be actively developed for 2 years for now. Even now, npm offers to install version 0.6.1 instead of the last available version (although now I realized that this is due to the fact that such version is specified in package-lock.js at the top level)

Maybe we should rethink our dependency on source-map. All we need is the generator, and I think it's the consumer that depends on tr46, etc, which adds the huge tables. I wonder if we could make https://github.com/a-la/source-map-generator work. It would need at least one patch to not use node's url, and rely on the built-in URL instead, but we can send them a PR or fork if needed. I think this would make rollup a lot easier and reduce the size of the generated code by a lot.

I'm working up that PR for source-map-generator, just in case.

I don't recall the specific issue I had with rollup, but I'll check out your current head to see if it shows up again.

The way I solved the test issue in source-map-generator was to use the original source-map as a dev dependency. I think that's what you said you tried. Anyway, I'll take a look here.

All our dependencies are dev dependencies, so yes, I tried have both and right now it seems that having https://github.com/hildjj/source-map-generator is just redundant

I almost have it working here, I think. Give me another hour or so, and I'll send a PR to your repo.

See Mingun#1

Changes merged

hildjj · 2021-08-22T20:02:20Z

README.md

  — more powerful than traditional LL(_k_) and LR(_k_) parsers
 - Usable [from your browser](https://peggyjs.org/online), from the command line,
  or via JavaScript API
+- [Source map](https://developer.mozilla.org/en-US/docs/Tools/Debugger/How_to/Use_a_source_map) support


This worked when I added a //# sourceMappingURL= comment to the end of the generated .js file, which I think should be done automatically.

This pointed out that the filenames in the .map file might need to be tweaked; from the project root, try ./bin/peggy.js examples/fizzbuzz.peggy --source-map. The map file includes "sources":["examples/fizzbuzz.peggy"] which should probably instead be `"sources":["fizzbuzz.peggy"]". The filename should be relative to the .map file.

I'm not sure that automatic appending of this line will be desired behavior in each case, mostly because I don't known how people will use source-map feature. Source maps could be hosted not in the same directory where it was generated and only the user know the right place. Adding this line is a trivial task, so I leaved it out of scope of my PR. We can firstly gather the feedback and add this line later if users will complain about it. Or, if you have a clear vision, I can add it now.

I think the line should be auto-appended in the CLI only. We can add an option for inlining it later.

The filename should be relative to the .map file

Done.

I think the line should be auto-appended in the CLI only. We can add an option for inlining it later.

I'll leave that for the next PRs. There a many options to forming such URL. For example, someone can want to form a data URL embedding generated source map in it.

I think this is going to take a few more PRs before we're ready to release, and I don't want to docs to be wrong for that long a time, so I'm going to create a "1.3" branch that we can merge this into, and start to merge other work into as well.

After this lands in the 1.3 branch, and we add in #199, we should have a better way to talk about how to test whether the maps are working as expected. Make sense @Mingun?

Yes, I think this is a good plan

bin/peggy.mjs

hildjj · 2021-08-22T21:34:53Z

lib/compiler/index.js

    switch (options.output) {
      case "parser":
-        return eval(ast.code);
+        return eval(ast.code.toString());


Is this is going to break ts-pegjs? I guess .toString is safe to add to a string, but I wonder if there's anything else that will break.

Are you worried that ts-pegjs might have replaced the code with its object? Yes, it should be upgraded to correctly handle the new sourceMap option (or new "output-with-map" output variant)

Let's get @pjmolina 's opinion early. We can move several of the functions in generate-js into utils to make them easier to reuse, if that would help.

lib/peg.d.ts

Mingun

I'll try to address review comments this week.

Mingun · 2021-08-23T10:45:14Z

README.md

  — more powerful than traditional LL(_k_) and LR(_k_) parsers
 - Usable [from your browser](https://peggyjs.org/online), from the command line,
  or via JavaScript API
+- [Source map](https://developer.mozilla.org/en-US/docs/Tools/Debugger/How_to/Use_a_source_map) support


I'm not sure that automatic appending of this line will be desired behavior in each case, mostly because I don't known how people will use source-map feature. Source maps could be hosted not in the same directory where it was generated and only the user know the right place. Adding this line is a trivial task, so I leaved it out of scope of my PR. We can firstly gather the feedback and add this line later if users will complain about it. Or, if you have a clear vision, I can add it now.

bin/peggy.mjs

Mingun · 2021-08-23T11:01:25Z

lib/compiler/index.js

    switch (options.output) {
      case "parser":
-        return eval(ast.code);
+        return eval(ast.code.toString());


Are you worried that ts-pegjs might have replaced the code with its object? Yes, it should be upgraded to correctly handle the new sourceMap option (or new "output-with-map" output variant)

Mingun · 2021-08-23T11:08:34Z

lib/compiler/passes/generate-js.js

+      return toSourceNode(node.code, node.codeLocation, "$" + node.type);
+    }
+
+    return node.code;


Could you expand your thought? If you think about something like ts-pegjs which originally will have a TypeScript code here, it should already translate it to the JS, but I suspect it have their own generate-ts pass. Or you talk about extracting this function into utils?

lib/peg.d.ts

Mingun · 2021-08-23T11:21:59Z

package.json

    "rimraf": "^3.0.2",
    "rollup": "^2.56.2",
    "sinon": "^11.1.2",
+    "source-map": "^0.7.3",


Ok. Actually, I doubted should I depend on the beta version of the project that not seems to be actively developed for 2 years for now. Even now, npm offers to install version 0.6.1 instead of the last available version (although now I realized that this is due to the fact that such version is specified in package-lock.js at the top level)

hildjj · 2021-10-04T19:54:39Z

Where do we stand on this? I've talked to the folks a Moz, and we're figuring out a better way to move forward in the long run. In the short run, it's ok to depend on https://github.com/hildjj/source-map-generator

Mingun · 2021-10-05T18:01:55Z

I'll return to this this weekend. Need to run coverage and see where I can improve it, as you noticed.

Ok, I'll switch to https://github.com/hildjj/source-map-generator.

Such joins stringify SourceNode's prematurally

Again, this prevents premature stringification of the SourceNodes

Here just for uniformity

…ionsBase

Regenerate parser, add documentation and tests

… be written Also exit with error code 2 when CLI fails because of invalid test input and update CLI tests to check exit code

…functions

… of `sourceMap` flag

Mingun · 2021-10-17T20:37:33Z

Latest changes:

included package-lock.js update in the first commit
added ef9f014:
- check exit code of the CLI in the tests
- exit with code 2 if error from the input test
- do not interrupt writing of the parser code to file if the tested input raises grammar error
e47b5f4
- increase coverage
- relativise paths in the source map
- handle --test/--test-file interaction
- add examples to documentation
fe39cbf -- cover ast2SourceNode as requested

Do not use https://github.com/hildjj/source-map-generator, because I need SourceMapConsumer in tests

…lder version or dropping node 10 support.

hildjj · 2021-10-18T20:13:43Z

Please move base branch to 1.3 and I'll merge.

Mingun · 2021-10-18T20:20:54Z

Done

Mingun mentioned this pull request Jun 26, 2021

How line and column of SourceNode instance related to the contained chunk(s)? mozilla/source-map#442

Open

Mingun force-pushed the source-map branch from 55247d7 to a16f8d7 Compare July 25, 2021 19:51

Mingun force-pushed the source-map branch from a16f8d7 to 5674b34 Compare August 2, 2021 19:01

Mingun marked this pull request as ready for review August 2, 2021 19:30

Mingun force-pushed the source-map branch from 5674b34 to ed99db6 Compare August 20, 2021 20:00

hildjj reviewed Aug 22, 2021

View reviewed changes

Mingun commented Aug 23, 2021

View reviewed changes

Mingun added 5 commits October 18, 2021 00:52

source-map: Create SourceNodes for functions and initializers

769a432

source-map: Generate tables of constants as a SourceNode

fcae22a

source-map: Remove excess .join("\n")s

de0ad72

Such joins stringify SourceNode's prematurally

source-map: Do not join arrays prematurely

4ad80de

Again, this prevents premature stringification of the SourceNodes

source-map: Remove yet another premature join

9bdf8eb

Here just for uniformity

Mingun and others added 13 commits October 18, 2021 01:01

source-map: Remove useless function

fa5da47

source-map: Generate top-level as a SourceNode

d482541

source-map: Generate module wrappers as a SourceNode

6eece33

source-map: Represent generated code in the AST as a SourceNode

8b7d69d

source-map: Don't Repeat Yourself - introduce new interface SourceOpt…

b352e92

…ionsBase

source-map: Implement API for producing source maps

c08e4d8

Regenerate parser, add documentation and tests

source-map: Run CLI tests in the test/cli directory

cadc935

source-map: Do not break process until generated code/source map will…

ef9f014

… be written Also exit with error code 2 when CLI fails because of invalid test input and update CLI tests to check exit code

source-map: Add new -m/--source-map parameter to the CLI

e47b5f4

source-map: Introduce helper for generate a parse rule function name

e5b1ebf

source-map: Add mapping for the rule names to the names of the parse …

1ee49ab

…functions

source-map: Cover ast2SourceNode completely

fe39cbf

source-map: Alternative API: new output mode "source-and-map" instead…

6f57b4a

… of `sourceMap` flag

Mingun force-pushed the source-map branch from ed99db6 to 6f57b4a Compare October 17, 2021 20:27

hildjj and others added 3 commits October 18, 2021 22:37

source-map: Make source-map work in the browser

4ee3472

Update all dependencies

0ac78fd

Only run lint on latest node, rather than holding back eslint to an o…

53f3d12

…lder version or dropping node 10 support.

Mingun changed the base branch from main to 1.3 October 18, 2021 20:20

hildjj merged commit 749913a into peggyjs:1.3 Oct 18, 2021

Mingun deleted the source-map branch October 18, 2021 20:25

Mingun mentioned this pull request Jan 3, 2022

Better/any systax errors from grammar eval #235

Closed

Mingun mentioned this pull request Oct 17, 2022

toStringWithSourceMap behaves incorrectly if SourceNodes source is null or is an empty string mozilla/source-map#444

Open

Conversation

Mingun commented Jun 1, 2021

Uh oh!

humanchimp commented Jun 25, 2021

Uh oh!

Mingun commented Jun 26, 2021

Uh oh!

o-alexandre-felipe commented Jul 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Mingun commented Jul 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hildjj commented Jul 8, 2021

Uh oh!

o-alexandre-felipe commented Jul 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

o-alexandre-felipe commented Jul 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Mingun commented Jul 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

o-alexandre-felipe commented Jul 21, 2021

Uh oh!

Mingun commented Aug 2, 2021

Uh oh!

o-alexandre-felipe commented Aug 3, 2021

Uh oh!

Mingun commented Aug 17, 2021

Uh oh!

hildjj commented Aug 17, 2021

Uh oh!

hildjj commented Aug 19, 2021

Uh oh!

Mingun commented Aug 20, 2021

Uh oh!

Mingun commented Aug 20, 2021

Uh oh!

hildjj left a comment

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!

Choose a reason for hiding this comment

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!

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!

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!

o-alexandre-felipe commented Jul 8, 2021 •

edited

Loading

Mingun commented Jul 8, 2021 •

edited

Loading

o-alexandre-felipe commented Jul 8, 2021 •

edited

Loading

o-alexandre-felipe commented Jul 19, 2021 •

edited

Loading

Mingun commented Jul 20, 2021 •

edited

Loading

Mingun commented Oct 17, 2021 •

edited

Loading