Skip to content

Feature: AST transformers#19

Merged
alessiostalla merged 24 commits intomasterfrom
feature/ast-transformers
Jan 11, 2023
Merged

Feature: AST transformers#19
alessiostalla merged 24 commits intomasterfrom
feature/ast-transformers

Conversation

@alessiostalla
Copy link
Copy Markdown
Member

Ported from Kolasu, not yet as well-tested as in Kolasu (we're missing some infrastructure)

Comment thread pylasu/model/errors.py


@dataclass
class GenericErrorNode(Node, ErrorNode):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What is the difference between ErrorNode and GenericErrorNode?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

In Kolasu, ErrorNode is an interface and GenericErrorNode a concrete class. I ported the same hierarchy even though in Python we could just use ErrorNode.

Comment thread pylasu/model/model.py
return isinstance(decl_type, type) and issubclass(decl_type, Node)


class Concept(ABCMeta):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

With LionWeb we may want to introduce Concept with a close but perhaps different meaning

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we have a corresponding Concept in Kolasu?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

For now Concept does not exist in Kolasu

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

No, at least not explicitly. In Kolasu the concept is implicitly defined by extension methods using reflection. We could have done the same in Pylasu, but the IDE doesn't give any support for Python extension methods, so it's better to be more explicit if possible.

Comment thread pylasu/testing/testing.py
from pylasu.model import Node


def assert_asts_are_equal(expected: Node, actual: Node, context: str = "<root>", consider_position: bool = False):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Was this left by mistake?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

No, this is a work in progress, I opened the PR just to have a high-level view of the changes



@dataclass
class GenericNode(Node):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In an experiment I am working on (based on Kolasu), I add a sort of annotation on "normal" nodes to mark them as placeholder for something we do not know how to translate yet. In this way I can create a node of the type that fit the hierarchy. In Python this may be less of an issue, given it is dynamic.

Perhaps this generic node could contain a reference to the thing that we do not know how to translate. That could be useful for debugging

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Indeed the GenericNode has its origin set to the transformation source, this is ported exactly from Kolasu

Copy link
Copy Markdown
Member

@ftomassetti ftomassetti left a comment

Choose a reason for hiding this comment

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

I just added some minor comments, as I am not a Python expert

@alessiostalla alessiostalla requested a review from loradd January 9, 2023 13:32
@alessiostalla
Copy link
Copy Markdown
Member Author

This is now good to go I think, I've ported all the necessary stuff from Kolasu.

flake8 tests --config tests/.flake8 --count --show-source --statistics
- name: Generate test parser
run: java -cp ../antlr-4.10.1-complete.jar org.antlr.v4.Tool -Dlanguage=Python3 -visitor -o simple_lang SimpleLangLexer.g4 SimpleLangParser.g4
- name: Generate test parsers
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would it be useful to put these into a script, so that it can be used also when running tests locally?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, it makes sense

Copy link
Copy Markdown
Contributor

@loradd loradd 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 to me!

def test_identitiy_transformer(self):
prop = PropertyRef("statements")
transformer = ASTTransformer()
transformer.register_node_factory(CU, CU).with_child(prop, prop)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this different from an IdentityTransformer?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes because it has a child. This is ported as-is from Kolasu.

DisplayIntStatement(value=456)])
transformed_cu = transformer.transform(cu)
self.assertEqual(cu, transformed_cu)
# assertTrue { transformed_cu.hasValidParents() }
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We may want to uncomment or remove

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I've opened a separate issue to port hasValidParents noting that this test will have to be enabled.

@ftomassetti
Copy link
Copy Markdown
Member

+1

@alessiostalla alessiostalla merged commit 32e84e2 into master Jan 11, 2023
@alessiostalla alessiostalla deleted the feature/ast-transformers branch January 11, 2023 09:01
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