Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add library mode #456

Merged
merged 10 commits into from
Jan 27, 2024
Merged

Add library mode #456

merged 10 commits into from
Jan 27, 2024

Conversation

hildjj
Copy link
Contributor

@hildjj hildjj commented Dec 31, 2023

This is an internal-only interface for calling into rules in other grammars.

This is a prerequisite for being able to import rules in grammars. The idea is:

  • Treat start rules as if they are exported
  • Publish the valid start rules as a part of the parser's compiled JS interface, so that import * as foo from "./foo.js" will be possible
  • Allow a starting offset and silent mode to be passed in to parse
  • When calling a parser in "library mode":
    • Don't error if there is unparsed input still left at the end
    • Return an object that has the needed
  • Rough in the ability to call rules in another .js file. I've played with it by hand, but it will certainly need to change before it is complete to account for how library modules are imported. The idea is that anywhere that would normally call peg$parseMyLibrary_Foo() will call peg$callLibrary(MyLibrary, "foo")

I've got the start of the code that processes imports in another branch, but I wanted to have a discussion about this part first.

@hildjj hildjj requested a review from Mingun December 31, 2023 20:32
Copy link
Member

@Mingun Mingun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that we need something like this. I cannot say what implementation would the best.
Your overall idea is to build state object each time when parser finish it work and pass it to other parsers via options, each field separately. The other way to do the same is to have a state object and pass it around, but this could have performance implications. That state object could be part of API. Theoretically, we even can put into state some parsing functions that is independent of a parser (parseAny, parseLiteral, parseClass), but that will require us to have a runtime.

@hildjj
Copy link
Contributor Author

hildjj commented Jan 1, 2024

I agree with you about passing a state object around. That might be cleaner, but probably a hair slower.

The other thing I don't like is setting up all of the constants each time a remote rule runs. A good JS runtime will probably make that not a big problem, but it's still hard to read.

I'd like to get the interface right here, we can always optimize the generated code better later. I'm going to land a few more changes on top of this before merging to see what happens.

@hildjj
Copy link
Contributor Author

hildjj commented Jan 8, 2024

OK, I think this is pretty close -- it seems to work. I think this gets everything important in #292, plus several more things. @Mingun would you like me to use opcode 43 instead of 41?

@frostburn
Copy link
Contributor

From the PR description:

Return an object that has the needed

The needed what?

@hildjj
Copy link
Contributor Author

hildjj commented Jan 8, 2024

From the PR description:

Return an object that has the needed

The needed what?

sorry. Has the needed properties for parsing to continue on with the appropriate state.

@hildjj
Copy link
Contributor Author

hildjj commented Jan 8, 2024

This still needs docs.

@frostburn
Copy link
Contributor

Managed to import a rule from a package hosted on GitHub: https://github.com/frostburn/peggy-string-concat/blob/main/index.peggy

import bar from "./lib.js" // Default rule
import {baz} from "./lib.js" // One rule imported into this namespace
import {
boo, bab,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to Wikipedia the canonical metasyntactic variables are foo, bar, baz, qux, quux, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is worth changing in a test.

@hildjj
Copy link
Contributor Author

hildjj commented Jan 10, 2024

Managed to import a rule from a package hosted on GitHub: https://github.com/frostburn/peggy-string-concat/blob/main/index.peggy

Did you check in those changes to peggy-string-concat? I was expecting to see an import statement at the top of https://github.com/frostburn/peggy-string-concat/blob/main/index.peggy which called to a compiled .js version of peggy-string.

@frostburn
Copy link
Contributor

Managed to import a rule from a package hosted on GitHub: https://github.com/frostburn/peggy-string-concat/blob/main/index.peggy

Did you check in those changes to peggy-string-concat? I was expecting to see an import statement at the top of https://github.com/frostburn/peggy-string-concat/blob/main/index.peggy which called to a compiled .js version of peggy-string.

I'm not sure what you're asking. I changed my peggy dependencies in both repositories to hildjj:library-mode. In peggy-string there's now a module.exports at the end of the compiled file and in peggy-string-concat there's now var peg$import0 = require("peggy-string"); at the start of the compiled file.

@hildjj
Copy link
Contributor Author

hildjj commented Jan 11, 2024

I'm not sure what you're asking.

I'm not either. After looking again and trying your code, it works great, exactly as expected. I appreciate your work!

CHANGELOG.md Outdated Show resolved Hide resolved
@hildjj hildjj merged commit 2e3cfd4 into peggyjs:main Jan 27, 2024
9 checks passed
@hildjj hildjj deleted the library-mode branch January 27, 2024 18:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants