Play with explicit AST #20

mkmik · 2020-10-06T16:14:28Z

This is just a draft to see whether walking the pest parse tree becomes easier to reason about if we implement the interpreter around an abstract syntax tree instead of using a chain of selectors.

Probably it could become even simpler if I tweaked the grammar a bit, but I resisted doing that in the first pass.

One of the most interesting aspects of this approach is that the pest parse tree could be, in principle, mechanically mapped on on an abstract tree, using tools such as pest_ast+pest_derive

glyn · 2020-10-06T16:40:43Z

I quite like having an AST, but since this AST represents a strict superset of valid selectors, it adds a bit of complexity as I worry about whether an AST could be produced which doesn't corresponding to a valid selector. I guess that issue is implicit in the current implementation, but at least it doesn't stand out so much.

mkmik · 2020-10-07T08:11:32Z

I quite like having an AST, but since this AST represents a strict superset of valid selectors, it adds a bit of complexity as I worry about whether an AST could be produced which doesn't corresponding to a valid selector. I guess that issue is implicit in the current implementation, but at least it doesn't stand out so much.

ideally a good AST should make invalid selectors unrepresentable; just to be clear, the case I can see is a "rootless" selector.
are there other things you can see?

mkmik · 2020-10-07T08:18:17Z

hmm, no actually the Root path enum variant must be present otherwise the type recursion doesn't terminate;
so I guess don't understand what you meant :-)

mkmik · 2020-10-07T08:47:49Z

is it because the this AST allows: Selector::Dot(Index::Number(0)) ?

glyn · 2020-10-07T08:50:57Z

is it because the this AST allows: Selector::Dot(Index::Number(0)) ?

Yes, precisely.

glyn

Looking good. Just a couple of nits on the tree diagrams.

src/ast.rs

mkmik · 2020-10-07T10:14:36Z

ok, so now I fixed the AST data model so that .4 is not representable;

I'd like to see how hard it would be to also make [1,"foo"] unrepresentable

glyn · 2020-10-07T10:34:13Z

I'd like to see how hard it would be to also make [1,"foo"] unrepresentable

That selector is valid and should be representable. Try the following tests:

  }, {
    "name": "mixed union, applied to array",
    "selector": "$[1,\"foo\"]",
    "document": ["first", "second"],
    "result": ["second"]
  }, {
    "name": "mixed union, applied to object",
    "selector": "$[1,\"foo\"]",
    "document": {"foo" : "A", "b" : "B"},
    "result": ["A"]

glyn · 2020-10-07T10:37:18Z

src/ast.rs

+///            ^              \
+///           / \              [Number(1), Number(2)]
+///          /   \
+/// Root ___/     \___ DotName("foo")


The two diagrams still seem inconsistent. I'd expect the part of the tree nearest the root to be identical in both cases since $.foo is a common prefix of both selectors.

That's because the expressions have right to left precedence

Still don't get it. $.foo.bar produces the AST:

Sel(Sel(Root, DotName("foo")), DotName("bar"))

and $.foo[1,2]["bar"] produces:

Sel(Sel(Sel(Root, DotName("foo")), Union([Number(1), Number(2)])), Union([Field("bar")]))

Root appears inside Sel(Root, DotName("foo")) in both cases. Shouldn't this subtree look the same in both diagrams?

Uh :brainfart:; fixed

glyn · 2020-10-07T10:42:18Z

src/ast.rs

+                Value::Object(m) => Box::new(m.values()),
+                Value::Array(a) => Box::new(a.iter()),
+                _ => Box::new(std::iter::empty()),
+            },


