Multi file #417

hildjj · 2023-04-28T22:27:24Z

This is a PR draft exploring another part of #292.

What if you could just pass multiple files in on the command line, and have Peggy stitch them together into a single output?

This points out a few issues:

Code generation does not rely on source information in the AST adequately. Instead, it guesses based on the grammarSource passed in. This is another strong argument for changing how we do code generation.
topLevelInitializer and initializer in the AST should be arrays. This is so that when they get copied into the output, each initializer section would be marked up with the correct source-map info.
There might be imports in the top-level initializers of multiple files. They should probably all be moved to the top, but this is a JS-specific transform that should be in a compiler pass.

hildjj · 2023-04-28T22:33:57Z

There's a timing bug in the watchfile tests as well. Will deal with it if we end up using this.

hildjj · 2023-12-10T04:58:18Z

I think the sourcemap thing isn't an issue. This works surprisingly well already. Still needs a compiler pass that checks if output type is "es" and looks for import lines to move to the top.

hildjj · 2023-12-11T23:35:11Z

Still needs docs, a changelog entry, and some thought about how this affects the version number.

hildjj · 2023-12-12T21:39:36Z

Being able to specify a URL in the CLI would be pretty cool. In order to do this, I'd look for filenames starting with https?://, use fetch to pull down the contents, and cache it, like deno. This would up the minimum version of node required to be v18 -- That's fine with me because:

a) we're taking a breaking change in this release for node version anyway.
b) Node 16 is no longer supported.

This would make importing rules from a central repository easier, I think.

bin/peggy-cli.js

frostburn · 2023-12-22T13:35:03Z

Tested combining grammars from two repositories successfully: https://github.com/frostburn/peggy-string-concat

frostburn · 2023-12-23T07:15:58Z

bin/peggy-cli.js

        }
        this.outputFile = this.progOptions.output;
        this.outputJS = this.progOptions.output;

