Generic tree typing #967

plannigan · 2021-08-20T16:08:45Z

Updates the type information for Tree so that it is generic based on the type of the child leaf.
Adjusts the various classes that operate on trees to also understand the type of trees they can operate on.
Add ParseTree alias so that it is easy to refer to tree produced by the lark parser.

The changes don't introduce any new mypy errors, but there is one case where more type information demonstrated an existing inconsistency.

Addresses #966

MegaIng

Very nice :-)

plannigan · 2021-08-20T16:12:55Z

lark/tree.py

@@ -109,17 +112,18 @@ def iter_subtrees(self) -> 'Iterator[Tree]':
        subtrees = OrderedDict()
        for subtree in queue:
            subtrees[id(subtree)] = subtree
-            queue += [c for c in reversed(subtree.children)
+            # Reason for type ignore https://github.com/python/mypy/issues/10999


With the current type system it isn't possible to convey that "_Leaf_T can be anything except a Tree". So this will likely always be a thing mypy won't be able to figure out on its own. The referenced issue has more context and might provide some insights on how to better handle it.

lark/visitors.py

erezsh

Thanks for the creative PR!

I have a few questions, since I'm not well versed in Python's type annotations.

I'm also, in general, a little concerned about the added complication. I can't figure out if it's correct just from a cursory glance. But perhaps that would improve as I get used to it.

lark/tree.py

lark/visitors.py

erezsh · 2021-08-21T17:18:47Z

lark/lark.py

@@ -591,7 +592,7 @@ def parse_interactive(self, text: Optional[str]=None, start: Optional[str]=None)
        """
        return self.parser.parse_interactive(text, start=start)

-    def parse(self, text: str, start: Optional[str]=None, on_error: 'Optional[Callable[[UnexpectedInput], bool]]'=None) -> Tree:
+    def parse(self, text: str, start: Optional[str]=None, on_error: 'Optional[Callable[[UnexpectedInput], bool]]'=None) -> 'ParseTree':


If you change this to 'Tree[Token]', there is no need for a ParseTree variable.

Are you talking about it not being needed in this instance or not being needed in general?

For this specific use case, yes. That would not require the added import of ParseTree when type checking.

In general, ParseTree isn't strictly required at all in general. However, I think it is a nice ease of use to a have a specific name for trees returned by parse() since it is a core type that users of the library will be interacting with a lot.

With this mypy will raise an error is the code attemps to use __mul__ when the left hand side does not produce a Tree

lark/visitors.py

…operation

plannigan · 2021-08-27T15:45:34Z

lark/visitors.py


-    def __init__(self, *transformers: Transformer[_T]) -> None:
+    def __init__(self, *transformers: 'Union[Transformer, TransformerChain]') -> None:


With the initialize taking a N transformers, it is not possible to use type annotations to ensure that the T -> U -> V sequence is correct. This could be addressed by changing the class to only ever be aware of 2 Transformers. That is the only way it is called in the library, but users could be building chains directly.

Since there is already a lot going on in this PR. I'll hold off from also making any changes to the number of arguments accepted by this function.

erezsh · 2021-12-14T14:36:43Z

Sorry it took me so long to review this. Overall it looks fine.

A few notes:

The phrase Union[_Leaf_T, Tree[_Leaf_T]] repeats itself quite a lot, making it hard to read. Why not set

Branch = Union[_Leaf_T, 'Tree[_Leaf_T]']

and use that throughout the code? (preliminary tests show that it should work).

erezsh

Additional notes

lark/visitors.py

erezsh · 2021-12-14T14:44:12Z

lark/visitors.py

-    def __mul__(self, other: 'Transformer[_T]') -> 'TransformerChain[_T]':
+    def __mul__(
+            self: 'Transformer[Tree[_Leaf_U], _Leaf_T]',
+            other: 'Union[Transformer[_Return_V, _Leaf_U], TransformerChain[_Return_V, _Leaf_U]]'


Seems like everywhere that TransformerChain is used as a type, so is Transformer. So maybe it makes sense to just use AbsTransformer, or ITransformer instead.

This is true. Transformer and TransformerChain have the same public interface. Protocol could be used to define that without requiring a base class. (It would just require typing-extensions for less than Python 3.8, but that's not a big issue.) I can work on that next.

erezsh · 2021-12-14T14:46:11Z

lark/visitors.py

        while q:
            t = q.pop()
            rev_postfix.append(t)
            if isinstance(t, Tree):
                q += t.children

        # Postfix to tree
-        stack = []
+        stack: List = []


Why this addition?

Yeah, so this is a fun one:

There isn't enough type information based on the stack.append() calls for mypy to understand what should be allowed to be in the list. So it asks for the initialization of the variable to be annotated.

However, the list can contain anything based on the implementations of the user functions (See comment). So that means that the type is List[Any].

But the generic Any is redundant and can be removed.

So we are left we are left with an annotation that seems to be redundant to humans, but is there to explicitly tell mypy how the list is intended to be used.

Branch must be used as a generic so that mypy understands that _Leaf_T is the same type associated with the Tree

erezsh · 2021-12-20T21:42:32Z

lark/tree.py


-    def __init__(self, data: str, children: 'List[Union[_Leaf_T, Tree[_Leaf_T]]]', meta: Optional[Meta]=None) -> None:
+    def __init__(self, data: str, children: 'List[Branch[_Leaf_T]]', meta: Optional[Meta]=None) -> None:


Sorry if it's obvious, but why do we need to write Branch[_Leaf_T] and not just Branch, since it's already defined using _Leaf_T?

Not a problem, it is a subtle point. At the top level _Leaf_T stand for some type, but it can still be anything. Once a variable containing a tree is defined as Tree[Foo], then that tree's _Leaf_T is bound to Foo. Since Branch is defined at the top level, it can still be any type. Specifically using Branch[_Leaf_T] within the context of a Tree, means that the type has to match the bound _Leaf_T (in our example Foo).

erezsh · 2021-12-31T20:42:09Z

lark/visitors.py

+    from typing_extensions import Protocol
+
+_Return_T = TypeVar('_Return_T', covariant=True)
+_Return_V = TypeVar('_Return_V', covariant=True)


Quick question: Why is it Leaf_U but Return_V? Do the U and V stand for something?

The signature for the multiplication operator has a lot of moving pieces. It starts with a ITransformer[_Leaf_T, Tree[_Leaf_U]] instance (a transformer that converts a Tree[_Leaf_T] to Tree[_Leaf_U]). Then a ITransformer[_Leaf_U, _Return_V] instance (a transformer that converts a Tree[_Leaf_U] to _Return_V). Which results in a ITransformer[_Leaf_T, _Return_V] (a transformer that converts a Tree[_Leaf_T] to _Return_V).

There isn't a technical reason why it needs to be named with _Return_V instead of _Return_U. However, I was thinking that using _Return_V would make it clearer that the final type isn't necessarily related to the intermediary _Leaf_U. I'm open to switching to _Leaf_U or putting some of my above description into a comment in the source code if you think the current state should be changed.

chanicpanic

In #926 we chose not to add typing_extensions as a dependency. A workaround would be to put the import of Protocol and the definition of ITransformer within an if TYPE_CHECKING.

plannigan · 2022-01-02T13:24:14Z

Ok, I didn't have the context of that previous PR when I implemented this change. The one catch that makes this different is that ITransformer would only exist within lark and is exposed as part of the public API (argument to __mul__()). So if a user wanted to annotate a variable as ITransformer[Foo, Bar], they would have to do:

from typing import TYPE_CHECKING

if TYPE_CHECKING:
  from lark.visitors import ITransformer

class Foo:
    pass

class Bar:
    pass

my_tansformer: 'ITransformer[Foo, Bar]'

That being said, ITransformer is a protocol, so doing that is not a requirement.

This leaves us with two options:

Use the current Protocol implementation with a TYPE_CHECKING guard. And document that, while variables can be annotated with ITransformer using the example above, the recommendation is to not do that (relying on the functionality of Protocol).
Switch back to the previous Union implementation and avoid this potentially confusing corner case at the cost of slightly less readable type annotations in a few places.

I'm open to either option, but have a slight preference to using Union as it doesn't introduce the odd corner case.

erezsh · 2022-01-02T13:45:34Z

Is there a way to get the same effect without Protocol? Maybe with an ABC somehow?

Anyway, I agree, it doesn't seem worth it to add a dependency just to support ITransformer for 3.6 and 3.7

plannigan · 2022-01-02T14:00:28Z

ABC could also work, but would require a inheritance of the base class, something Transformer and TransformerChain don't currently have. I'll revert back to the Union implementation.

plannigan · 2022-01-03T17:34:32Z

Reminder from an earlier conversation:

I was about to add a new comment about that. I agree that it reads better at Transformer[_Leaf_T, _Return_T] . I initially added the leaf type at the end to minimize that changes. However, going from 1 to 2 generic parameters is a breaking change anyways, so moving leaf to be the first type passed in isn't a huge issue.

Note: These typing changes are only breaking changes for for code already taking advantage of type annotations and the generic types.

Since the PR didn't make it into the 1.0 release, that is something that should be kept in mind. If the choice is made to go the 2.0 route, I remember seeing a couple of places where minor API changes could resolve a few of the mypy errors that are currently reported on the library.

erezsh · 2022-01-03T17:49:00Z

If the choice is made to go the 2.0 route

You mean Lark 2.0? That's not going to happen for a while.

minor API changes could resolve a few of the mypy errors

Like what?

plannigan · 2022-01-04T00:08:27Z

I understand the desire to not have a 2.0 so shortly after 1.0 was released. But it is unfortunate that this PR did not get merged in time when breaking changes were less of a concern.

I think I should be able to resolve most, maybe all, of the existing smaller mypy issue with smaller API-compatible PRs., But two minor API changes I'd recommend:

1 - Have TransformerChain take only two Transformers in __init__() instead of N. This would allow for catching:

An attempt to chain a Transformer on after a Transformer that produces a non-Tree value
The first Transformer produces Tree[Foo], while the second Transformer expects a Tree[Bar].
Would disallow things like TransformerChain() or TransformerChain(my_transformer) as these don't provide much value.
This wouldn't break any code in the code base. However, it could cause break users that explicitly create TransformerChains instead of using multiplication operator AND passed in a less or more than 2 arguments.)

2 - Replace Tree.expand_kids_by_index() with a similar method that accepts a predicate (like find_pred() & scan_values()) instead of an arbitrary number of indexes. The current API expects the given indexes to correspond to Tree instances in the list of children. However, this allows for:

indexes that aren't Tree instances (AttributeError)
invalid indexes (IndexError)

The locations in the code base that use this method use it correctly by first doing a list comprehension with an if clause that ensures the index points to a Tree instance and satisfies the required criteria. Using a predicate instead would:

Ensure that non-Tree instances are skipped
Prevent indexing outside of the list.
Remove an extra iteration currently being done to produce the indexes to expand.
Simplify the calling code by only requiring a predicate of Callable[[Tree],bool].

A larger API change I'd recommend to be included in a theoretical future 2.0 would be to remove the use of custom double underscore names (ex. __default__()) with in the library as PEP 8 says:

__double_leading_and_trailing_underscore__: "magic" objects or attributes that live in user-controlled namespaces. E.g. __init__, __import__ or __file__. Never invent such names; only use them as documented.

erezsh · 2022-01-04T08:21:06Z

would be to remove the use of custom double underscore names (ex. default())

What would you name it instead? (the goal is to avoid name collision with any user-defined rule)

plannigan · 2022-01-04T13:16:53Z

Since "rules whose name begins with an underscore will be inlined into their containing rule", a parse tree won't have any with a single leading underscore. So _default() could work. _default_tree() would more closely match _default_token() and be less generic. It is a bit more verbose, but _tree_func_not_found() is more explicit.

I actually ran into an issue with a rule name collision with a Python keyword. I'll open a new issue for that to keep this discussion on topic, but the solution I had in mind would also help resolve the issue with rule names colliding with lark "internal" names.

erezsh · 2022-01-04T14:21:59Z

The main issue with something like _default() is that single underscores signify "private members" in Python, so I think many users will find it confusing, that it is accessed from the outside.

(and __ means private + protection, but it's rarely used in practice).

In contrast, magic_method syntax is the common way to facilitate interaction with the runtime, like eq, name, and iter. Also other libraries, like rich, use this for similar purposes: https://rich.readthedocs.io/en/stable/protocol.html

erezsh

Seems good enough for now.
Any improvements to it can be added in separate PRs.

erezsh · 2022-01-20T18:35:30Z

@MegaIng @chanicpanic Anything to add before I merge it?

erezsh · 2022-01-21T07:52:53Z

Thanks @plannigan ! And sorry for being such a stickler :)

Oops: Forgot it was rebased on top of 1.0b . I'll get it into master.

Edit2: Done.

plannigan added 2 commits August 20, 2021 11:56

Make Tree generic & update releated classes

b1ee70c

Fixes to allow for building a wheel

9d1dcd4

MegaIng approved these changes Aug 20, 2021

View reviewed changes

plannigan commented Aug 20, 2021

View reviewed changes

erezsh requested changes Aug 21, 2021

View reviewed changes

plannigan added 4 commits August 23, 2021 20:06

Improve type annotations related to TransformerChains

9c844c9

With this mypy will raise an error is the code attemps to use __mul__ when the left hand side does not produce a Tree

Clean up the internal annotations for Transformer_NonRecursive

862fb79

Clean up annotation for Interpreter

f6b39d9

Remove redundant generic typing of Any

f6e1abe

MegaIng approved these changes Aug 27, 2021

View reviewed changes

lark/visitors.py Outdated Show resolved Hide resolved

Allow for TransformerChain to be used on the right side of the chain …

8a8b70d

…operation

plannigan requested a review from erezsh August 27, 2021 15:15

MegaIng approved these changes Aug 27, 2021

View reviewed changes

plannigan commented Aug 27, 2021

View reviewed changes

erezsh marked this pull request as ready for review December 14, 2021 14:14

erezsh requested changes Dec 14, 2021

View reviewed changes

plannigan added 2 commits December 19, 2021 17:49

Make an alias for tree branches

acf2a26

Branch must be used as a generic so that mypy understands that _Leaf_T is the same type associated with the Tree

Reorder child type & return type

732143a

erezsh reviewed Dec 20, 2021

View reviewed changes

Use Protocol instead of Union of Transformer tpes

1646426

plannigan requested a review from erezsh December 31, 2021 16:26

erezsh reviewed Dec 31, 2021

View reviewed changes

chanicpanic reviewed Dec 31, 2021

View reviewed changes

Revert Protocol implmentation to avoid new dependency

c9c0140

plannigan requested a review from erezsh January 2, 2022 14:10

erezsh approved these changes Jan 20, 2022

View reviewed changes

chanicpanic approved these changes Jan 20, 2022

View reviewed changes

MegaIng approved these changes Jan 20, 2022

View reviewed changes

erezsh merged commit ee27eb4 into lark-parser:1.0b Jan 21, 2022

plannigan deleted the generic_tree_typing branch January 23, 2022 21:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generic tree typing #967

Generic tree typing #967

plannigan commented Aug 20, 2021

MegaIng left a comment

plannigan Aug 20, 2021

erezsh left a comment

erezsh Aug 21, 2021

plannigan Aug 22, 2021

plannigan Aug 27, 2021

erezsh commented Dec 14, 2021 •

edited

Loading

erezsh left a comment

erezsh Dec 14, 2021

plannigan Dec 20, 2021

erezsh Dec 14, 2021

plannigan Dec 20, 2021

erezsh Dec 20, 2021

plannigan Dec 20, 2021

erezsh Dec 31, 2021

plannigan Dec 31, 2021

chanicpanic left a comment

plannigan commented Jan 2, 2022

erezsh commented Jan 2, 2022

plannigan commented Jan 2, 2022

plannigan commented Jan 3, 2022

erezsh commented Jan 3, 2022

plannigan commented Jan 4, 2022

erezsh commented Jan 4, 2022

plannigan commented Jan 4, 2022

erezsh commented Jan 4, 2022

erezsh left a comment

erezsh commented Jan 20, 2022

erezsh commented Jan 21, 2022 •

edited

Loading


		def __init__(self, *transformers: Transformer[_T]) -> None:
		def __init__(self, *transformers: 'Union[Transformer, TransformerChain]') -> None:


		def __init__(self, data: str, children: 'List[Union[_Leaf_T, Tree[_Leaf_T]]]', meta: Optional[Meta]=None) -> None:
		def __init__(self, data: str, children: 'List[Branch[_Leaf_T]]', meta: Optional[Meta]=None) -> None:

Generic tree typing #967

Generic tree typing #967

Conversation

plannigan commented Aug 20, 2021

MegaIng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erezsh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erezsh commented Dec 14, 2021 • edited Loading

erezsh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chanicpanic left a comment

Choose a reason for hiding this comment

plannigan commented Jan 2, 2022

erezsh commented Jan 2, 2022

plannigan commented Jan 2, 2022

plannigan commented Jan 3, 2022

erezsh commented Jan 3, 2022

plannigan commented Jan 4, 2022

erezsh commented Jan 4, 2022

plannigan commented Jan 4, 2022

erezsh commented Jan 4, 2022

erezsh left a comment

Choose a reason for hiding this comment

erezsh commented Jan 20, 2022

erezsh commented Jan 21, 2022 • edited Loading

erezsh commented Dec 14, 2021 •

edited

Loading

erezsh commented Jan 21, 2022 •

edited

Loading