I really like the readability of this approach. (It's a pity Box::new can't be factored out of the outer match, but that's rust for you.)

Hopefully soon rust will have the box keyword which at least removes the need for extra parentheses

mkmik · 2020-10-07T10:50:48Z

That selector is valid and should be representable. Try the following tests:

Ah yes, you're right

src/ast.rs

glyn · 2020-10-08T08:01:58Z

src/ast.rs

+/// ```
+///
+/// Selectors are left associative, thus `$.foo[1,2]["bar"]` behaves
+/// like (pseudocode) `(($.foo)[1,2])["bar"]`; thus the root of the resulting


Is it worth pointing out that the Root selector ($) is not related to the root of the AST? Might confuse people otherwise.

src/ast.rs

src/grammar.pest

glyn

A few naming issues. What else do you have in mind for this PR before it is ready for merging?

mkmik · 2020-10-08T09:56:48Z

A few naming issues. What else do you have in mind for this PR before it is ready for merging?

nothing that can't be added in smaller subsequent PRs

glyn

Looks great. Thanks!

mkmik requested a review from glyn October 6, 2020 16:14

mkmik force-pushed the ast branch from 0bb7b00 to 281f4d2 Compare October 6, 2020 16:15

mkmik force-pushed the ast branch 2 times, most recently from 222cc12 to 1fbd1e0 Compare October 7, 2020 08:17

mkmik force-pushed the ast branch 2 times, most recently from df1f6c6 to 1641fd5 Compare October 7, 2020 09:19

Play with explicit AST

f5514c9

mkmik force-pushed the ast branch from 1641fd5 to f5514c9 Compare October 7, 2020 09:19

Marko Mikulicic added 2 commits October 7, 2020 11:46

Use new AST base parser by default

2ad8241

Simplify parser

4491566

glyn reviewed Oct 7, 2020

View reviewed changes

src/ast.rs Outdated Show resolved Hide resolved

src/ast.rs Outdated Show resolved Hide resolved

Improve AST diagram

9503b4a

Fix fmt

eca69d8

glyn reviewed Oct 7, 2020

View reviewed changes

mkmik force-pushed the ast branch from 2c878fb to d9c79d2 Compare October 7, 2020 11:11

glyn reviewed Oct 7, 2020

View reviewed changes

src/ast.rs Outdated Show resolved Hide resolved

mkmik force-pushed the ast branch 2 times, most recently from 1031ec9 to d35dad7 Compare October 7, 2020 21:10

Fix diagram

644ddc9

mkmik force-pushed the ast branch from d35dad7 to 644ddc9 Compare October 7, 2020 21:10

glyn reviewed Oct 8, 2020

View reviewed changes

src/ast.rs Outdated Show resolved Hide resolved

glyn reviewed Oct 8, 2020

View reviewed changes

src/ast.rs Outdated Show resolved Hide resolved

glyn reviewed Oct 8, 2020

View reviewed changes

src/grammar.pest Outdated Show resolved Hide resolved

glyn suggested changes Oct 8, 2020

View reviewed changes

Fix nomenclature

2ec6147

glyn approved these changes Oct 8, 2020

View reviewed changes

glyn merged commit 5f537a2 into jsonpath-standard:main Oct 8, 2020

Play with explicit AST #20

Play with explicit AST #20

Uh oh!

Conversation

mkmik commented Oct 6, 2020

Uh oh!

glyn commented Oct 6, 2020

Uh oh!

mkmik commented Oct 7, 2020

Uh oh!

mkmik commented Oct 7, 2020

Uh oh!

mkmik commented Oct 7, 2020

Uh oh!

glyn commented Oct 7, 2020

Uh oh!

glyn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mkmik commented Oct 7, 2020

Uh oh!

glyn commented Oct 7, 2020

Uh oh!

glyn Oct 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkmik Oct 7, 2020

Choose a reason for hiding this comment

Uh oh!

glyn Oct 7, 2020

Choose a reason for hiding this comment

Uh oh!

mkmik Oct 7, 2020

Choose a reason for hiding this comment

Uh oh!

glyn Oct 7, 2020

Choose a reason for hiding this comment

Uh oh!

mkmik Oct 7, 2020

Choose a reason for hiding this comment

Uh oh!

mkmik commented Oct 7, 2020

Uh oh!

Uh oh!

glyn Oct 8, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

glyn left a comment

Choose a reason for hiding this comment

Uh oh!

mkmik commented Oct 8, 2020

Uh oh!

glyn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

glyn Oct 7, 2020 •

edited

Loading