Update tree/pass model #5
Conversation
thinker227
added
enhancement
| /// <summary> | ||
| /// The pattern the transformation is triggered by. | ||
| /// </summary> | ||
| ITransformationPattern? Pattern { get; } |
There was a problem hiding this comment.
I wonder if we should decouple the pattern and the transformation. It could reduce code-duplication, as the transformation would not need to implement querying for relevant nodes each time. In exchange, Apply would change to something like Apply(Tree, Node).
There was a problem hiding this comment.
So a transformation would only handle the transformation of individual types as opposed to entire tree? How would the transformation know what to transform if not supplied with a pattern to work off of?
There was a problem hiding this comment.
Yes, it would handle the individual node. I'd say the situation depends on the transformation. A transformation might not need a pattern because it works with the entire tree (in which case the node would be null). If a pattern is not provided for a transformation that works on some node, you could come up with a sane default (like all nodes are transformed or something).
You could also keep the current design, I just feel like the node matching logic should be externalized to minimize code repetition.
There was a problem hiding this comment.
I just feel like the node matching logic should be externalized to minimize code repetition.
I agree with this, although I'm still wondering about the transformation stuff. If a transformation should transform a member of a node, how would it know which member to transform without having a pattern to find the member(s) by? Or are members also "nodes" and can therefore also be sent to Apply?
There was a problem hiding this comment.
Ooooh fair point, I forgot about members honestly. That's a tough one, I have no immediate answer to that.
There was a problem hiding this comment.
I could personally see three possible solutions for this:
-
ITransformationhas two overloads forApply,Apply(Tree, TypeNode)andApply(Tree, TypeNode, MemberNode). For transformations which only transform one of them, the other can simply return the input tree. -
Alternatively it could be
Apply(Tree, TypeNode, MemberNode?)where the member could be null if only the type should be transformed. -
There are two sub-interfaces of
ITransformation,ITypeTransformationwhich deals with transformations of types, andIMemberTransformationwhich deals with transformations of members. A simple switch expression would be enough to call the appropriate method.
src/NanopassSharp/Pass.cs
Outdated
| ( | ||
| string Name, | ||
| string? Documentation, | ||
| Pass Previous, |
There was a problem hiding this comment.
If this was mutable, both the prev. and the next pass could be referenced.
src/NanopassSharp/Pass.cs
Outdated
| /// </summary> | ||
| /// <param name="Root">The root node.</param> | ||
| /// <param name="Nodes">The nodes of the tree.</param> | ||
| public sealed record class TypeNodeTree |
There was a problem hiding this comment.
I'm not a fan of this name, it doesn't immediately yell that this is the node hierarchy.
src/NanopassSharp/Pass.cs
Outdated
| /// <param name="Nodes">The nodes of the tree.</param> | ||
| public sealed record class TypeNodeTree | ||
| ( | ||
| TypeNode Root, |
There was a problem hiding this comment.
Is there a possibility for multiple root nodes? Or is this a non-concern for now?
There was a problem hiding this comment.
Depends on what kind of hierarchy we're trying to model, I suppose. In languages like C# and Java, nested types exist which would naturally lead to tree-like structure, but in other langs, the types would just have to be laid out flat and the tree structure implied by the referenced types in each type.
In these other langs, would each node be a root node?
There was a problem hiding this comment.
I'd say that you are still modeling a hierarchy, what this compiles to is a detail.
src/NanopassSharp/Pass.cs
Outdated
| /// <param name="Children">The children of the node (typically nested types).</param> | ||
| /// <param name="Members">The members of the type.</param> | ||
| /// <param name="Attributes">The language-specific attributes of the type.</param> | ||
| public sealed record class TypeNode |
There was a problem hiding this comment.
Again, the name is a bit cryptic. Why TypeNode? Why not AstNode?
src/NanopassSharp/Pass.cs
Outdated
| /// May be <see langword="null"/> in dynamically typed languages | ||
| /// or languages without explicitly typed members.</param> | ||
| /// <param name="Attributes">The language-specific attributes of the member.</param> | ||
| public sealed record class Member |
There was a problem hiding this comment.
Again, I'd consider the naming here. Maybe AstNodeMember? Or even ClassMember? NodeProperty?
src/NanopassSharp/Pass.cs
Outdated
| /// <param name="Documentation">The pass' corresponding documentation.</param> | ||
| /// <param name="Transformations">The transformations the pass applies to its nodes.</param> | ||
| /// <param name="Tree">The original node tree of the pass.</param> | ||
| public sealed record class Pass |
Update the model used for representing type node trees and passes in accordance with the language overhaul proposal (see #2). The exact implementation is still being actively discussed and this PR currently serves as more of a test to see what would work.