-        if ((this.inputFile === "-") && this.argv.watch) {
+        if ((this.inputFiles.indexOf("-") !== -1) && this.argv.watch) {


How about the more readable inputFiles.includes("-") insteads?

frostburn · 2023-12-23T07:26:50Z

bin/peggy-cli.js

+          const req = (
+            // In node 12+, createRequire is documented.
+            // In node 10, createRequireFromPath is the least-undocumented approach.
+            Module.createRequire || Module.createRequireFromPath


Using nullish coalescing ?? would be more idiomatic here. Are we on a node version that doesn't support it? Not seeing it anywhere in the source.

@hildjj, you say that you would like to bump required node version to 18, so you can use only createRequire

I think createRequire is in node12, but I'm fine bumping to Node18 required, and using anything new we want in the CLI code. The web-facing code we should be a little more careful with.

Just removed node 10 compatibility.

Mingun

The implementation looks good, but I don't like the fact that peggy grammar becomes tied to JS. That is the same problem that we have now for unbalanced {} which could be present in JS comments or strings and actually does not create unbalanced braces, but peggy grammar does not understand that because code blocks are language-agnostic.

I suggest to merge this without changes in the grammar and propose grammar changes in separate PR. Ideally we should find a way to make grammar modularised so language plugins can provide they own methods to parse code blocks.

Mingun · 2023-12-25T05:10:29Z

bin/peggy-cli.js

+          if (this.inputFiles.indexOf("-") === -1) {
+            let inFile = this.inputFiles[0];
+            // You might just want to run a fragment grammar as-is,
+            // particularyly with a specified start rule.


Misprint

Suggested change

// particularyly with a specified start rule.

// particularly with a specified start rule.

Mingun · 2023-12-25T05:15:49Z

bin/peggy-cli.js

+          const req = (
+            // In node 12+, createRequire is documented.
+            // In node 10, createRequireFromPath is the least-undocumented approach.
+            Module.createRequire || Module.createRequireFromPath


@hildjj, you say that you would like to bump required node version to 18, so you can use only createRequire

Mingun · 2023-12-25T05:18:13Z

bin/peggy-cli.js

+            // In node 10, createRequireFromPath is the least-undocumented approach.
+            Module.createRequire || Module.createRequireFromPath
+          )(prevSource);
+          prevSource = req.resolve(source.slice(4));


Maybe add a comment, what parts are skipped here.

Mingun · 2023-12-25T05:20:53Z

bin/watcher.js

        }
+        const filename = path.join(dir, fn);
+        // Might be a different fil changing in one of the target dirs


Misprint

Suggested change

// Might be a different fil changing in one of the target dirs

// Might be a different file changing in one of the target dirs

Mingun · 2023-12-25T05:24:36Z

docs/documentation.html

+<p>If you specify multiple input files, they will be folded together in the
+  order specified before generating a parser.  <code>import</code> statements in
+  the top-level initializers from each of the inputs will be moved to the top of
+  the generated code, and all other top-level initializers will be inserted
+  directly after those imports, in the order of the inputs.  This approach can
+  be used to keep libraries of often-used grammar rules in separate files.</p>
+


Maybe document possible caveats of automatic import movement. For example, if imports in the block comment (/*...*/), would them be moved?

See if the comment I added was good enough? Importantly, the ECMAscript grammar treats all comments after an import as part of the import.

Mingun · 2023-12-25T05:31:05Z

lib/compiler/asts.js

+  // Put library code above code that uses it, so that variables
+  // will be in scope.
+  const res = bb.filter(x => x).concat(aa.filter(x => x)); // Remove nulls.
+  return res;


It seems to me that comment should be at call site rather than here

Suggested change

// Put library code above code that uses it, so that variables

// will be in scope.

const res = bb.filter(x => x).concat(aa.filter(x => x)); // Remove nulls.

return res;

return bb.filter(x => x).concat(aa.filter(x => x)); // Remove nulls.

Agree. As well, if we aren't generating es-js, the inputs should not be reordered. That's a decision for the generator.

Mingun · 2023-12-25T05:35:26Z

src/parser.pegjs

@@ -57,14 +57,64 @@ Grammar
    }

 TopLevelInitializer
-  = "{" code:CodeBlock "}" EOS {
-      return {
+  = "{" "{" imports:ImportDeclarations code:BareCodeBlock "}" "}" EOS {


This can create problems for plugins that add support for other languages, because they would have other mechanisms for imports.

Yes, I agree this is an issue. I suppose we could have a sub-grammar that can be used in generate-js to perform this transform.

I've implemented the sub-grammar approach, which only gets called from generate-js, and only if we are in "es" format. This should be a lot safer.

hildjj · 2023-12-26T17:31:48Z

Another patch coming with other issues fixed.

hildjj · 2023-12-26T17:48:03Z

OK, I think I fixed all of the outstanding issues. I've left this as two patches since review to make it easier to see the changes, but I'll reorder and clean up the patch before merging.

frostburn · 2023-12-27T06:38:49Z

src/js-imports.peggy

+  / StringLiteral
+
+BindingIdentifier = id:IdentifierName &{
+  return reservedWords.indexOf(id[0]) === -1


.includes instead

No, this one needs to run in the browser.

frostburn · 2023-12-27T06:39:05Z

bin/peggy-cli.js

          this.argv.watch = false; // Make error throw.
          this.error("Can't watch stdin");
        }

        if (!this.outputFile) {
-          if (this.inputFile !== "-") {
-            this.outputJS = this.inputFile.slice(
+          if (this.inputFiles.indexOf("-") === -1) {


.includes instead

hildjj · 2023-12-27T18:13:48Z

This is ready to squash and merge.

…nto one output. Not ready. Still needs change to source-map generation, for topLevelInitializer to be an array, and to make moving imports to the top of all topLevelInitializers (keeping track of source info) a JS-specific pass.

hildjj · 2023-12-27T22:19:56Z

ready to go if anyone wants to take a last look. Next step is to merge the imports grammar back into the main grammar, and give it two entry points. Then, I'll use the imports grammar to add imports syntax before toplevel initializers.

hildjj force-pushed the multi-file branch from 4c1fcc4 to ae3e5c8 Compare July 13, 2023 18:44

hildjj force-pushed the multi-file branch from ae3e5c8 to a47755c Compare August 18, 2023 03:40

hildjj force-pushed the multi-file branch from a47755c to c944934 Compare December 10, 2023 04:57

hildjj force-pushed the multi-file branch 3 times, most recently from 519d96f to 121273b Compare December 11, 2023 21:14

hildjj marked this pull request as ready for review December 11, 2023 23:33

hildjj requested a review from Mingun December 11, 2023 23:33

hildjj mentioned this pull request Dec 12, 2023

Implement syntax for importing rules #292

Closed

hildjj force-pushed the multi-file branch from bd9d3d6 to c2abcbe Compare December 12, 2023 19:52

hildjj mentioned this pull request Dec 13, 2023

Importing grammar rules from a file #298

Closed

Mingun reviewed Dec 22, 2023

View reviewed changes

bin/peggy-cli.js Outdated Show resolved Hide resolved

frostburn reviewed Dec 23, 2023

View reviewed changes

Mingun reviewed Dec 25, 2023

View reviewed changes

frostburn reviewed Dec 27, 2023

View reviewed changes

hildjj added 4 commits December 27, 2023 15:12

Docs and changelog entry

a1b8bfc

Fix timing issue in debounce test

e5df72e

Update dependencies

7015aa3

hildjj force-pushed the multi-file branch from acf7b7f to 7015aa3 Compare December 27, 2023 22:14

hildjj merged commit cbea181 into peggyjs:main Dec 28, 2023
9 checks passed

hildjj deleted the multi-file branch December 28, 2023 16:35

hildjj added a commit to hildjj/peggy that referenced this pull request Jan 8, 2024

Forgot to update input types for generate() in peggyjs#417

8365600

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multi file #417

Multi file #417

hildjj commented Apr 28, 2023

hildjj commented Apr 28, 2023

hildjj commented Dec 10, 2023

hildjj commented Dec 11, 2023 •

edited

Loading

hildjj commented Dec 12, 2023

frostburn commented Dec 22, 2023

frostburn Dec 23, 2023

frostburn Dec 23, 2023

Mingun Dec 25, 2023

hildjj Dec 25, 2023

hildjj Dec 26, 2023

Mingun left a comment

Mingun Dec 25, 2023

Mingun Dec 25, 2023

Mingun Dec 25, 2023

Mingun Dec 25, 2023

Mingun Dec 25, 2023

hildjj Dec 26, 2023

Mingun Dec 25, 2023

hildjj Dec 26, 2023

Mingun Dec 25, 2023

hildjj Dec 25, 2023

hildjj Dec 26, 2023

hildjj commented Dec 26, 2023

hildjj commented Dec 26, 2023

frostburn Dec 27, 2023

hildjj Dec 27, 2023

frostburn Dec 27, 2023

hildjj commented Dec 27, 2023

hildjj commented Dec 27, 2023

	// particularyly with a specified start rule.
	// particularly with a specified start rule.

	// Might be a different fil changing in one of the target dirs
	// Might be a different file changing in one of the target dirs

Multi file #417

Multi file #417

Conversation

hildjj commented Apr 28, 2023

hildjj commented Apr 28, 2023

hildjj commented Dec 10, 2023

hildjj commented Dec 11, 2023 • edited Loading

hildjj commented Dec 12, 2023

frostburn commented Dec 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hildjj commented Dec 26, 2023

hildjj commented Dec 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hildjj commented Dec 27, 2023

hildjj commented Dec 27, 2023

hildjj commented Dec 11, 2023 •

edited

Loading