Skip to content

Conversation

@ratmice
Copy link
Collaborator

@ratmice ratmice commented Feb 27, 2025

Here is take two on my previous PR #491

Had to slightly rush to get this out the door, as there is a possibility I may not be able to work on this or respond for a few days but hopefully nothing has been overlooked.

I did slightly relax the documentation string for YaccKindResolver::Force that you had given, changing it from being an error if they differ, to an error unless they are compatible. This is primarily to allow for differing YaccOriginalActionKind behaviors when used with the Original kind.

I've added %grmtools sections to some of the files in the repo while testing, but certainly not all this should probably
done though.

--

This is a breaking change which adds a YaccKindResolver type. YaccKindResolver can allow a yacc grammar to be read from a %grmtools section of a yacc grammar source file.

CTParserBuilder which previously required a YaccKind, has been relaxed to allow the YaccKind to be unspecified in which case it will now attempt to find the yacckind in the %grmtools section.

nimbleparse previously would default to using YaccKind::Original, now attempts to read the yacc kind from the %grmtools section and otherwise throws an error. This should improve error messages and avoid defaulting to the wrong yacc kind.

@ltratt
Copy link
Member

ltratt commented Feb 28, 2025

This looks close to being ready to merge to me. Since it will imply a breaking release I have two "not exactly related, but worth noting here" thoughts:

  1. We should do a final 0.13 release now before merging this PR.
  2. If we're going to do a breaking release, what other breaking things do we want to consider?

@ratmice
Copy link
Collaborator Author

ratmice commented Feb 28, 2025

Regarding 2. I think I should at least work on the lrlex variation of %grmtools for #490 I'm not sure whether that will entail a breaking change or not though.

But everything else I can think of that has been on my radar has been non-breaking.

Edit: I'll keep it in mind though as I work on the lex stuff.

@ltratt
Copy link
Member

ltratt commented Feb 28, 2025

I think we're ready to squash? If so, please go ahead.

This is a breaking change which adds a `YaccKindResolver` type.
`YaccKindResolver` can allow a yacc grammar to be read from a
`%grmtools` section of a yacc grammar source file.

CTParserBuilder which previously required a YaccKind, has
been relaxed to allow the YaccKind to be unspecified in which case
it will now attempt to find the `yacckind` in the `%grmtools` section.

nimbleparse previously would default to using `YaccKind::Original`,
now attempts to read the yacc kind from the `%grmtools` section
and otherwise throws an error. This should improve error messages
and avoid defaulting to the wrong yacc kind.

Fixes softdevteam#349
@ratmice ratmice force-pushed the grmtools_directive_2 branch from c84c3f2 to 2ffcffb Compare February 28, 2025 15:00
@ratmice
Copy link
Collaborator Author

ratmice commented Feb 28, 2025

Squashed.

@ltratt ltratt added this pull request to the merge queue Feb 28, 2025
Merged via the queue into softdevteam:master with commit 2d554c2 Feb 28, 2025
2 checks passed
@ratmice ratmice deleted the grmtools_directive_2 branch February 28, 2025 15:13
@ratmice
Copy link
Collaborator Author

ratmice commented Feb 28, 2025

Edit: After thinking about it, I don't really think there is anything here that can't be done in a totally compatible fashion.

So the first thing that came to mind (could potentially be some kind of breaking change, uncertain about that though).

Has to do with source code generation, currently we have like nimbleparse, and build.rs code typically uses CTParserBuilder. I would like to get to the point where nimbleparse_lsp can lint actions code, by generating sources and running clippy/etc on the generated source and translating.

It isn't really clear that CTParserBuilder is the right thing that an lsp should be running.
E.g. it writes directly to the file system, where a LSP should probably generate code to a user provided string.
I suppose we could just add an output_string(self: CTParserBuilder, &mut String) and that would be a compatible change?

Then we also have issues like #448 (This is only tangentially related to the above in that one could consider graphviz or railroad diagrams as output) and one could imagine some kind of YaccOutputKind type and everything that encompasses both source generation and diagram generation and the like, and have the things that operate directly on filesystem paths internally use the string based APIs.

enum YaccOutputKind {
    RailroadDiagramSvg,
    GraphvizDiagram,
    RustParserModuleSource,
}

I'm not certain I currently have any real specific/concrete API in mind, and I'm not sure it's necessary to do so in a breaking fashion, but there is this seed of an idea that both diagram and source generation have this generation/output step in common, so we could consider that sort of thing from first principles if you'd like.

Perhaps I should make a separate issue for this stuff?

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.

2 participants