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

Merging Spans ♻️ #887

Merged
merged 3 commits into from Jul 16, 2023
Merged

Merging Spans ♻️ #887

merged 3 commits into from Jul 16, 2023

Conversation

0nyr
Copy link
Contributor

@0nyr 0nyr commented Jul 14, 2023

Hello Pest team!

I'm working on a Ctiny language interpreter for a university course. You can find my repository here | GitHub.

While working on the AST, I have faced a problem.

Considering the following grammar:

expression = { disjunction } // top level expression is disjunction
disjunction = { conjunction ~ (disjunction_operator ~ conjunction)* }
conjunction = { equality ~ (conjunction_operator ~ equality)* }
equality = { relation ~ (equality_operator ~ relation)* }
relation = { addition ~ (relation_operator ~ addition)? }
addition = { term ~ (addition_operator ~ term)* }
term = { factor ~ (multiplication_operator ~ factor)* }
factor = { unary_operator? ~ primary }

and my AST node structs:

// Base AST node
#[derive(Debug, PartialEq)]
pub struct Node<T> {
    pub sp: Span,   // contains information about the node's position (position of span) to be matched to string in the source code
    pub data: T,            // contains the data, wrapped into an inner type
}

// Expressions
#[derive(Debug, PartialEq)]
pub enum Expression {
    Literal(Literal), // direct value
    UnaryExpression(UnaryExpression),
    BinaryExpression(BinaryExpression),
    FunctionCall(FunctionCall),
    TypeCast(TypeCast),
    GetOrSetValue(GetOrSetValue),
}

While parsing the pairs of an expression, I want to be able to combine Spans to create Nodes of Expression::BinaryExpression for easier debugging later (giving the span of the Node to a pest::CustomError). But so far, there is no merge function for Span, and since there is no getter for the its input field... it would require me to pass everywhere the &str of the original input string... which is really annoying, especially considering I'm only manipulating pairs and spans that all have the reference I'm looking for. So the best approach to this is really to add a getter get_input and a merge function to Span.

You can have a look at this fork in action, specifically on span merging here. Especially at the following code:

let common_span = merge_spans_no_check!(
                &left_operation.sp,
                &right_operation.sp
            ).ok_or(make_ast_error_from_pair(
                $input_pair.clone(), 
                format!("🔴 Couldn't build a span from the left and right operations. 
                    str_len: {}, left_start: {}, left_end: {}, right_start: {}, right_end: {}",
                    $input_pair.as_str().len(),
                    left_operation.sp.start(),
                    left_operation.sp.end(),
                    right_operation.sp.start(),
                    right_operation.sp.end(),
                ).as_str())
            )?;

I'm not the only one facing this issue. A friend of mine, also working on this course is facing a similar issue.

That's what I have done with this PR. I also added meaningful documentation in comments as well as tests.

@0nyr 0nyr requested a review from a team as a code owner July 14, 2023 17:25
@0nyr 0nyr requested review from tomtau and removed request for a team July 14, 2023 17:25
Copy link
Contributor

@tomtau tomtau left a comment

Choose a reason for hiding this comment

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

looks good. Can you also run cargo fmt on that code?

pest/src/span.rs Outdated Show resolved Hide resolved
@0nyr 0nyr requested a review from tomtau July 15, 2023 17:01
@tomtau tomtau merged commit 597e09b into pest-parser:master Jul 16, 2023
9 checks passed
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.

None yet

2 participants