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

Load files once #998

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

MegaIng
Copy link
Member

@MegaIng MegaIng commented Sep 14, 2021

The changes implemented during the discussion of #992, and a basic implementation of %include (typo in commit message :-/). Should I also implement more complex load once stuff in this PR?

@ornariece
Copy link
Contributor

@MegaIng say i have 2 grammars A and B that both include a grammar C. now if i include grammars A and B in a grammar D, i get conflict errors as rules defined in grammar C are declared twice. is that wanted?
in fact (not exclusive to %include), why prohibit defining a rule more than once, instead of using the last definition?

@MegaIng
Copy link
Member Author

MegaIng commented Sep 25, 2021

@ornariece Without a major rewrite of GrammarLoader, that is not avoidable. I will take %include out of this PR, and start on a rework. The rest of this PR works.

The logic is that you can manually specify %extend or %override which might both be correct behavior for a repeated definition.

@MegaIng
Copy link
Member Author

MegaIng commented Sep 25, 2021

@erezsh This PR is now done. I will put %include into a new one.

@ornariece
Copy link
Contributor

ornariece commented Sep 25, 2021

@MegaIng you added %include back

@MegaIng
Copy link
Member Author

MegaIng commented Sep 25, 2021

HOW? I completely reverted those changes (they aren't even in my local files). I am not sure where git got those changes from...

@ornariece
Copy link
Contributor

@MegaIng (currently tweaking my grammars to use %include)
%extend can't be used on a rule that got defined through an %include statement (ie. not through direct definition or %import).
the error that is raised is lark.exceptions.GrammarError: Can't extend rule name as it wasn't defined before

@MegaIng
Copy link
Member Author

MegaIng commented Sep 27, 2021

@ornariece That is part of the reason why I removed this implementation of %include.

@erezsh
Copy link
Member

erezsh commented Sep 27, 2021

@MegaIng I'm probably missing something, but why not just make it use the same implementation of %import but just with an empty namespace?

@MegaIng
Copy link
Member Author

MegaIng commented Sep 27, 2021

@erezsh That leads exactly to the problem @ornariece describes, since imports (and then includes) are added before anything in the grammar is done, meaning that an included grammar could not extend symbols in the main grammar. Also, that would still create a full GrammarLoader instance for each included grammar, meaning that no actual import de duplication would happen.

@erezsh
Copy link
Member

erezsh commented Sep 27, 2021

an included grammar could not extend symbols in the main grammar

I think that's the desirable behavior.

Perhaps these symbols should be refactored into a shared.lark, from which they can be extended.

@MegaIng
Copy link
Member Author

MegaIng commented Sep 27, 2021

I think that's the desirable behavior.

I disagree. Including a grammar should behave no different then just pasting the entire content at that place.

@erezsh
Copy link
Member

erezsh commented Sep 27, 2021

For that behavior, it has to be processed before all other directives.

@MegaIng
Copy link
Member Author

MegaIng commented Sep 27, 2021

@erezsh That is the plan, but that is interdependent of this PR. I will create one for those changes.

@erezsh
Copy link
Member

erezsh commented Oct 17, 2021

Sorry I'm only getting to this now. Any chance you can fix the PR for the recent changes?

It might even need a few more changes for allowing abnf and such? (maybe not. I didn't look that deeply yet)

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.

3 participants