Skip to content
This repository was archived by the owner on Jan 12, 2024. It is now read-only.

Conversation

@bettinaheim
Copy link
Contributor

@bettinaheim bettinaheim commented Feb 3, 2022

This PR contains the second batch of changes to improve the consistency of syntax tree transformations. The first half (included in this PR) can be found here: #1290 These changes are incorporated into this PR. Additionally, this PR changes the data structures used to represent lambda expressions, and adds the common handles for dealing with argument tuples.

@bettinaheim bettinaheim marked this pull request as ready for review February 3, 2022 07:56
@bettinaheim
Copy link
Contributor Author

The repo build is failing due to an unrelated security alert.

@bettinaheim bettinaheim changed the title Syntax tree transformation improvements - part 2 Syntax tree transformation improvements Feb 4, 2022
Copy link
Contributor

@ScottCarda-MS ScottCarda-MS left a comment

Choose a reason for hiding this comment

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

I still have a bit more looking into with the changes with the base transformations.

@cesarzc
Copy link
Contributor

cesarzc commented Feb 5, 2022

@bettinaheim after reviewing the PR, the summary of the major changes is the following:

  • Unchecked vs checked lambdas are now type differentiated.
  • CommonTransformationNodes was introduced as a way to better organize transformations that happen across multiple kinds of nodes.
    • By introducing this, overrides affect all transformation kinds that make use a specific method.

Am I missing something?

@bettinaheim
Copy link
Contributor Author

bettinaheim commented Feb 5, 2022

@bettinaheim after reviewing the PR, the summary of the major changes is the following:

  • Unchecked vs checked lambdas are now type differentiated.

  • CommonTransformationNodes was introduced as a way to better organize transformations that happen across multiple kinds of nodes.

    • By introducing this, overrides affect all transformation kinds that make use a specific method.

Am I missing something?

Regarding "Unchecked vs checked lambdas are now type differentiated.":
This was also the case without my changes. The reason for my changes to the lambda data structure specifically were primarity to align how parameter tuples are handled for lambdas with how they work for global declarations. This allows to define a common handler for both in the syntax tree transformation infrastructure. Additionally, this also automatically creates room for potentially allowing to annotate a parameter type in the future (e.g. (a : Int, b : Int) -> a + b).

Regarding CommonTransformationNodes:
Yes, exactly. This class is largely not publicly visible, such that the common nodes are overridded by adding an override to the SyntaxTreeTransformation itself rather than to a subclass.

This is the summary for the change:

CommonTransformationNodes was introduces as per your comment, and the nodes in that class are overridded by overriding them in the SyntaxTreeTransformation. The common nodes extracted in that way are the following:

  • The transformations now use OnLocalNameDeclaration and OnLocalName for all locally declared symbols. The distinction between the declaration of a local name and its use wasn't properly made previously.
  • The change also introduces OnItemNameDeclaration and OnItemName to easily modify named items in user defined types. Previously there was no handle that would have allowed to conveniently change e.g. the name of a named item in a user defined type.
  • The data structure used for parameter tuples of lambdas and globally declared callables is now the same, allowing for a common handler for dealing with argument tuples. A good future change would be to also align parameter tuples with symbol tuples in variable declarations (symbols declared as part of let-, mutable-, use- and borrow-statements, as well as in for loops). The PR is big enough as it is, and I decided against it for this PR.
  • All methods for handling positional information were moved into the CommonTransformationNodes. This includes a descriptive name for absolute locations vs relative locations, and adding a handler for symbol locations which didn't previously exist.

Copy link
Contributor

@ScottCarda-MS ScottCarda-MS left a comment

Choose a reason for hiding this comment

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

Just had one open concern about the changes to how subtransformations relate to each other and its effect on transformation modularity and flexibility. Outside of that, this looks great!

@ScottCarda-MS ScottCarda-MS mentioned this pull request Feb 9, 2022
12 tasks
@bettinaheim bettinaheim merged commit 28d9b34 into feature/lambda Feb 12, 2022
@bettinaheim bettinaheim deleted the beheim/ast branch February 12, 2022 01:26
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants