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.

Sign up for GitHub

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

refactor: Move grammar parser constructor to codegen_grammar #746

Closed
wants to merge 2 commits into from
Closed

refactor: Move grammar parser constructor to codegen_grammar #746

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
feca5ea
refactor: Move grammar parser constructor to codegen_grammar
Xanewok Jan 15, 2024
199bd95
Require the trivia parser to not be empty
Xanewok Jan 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions crates/codegen/grammar/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,5 +6,7 @@ edition.workspace = true
publish = false

[dependencies]
codegen_language_definition = { workspace = true }
indexmap = { workspace = true }
semver = { workspace = true }
strum_macros = { workspace = true }
9 changes: 5 additions & 4 deletions ...s/solidity/inputs/language/src/grammar.rs → crates/codegen/grammar/src/constructor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,16 @@ use std::collections::{BTreeMap, BTreeSet, HashMap};
use std::ops::Deref;
use std::rc::Rc;

use codegen_grammar::{
use codegen_language_definition::model::{self, FieldsErrorRecovery, Identifier, Item};
use indexmap::IndexMap;

use crate::{
Grammar, GrammarElement, KeywordScannerDefinition, KeywordScannerDefinitionNode,
KeywordScannerDefinitionVersionedNode, Named, ParserDefinition, ParserDefinitionNode,
PrecedenceOperatorModel, PrecedenceParserDefinition, PrecedenceParserDefinitionNode,
ScannerDefinition, ScannerDefinitionNode, TriviaParserDefinition, VersionQuality,
VersionQualityRange,
};
use codegen_language_definition::model::{self, FieldsErrorRecovery, Identifier, Item};
use indexmap::IndexMap;

/// Materializes the DSL v2 model ([`model::Language`]) into [`Grammar`].
pub trait GrammarConstructorDslV2 {
Expand Down Expand Up @@ -143,7 +144,7 @@ impl KeywordScannerDefinition for NamedKeywordScanner {
self.name
}

fn definitions(&self) -> &[codegen_grammar::KeywordScannerDefinitionVersionedNode] {
fn definitions(&self) -> &[KeywordScannerDefinitionVersionedNode] {
&self.defs
}

Expand Down
2 changes: 2 additions & 0 deletions crates/codegen/grammar/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
mod constructor;
mod grammar;
mod parser_definition;
mod precedence_parser_definition;
mod scanner_definition;
mod version_quality;
mod visitor;

pub use constructor::GrammarConstructorDslV2;
pub use grammar::*;
pub use parser_definition::*;
pub use precedence_parser_definition::*;
Expand Down
1 change: 0 additions & 1 deletion crates/codegen/language/definition/src/model/terminals/trivia.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ pub enum TriviaParser {
Choice { parsers: Vec<TriviaParser> },

Optional { parser: Box<TriviaParser> },
// TODO(#638): Remove this, once we adapt the DSL v1 codegen model to the new v2 definition.
OneOrMore { parser: Box<TriviaParser> },
ZeroOrMore { parser: Box<TriviaParser> },

Expand Down
3 changes: 0 additions & 3 deletions crates/solidity/inputs/language/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,7 @@ infra_utils = { workspace = true }
[dependencies]
anyhow = { workspace = true }
bson = { workspace = true }
codegen_grammar = { workspace = true }
codegen_language_definition = { workspace = true }
codegen_language_macros = { workspace = true }
codegen_schema = { workspace = true }
indexmap = { workspace = true }
semver = { workspace = true }
strum_macros = { workspace = true }
2 changes: 0 additions & 2 deletions crates/solidity/inputs/language/src/definition.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@ pub use solidity::SolidityDefinition;
codegen_language_macros::compile!(Language(
name = Solidity,
root_item = SourceUnit,
// TODO(#638): For now this is on par with the DSL v1 definition to minimize the fallout.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We decided against virtual EndOfInput tokens, so not bringing that trivia parser back from the original DSLv2. Also, the trivia has to require something, i.e. we shouldn't create empty rules. Will be unnested anyway to sibling tokens with #737

The new definition was also wrong, as in, we need to find an ergomonic way to accept MultilineComment as part of the leading trivia but only if it doesn't have newline in it; I'd consider this a separate work item (new "feature") and it's not of urgent priority - we should probably stick to single-line comments and revisit this at some point.

Copy link
Contributor

@OmarTawfik OmarTawfik Jan 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Also, the trivia has to require something, i.e. we shouldn't create empty rules

Not sure I'm following. Since they are siblings, we should be using the original ZeroOrMore, as it should always be possible to have no leading trivia, right?
We will run that parser (that adds all its results to the "current" parent rule node), and possibly adding nothing.

-> Same concern for the removed TODO comment in model/terminals/trivia.rs

accept MultilineComment as part of the leading trivia but only if it doesn't have newline in it

Not sure I'm following either? newlines is legal/valid in leading trivia.

// We should replace this with the new definition from #629.
leading_trivia = OneOrMore(Choice([
Trivia(Whitespace),
Trivia(EndOfLine),
Expand Down
2 changes: 0 additions & 2 deletions crates/solidity/inputs/language/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,10 @@
//! Call the [`SolidityLanguageExtensions::load_solidity`] method to load the precompiled language definition.

mod definition;
mod grammar;

use anyhow::Result;
use codegen_schema::types::{LanguageDefinition, LanguageDefinitionRef};
pub use definition::SolidityDefinition;
pub use grammar::GrammarConstructorDslV2;

pub trait SolidityLanguageExtensions {
/// Loads the precompiled Solidity language definition.
Expand Down
4 changes: 2 additions & 2 deletions crates/solidity/outputs/cargo/crate/build.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,10 @@
//! It is removed when publishing to crates.io.

use anyhow::Result;
use codegen_grammar::Grammar;
use codegen_grammar::{Grammar, GrammarConstructorDslV2};
use codegen_parser_generator::{AstModel, RustGenerator};
use infra_utils::cargo::CargoWorkspace;
use solidity_language::{GrammarConstructorDslV2, SolidityDefinition};
use solidity_language::SolidityDefinition;

fn main() -> Result<()> {
OmarTawfik marked this conversation as resolved.
Show resolved Hide resolved
let language = SolidityDefinition::create();
Expand Down