Skip to content

Conversation

@ratmice
Copy link
Collaborator

@ratmice ratmice commented May 26, 2025

Apologies in advance for the number of words I am likely to spill on this one line patch.

I seek to convince that we can reasonably add #[non_exhaustive] to struct GrammarAST,
the short explanation is that we have GrammarAST::new() and all the fields are pub so any usage of

GrammarAST{
  ...
}

Should be easily replaced by GrammarAST::new and field access.

The longer explanation I seek to explain that even GrammarAST::new() is likely not useful function, in the sense that we could consider making it pub crate. Then I'll argue against that for future expansion.

The key thing to note is that it does not appear there is anything tangible you can actually do anything with a let ast = GrammarAST::new().

The closest function we have to anything taking a fn (GrammarAST) -> YaccGrammar is YaccGrammar::new_from_ast_with_validity_info however that doesn't take a GrammarAST but a ASTWithValidityInfo, and there currently isn't any way to construct those expet from a &str either.

In this PR I didn't mark new as pub crate precisely because it seems like there might be a reasonable argument
that we could add an unsafe fn set_ast(&mut self, ast: GrammarAST) to ASTWithValidityInfo. This might be useful as an alternative to the hack discussed in #484

I.e. you could construct one GrammarAST from a source file, clone it, changing the start rule and using the unsafe function.

Lastly GrammarAST isn't Eq, or anything limiting the chances people might be constructing one manually and comparing it to another.

Edit: I should perhaps caveat this one with a: "unless I'm missing something completely obvious"

@ltratt
Copy link
Member

ltratt commented May 26, 2025

I tend to think this is a good idea.

@ltratt ltratt added this pull request to the merge queue May 26, 2025
Merged via the queue into softdevteam:master with commit 5f05d1d May 26, 2025
2 checks passed
@ratmice ratmice deleted the GrammarAST_non_exhaustive branch May 30, 2025 15:35
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