Fix #696 now providing the correct amount of placeholders #922

ornariece · 2021-06-26T14:10:09Z

p = Lark("""!start: ["a" "b" "c"] """, maybe_placeholders=True)
p.parse("").children

now returns [None, None, None] instead of [None]

same for !start: ["a" ["b" "c"]].

MegaIng

That is a lot shorter of a PR than I expected. Good job!

MegaIng · 2021-06-26T14:12:34Z

lark/load_grammar.py

@@ -231,12 +231,11 @@ def will_not_get_removed(sym):
                return not sym.name.startswith('_')
            if isinstance(sym, Terminal):
                return keep_all_tokens or not sym.filter_out
-            assert False
+            if isinstance(sym, Symbol):


This should not be needed. There are only two Symbols subclasses: NonTerminal and Terminal.

Right now _EMPTY is a Symbol, and isn't a terminal or nonterminal.

But it probably makes more sense to test is _EMPTY.

yea testing for is _EMPTY would make more sense. i went with isinstance(sym, Symbol) because Symbol is only ever called directly when defining _EMPTY, but it's less clear and less precise than testing for is _EMPTY.

MegaIng · 2021-06-26T14:16:17Z

lark/load_grammar.py

@@ -231,12 +231,11 @@ def will_not_get_removed(sym):
                return not sym.name.startswith('_')


We might want to think about changing this behavior. But I am not sure if there is a better solution...

erezsh · 2021-06-27T19:50:56Z

@MegaIng I'm surprised you aren't concerned with breaking backwards-compatibility. I think I'll have to add a new value for this behavior. Maybe maybe_placeholders='per_symbol'. Open to suggestions.

MegaIng · 2021-06-27T20:05:13Z

@erezsh True. I didn't consider it. This is obviously breaking. We can add a new option, or push it back till 1.0 . (on the other hand, I find the current system so confusing, we might as well break in the next 0.x.1 release.)

ThatXliner · 2021-06-28T17:39:35Z

@MegaIng I'm surprised you aren't concerned with breaking backwards-compatibility. I think I'll have to add a new value for this behavior. Maybe maybe_placeholders='per_symbol'. Open to suggestions.

Yes, definitely. I think ["thing" "thing" "thing"] should return [None]. It makes the most sense to me and I have code that needs this behavior.

erezsh · 2021-06-28T18:23:55Z

It makes the most sense to me

@ThatXliner Can you explain your reasoning?

The issue is that a match and a mismatch may provide a different amount of arguments.

ThatXliner · 2021-06-28T18:34:48Z

It makes the most sense to me

@ThatXliner Can you explain your reasoning?

Well the docs said that [...] should act like (...)? but return None on no match, right?

The issue is that a match and a mismatch may provide a different amount of arguments.

Yeah I saw. You originally thought of it being the right behavior, right?

The point is, I agree that we should change it to maybe_placeholders="per_symbol" or something to keep backwards incompatibility.

MegaIng · 2021-06-28T18:37:21Z

@ThatXliner Our current plan is to add this change with a few other in a very breaking version 1.0 .

ThatXliner · 2021-06-28T18:37:39Z

On the issue of changing behavior, you might want to replace the maybe_placeholders argument to something different entirely for 1.x. Maybe change the name to placeholder_behavior and the accepted values to an enum.

ThatXliner · 2021-06-28T18:37:46Z

@ThatXliner Our current plan is to add this change with a few other in a very breaking version 1.0 .

Oh ok. In that case, you might want to update the docs, as well.

erezsh · 2021-06-28T18:40:59Z

On the issue of changing behavior, you might want to replace the maybe_placeholders argument to something different entirely for 1.x. Maybe change the name to placeholder_behavior and the accepted values to an enum.

That's a great idea! thanks for the suggestion.

erezsh · 2021-06-28T18:41:20Z

P.S. the question is, should we even support the old behavior in 1.x, or just keep the new one?

ThatXliner · 2021-06-28T18:51:39Z

Ok, so I here's an example of using the old behavior for conciseness: https://github.com/ThatXliner/tylaireum/blob/4ae6772a0660cb75c307edc7d2bdff94667b8cdc/tylaireum/grammar.lark#L59

If I wanted to be compatible with the new behavior, I'd need to make another rule. (the ...s rules like fdec_args vs fdec_arg is to please ast_utils. Not relevant)

erezsh · 2021-06-28T19:06:50Z

@ThatXliner Maybe you misunderstood. [IDENTIFIER "="] will still be one argument when matched, because "=" gets discarded.

ThatXliner · 2021-06-28T19:11:19Z

@ThatXliner Maybe you misunderstood. [IDENTIFIER "="] will still be one argument when matched, because "=" gets discarded.

Oh right. Do terminals starting with _ get discarded, too? If so, then I think changing the behavior entirely would be a plus. (I would need to modify my grammar a bit but I can't think of a situation where the new behavior would be a downside)

ornariece · 2021-09-03T14:26:50Z

i've encountered a usecase where my fix doesn't suffice.

p = Lark("""!start: ["a"] | ["b"] ["c"] """, maybe_placeholders=True)
p.parse("").children

should return [None, None]. currently it returns [None], which is wrong (following the same logic as before).

i've taken a look at what's causing this, and from what i can tell this looks like the fix is gonna be much more complex than my previous one. i haven't had the time to dive into it, and it's likely i'm gonna have some trouble with it.
help much appreciated :)

MegaIng · 2021-09-03T14:52:15Z

should return [None, None].

I disagree. While 2 children is the maximal account that can occur, it can also return just one (just "a"), so it is ambiguous. That is just a badly designed rule. (there are a total of 3 ways how the empty string could be parse).

This is also especially none-bad since a simple rewrite of the rule to ["a" | ["b"] ["c"]] produces the 'correct' (according to you) result. And even that rule can be made cleaner by dropping the outer []: "a" | ["b"] ["c"] is also equivalent and produces the same "correct" result.

ornariece · 2021-09-03T19:16:24Z

(i edited my previous comment, the outer [] was an unwanted leftover from a copy-paste)
i somehow missed the obvious, you're right "a" | ["b"] ["c"] is equivalent and the proper way to declare this grammar.
that said, i'd still argue ["a"] | ["b"] ["c"] should output 2 children in any case. but as i said, the "fix" is probably too complex for a gain that is negligible since there is a better and functional way to write such a grammar.

erezsh · 2021-09-03T19:22:46Z

@ornariece I'm inclined to rebase this PR over v1.0 instead, and then merge it. Any objections?

If you want it to get to the current master, we can try the maybe_placeholders='per_symbol' route. But I don't think it will be much longer before v1.0 is in master anyway.

ornariece · 2021-09-03T19:29:29Z

@ornariece I'm inclined to rebase this PR over v1.0 instead, and then merge it. Any objections?

fine by me!

ornariece · 2021-09-04T11:15:37Z

another example where the current fix doesn't suffice, this time the grammar should be minimal:
"a" ["b"] | ["a"] "b" "c" should output three children in any case, but it outputs two children when a or ab are parsed.

one workaround would be to add redundancy to the rule:
"a" ["b" ["c"]] | ["a"] "b" "c"
but that is suboptimal.
now is working on a fix worth it? i'm not sure

erezsh · 2021-09-04T12:13:07Z

@ornariece I would say that's the intended behavior.

In a rule like some_rule: a b | a b c, the result can be either two children or three. And that's fine, it's the responsibility of the grammar author to make sure they are balanced, if they so wish.

Perhaps one could argue that this balancing should happen automatically by adding Nones or some other way. But that's a whole separate issue in Lark, and not related to this specific fix.

ornariece · 2021-09-04T14:25:06Z

hmmm true, i didn't think of it as a separate behavior.

one could argue some_rule: a b | a b c should output 3 children in any case (by providing a None) when maybe_placeholders=True, but you're right, let's stick to this specific fix here

erezsh · 2021-09-04T14:50:59Z

one could argue some_rule: a b | a b c should output 3 children in any case

The feel free to make the argument, because I think it's not too hard to implement.

ornariece · 2021-09-05T12:54:55Z

well the thing is you made it so that only items enclosed in [] are to generate Nones if there is no match.
i'm not sure why you went with that restriction, but by that logic a b | a b c which is equivalent to a b c? shouldn't generate a None either.
so really the argument is about whether or not maybe_placeholders=True's role should be to generate a fixed amount of children in any case, or not

MegaIng · 2021-09-05T12:57:05Z

If you want a b | a b c to produce a, b, None as children, make that explicit. We can provided a user facing ability to add padding, but I would not make any assumptions by default, even when maybe_placeholders=True, or even if we added another option. What about a c | a b c?

ornariece · 2021-09-05T13:01:23Z

the only assumption i'm expecting is "if the user set maybe_placeholders to True, then he wants a fixed amount of children, so let's take the maximum amount, of children there can be, and fill the list of matched items with Nones until we reach that amount".
meaning a c | a b c would produce a, c, None or a, b, c, and i would say that's fine, since it's the user's responsability to care for the order in which he wants the children to appear in the transformer method

ornariece · 2021-09-05T13:02:52Z

ok no this is not the only assumption, since you'd still want a [b] c to produce a, None, c and not a, c, None

MegaIng · 2021-09-05T13:05:04Z

If is the user responsibility, make them express it in the grammar. Also, what about a+ | b c d e? We can warn about mismatching number of children (Maybe even error with some kind of strict option); and we an provided a padding symbol (Maybe _?) that will produce a None, but I wouldn't make assumptions about how many children they want. Also, what about a b? | c? We currently have ? to explicitly not provided None.

ornariece · 2021-09-05T13:13:45Z

If is the user responsibility, make them express it in the grammar

i agree with that

what about a+ | b c d e?

good point. the same issue arises for [a+] though, which currently would produce a single None i think?

erezsh · 2021-09-13T08:27:50Z

Thanks, and sorry it took so long!

ornariece · 2021-09-16T20:57:06Z

@erezsh @MegaIng currently ["a" _some_rule] would output a single None. i'd be inclined to modify that behavior so that it outputs 2 Nones

MegaIng · 2021-09-16T21:04:04Z

@ornariece No. What if _some_rule outputs more content? We can maybe add a check so that whatever _some_rule outputs get's added (If no recursion is detected)

ornariece · 2021-09-16T21:09:19Z

What if _some_rule outputs more content?

well same issue as with "a"+, which currently outputs a single None
but yea using the output is probably better, although maybe a bit confusing for the user?

Fix lark-parser#696 now providing the correct amount of placeholders

c3fc226

MegaIng approved these changes Jun 26, 2021

View reviewed changes

Fix lark-parser#696 testing for is _EMPTY

c24d6ec

MegaIng approved these changes Jun 27, 2021

View reviewed changes

ornariece added 2 commits July 18, 2021 17:12

Merge branch 'lark-parser:master' into master

99ced1f

Merge branch 'lark-parser:master' into master

31509ac

erezsh merged commit a12effe into lark-parser:master Sep 13, 2021

erezsh mentioned this pull request Jan 15, 2022

Bug in handling of maybe_placeholders #1078

Closed

		@@ -231,12 +231,11 @@ def will_not_get_removed(sym):
		return not sym.name.startswith('_')

Fix #696 now providing the correct amount of placeholders #922

Fix #696 now providing the correct amount of placeholders #922

Conversation

ornariece commented Jun 26, 2021

MegaIng left a comment • edited Loading

Choose a reason for hiding this comment

MegaIng Jun 26, 2021

Choose a reason for hiding this comment

erezsh Jun 26, 2021

Choose a reason for hiding this comment

ornariece Jun 26, 2021

Choose a reason for hiding this comment

MegaIng Jun 26, 2021

Choose a reason for hiding this comment

erezsh commented Jun 27, 2021

MegaIng commented Jun 27, 2021

ThatXliner commented Jun 28, 2021

erezsh commented Jun 28, 2021

ThatXliner commented Jun 28, 2021

MegaIng commented Jun 28, 2021

ThatXliner commented Jun 28, 2021

ThatXliner commented Jun 28, 2021 • edited Loading

erezsh commented Jun 28, 2021

erezsh commented Jun 28, 2021

ThatXliner commented Jun 28, 2021

erezsh commented Jun 28, 2021

ThatXliner commented Jun 28, 2021 • edited Loading

ornariece commented Sep 3, 2021 • edited Loading

MegaIng commented Sep 3, 2021

ornariece commented Sep 3, 2021

erezsh commented Sep 3, 2021

ornariece commented Sep 3, 2021

ornariece commented Sep 4, 2021

erezsh commented Sep 4, 2021

ornariece commented Sep 4, 2021

erezsh commented Sep 4, 2021

ornariece commented Sep 5, 2021

MegaIng commented Sep 5, 2021 • edited Loading

ornariece commented Sep 5, 2021

ornariece commented Sep 5, 2021 • edited Loading

MegaIng commented Sep 5, 2021

ornariece commented Sep 5, 2021

erezsh commented Sep 13, 2021

ornariece commented Sep 16, 2021

MegaIng commented Sep 16, 2021

ornariece commented Sep 16, 2021

MegaIng left a comment •

edited

Loading

ThatXliner commented Jun 28, 2021 •

edited

Loading

ThatXliner commented Jun 28, 2021 •

edited

Loading

ornariece commented Sep 3, 2021 •

edited

Loading

MegaIng commented Sep 5, 2021 •

edited

Loading

ornariece commented Sep 5, 2021 •

edited

Loading