Skip to content

Conversation

@simonwuelker
Copy link
Contributor

The current XPath AST is tightly tied to the parser grammar. To address issues like #39739, we'll want to do optimization passes on the AST in the future, which requires relaxing it's structure first.

This PR moves the AST structure into a separate module and flattens it into a shape where most expressions are variants of the core Expression enum.

This also has the nice side effect of being a net reduction in LoC and making the AST significantly easier to read.

I think the difference between the old and new structure is best explained by example.
One of our unit tests parses concat('hello', ' ', 'world'), and the current AST looks like this:

Expr::Path(PathExpr {
    is_absolute: false,
    is_descendant: false,
    steps: vec![StepExpr::Filter(FilterExpr {
        primary: PrimaryExpr::Function(CoreFunction::Concat(vec![
            Expr::Path(PathExpr {
                is_absolute: false,
                is_descendant: false,
                steps: vec![StepExpr::Filter(FilterExpr {
                    primary: PrimaryExpr::Literal(Literal::String(
                        "hello".to_string(),
                    )),
                    predicates: PredicateListExpr { predicates: vec![] },
                })],
            }),
            Expr::Path(PathExpr {
                is_absolute: false,
                is_descendant: false,
                steps: vec![StepExpr::Filter(FilterExpr {
                    primary: PrimaryExpr::Literal(Literal::String(" ".to_string())),
                    predicates: PredicateListExpr { predicates: vec![] },
                })],
            }),
            Expr::Path(PathExpr {
                is_absolute: false,
                is_descendant: false,
                steps: vec![StepExpr::Filter(FilterExpr {
                    primary: PrimaryExpr::Literal(Literal::String(
                        "world".to_string(),
                    )),
                    predicates: PredicateListExpr { predicates: vec![] },
                })],
            }),
        ])),
        predicates: PredicateListExpr { predicates: vec![] },
    })],
}),

After this change, the AST looks like this:

Expression::Function(CoreFunction::Concat(vec![
    Expression::Literal(Literal::String("hello".to_string())),
    Expression::Literal(Literal::String(" ".to_string())),
    Expression::Literal(Literal::String("world".to_string())),
])),

Testing: No behaviour change intended, covered by existing tests.
Part of #34527

Signed-off-by: Simon Wülker <simon.wuelker@arcor.de>
Signed-off-by: Simon Wülker <simon.wuelker@arcor.de>
@servo-highfive servo-highfive added the S-awaiting-review There is new code that needs to be reviewed. label Oct 12, 2025
context: &EvaluationCtx<D>,
) -> Result<Value<D::Node>, Error<D::JsError>> {
// Use starting_node for absolute/descendant paths, context_node otherwise
let mut current_nodes = if self.is_absolute || self.is_descendant {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is valid because self.is_descendant is never true unless self.is_absolute is also true.

@servo-highfive servo-highfive removed the S-awaiting-review There is new code that needs to be reviewed. label Oct 12, 2025
@jdm jdm added this pull request to the merge queue Oct 12, 2025
@servo-highfive servo-highfive added the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Oct 12, 2025
Merged via the queue into servo:main with commit e1c418d Oct 12, 2025
28 checks passed
@servo-highfive servo-highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Oct 12, 2025
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