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

Grammar syntax for imports/includes #286

Open
Schahen opened this issue Jan 21, 2021 · 9 comments
Open

Grammar syntax for imports/includes #286

Schahen opened this issue Jan 21, 2021 · 9 comments

Comments

@Schahen
Copy link

Schahen commented Jan 21, 2021

Correct me if I'm wrong, but from what I see there's no way to include grammar definitions from one file into another one.

My questions are following:

  1. Is it indeed the case?
  2. If yes, can we discuss introduction of such feature? I'm OK with participating if needed.
@pdubroy
Copy link
Contributor

pdubroy commented Jan 22, 2021

Hi @Schahen, are you referring to something like an import/include statement in the grammar itself?

E.g.,

include '../Arithmetic.ohm'

MyGrammar <: Arithmetic {
    ...
}

If so, then you are correct — it's not something we support today. But I'm definitely open to it if you have a proposal!

@pdubroy pdubroy changed the title Is there's a way to include grammar definitions from other file Grammar syntax for imports/includes May 24, 2021
@LiamRiddell
Copy link

@pdubroy Following up on this, I think this would be a really useful feature but I feel it could be done in the ohm-cli.

  1. Load the grammars from disk
  2. For each match grammar
    2a. Try and process imports
    2b. Attempt to load defined (if any) imports from disk using relative path.
    2c. If successful Substitute the contents of the loaded file in place of the import.
    2c. If error Throw FileNotFound for import error
    2e. Follow existing parsing flow.

With the above in mind, is ohm-js used to parse the ohm files when pre-building grammars using the cli? If you have a better approach, please let me know.

I'd be keen to potentially look at getting this working as it's exactly what I need.

@pdubroy
Copy link
Contributor

pdubroy commented Aug 16, 2023

As mentioned in #457, I think a complete implementation of this feature needs to address a few things.

  1. We should add the syntax to the canonical grammar for the Ohm language (ohm-grammar.ohm), rather than using a regex.
  2. Implement a way for this to work when instantiating a grammar via ohm.grammar. Otherwise, we would effectively be forking the language into one version that's supported by @ohm-js/cli, and another version that's supported in the API.
  3. Implement a solution for (3) that works in both Node and in browsers.
  4. We'd also need to make sure that when you generate TypeScript types for a grammar, it handles imports correctly.

Regarding (2), a simple approach might be something like the following:

ohm.grammar(`
  include './foo.ohm'

  G {
    ...
  }
`, {
  fetchGrammar(relativeUrl) {
    return fs.readFileSync(relativeUrl, import.meta.url, 'utf-8');
  }
})

The idea would be that when you call ohm.grammar, you can optionally pass an options object, and that object could contain a function that would be used to resolve imports. So, Ohm itself wouldn't explicitly resolve imports, but it would be easy enough to write a one-liner that handles imports in either Node or the browser.

Alternatively, we could include default implementations, but that would be a bit more complicated with regards to bundling, etc.

@LiamRiddell
Copy link

Implemented the above, awaiting review on the linked #457

@marco3479
Copy link

Any progress on this front? No way to generateBundles of a grammar that extends another. Or is there?

@LiamRiddell
Copy link

LiamRiddell commented Nov 8, 2023

Any progress on this front? No way to generateBundles of a grammar that extends another. Or is there?

@marco3479 You can extend grammars but they have to be defined in the same file. For me, this was too restrictive. I have since put together a proposal PR but it was awaiting review for a while and I've had to close it due to lack of time on my end. Hopefully it can be picked up again later.

What you can do is write your own pre-processing to automatically search and replace includes... I've seen others using tools for this. Otherwise, you can use my fork which is currently used by my project Solve.

@marco3479
Copy link

Noted. Thanks @LiamRiddell.

@pdubroy
Copy link
Contributor

pdubroy commented Nov 10, 2023

@marco3479 Can you share some details about your use case?

@LiamRiddell, I appreciate you submitting the PR and I apologize for letting it drop. I'd also like to hear a bit more about the specifics of your use case.

I would like to figure out how to best support this, but I had second thoughts about the solution I proposed. It complicates the API and language more than I'd like.

@marco3479
Copy link

@pdubroy, I have a two grammars; B extends A. I was trying to keep the grammar definitions in their own .ohm files. When trying to generate the TS bundle for B, reasonably, it wouldn't recognize A (as was defined elsewhere). Adding the import syntax should fix this.
Honestly, the only minor bother regarding this is that I can't keep the grammars isolated, rather I have to define in a single .ohm file, and if I make updates to any of the grammars, I have to generate TS bundles for both simultaneously. Unless I'm missing something, it's not a deal-breaker.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants