refactor: Store a reference to LangProfile in each AstNode #386

wetneb · 2025-05-15T00:13:09+02:00

wetneb commented

2025-05-15 00:13:09 +02:00

This is the first step towards #5. Keeping track of which language a node comes from will make it possible to store nodes coming from different languages in the same tree, while still being able to look up their commutative parent and signature definitions from the correct language profile.

@ada4a I've tried to make sufficiently granular commits to ease the review, I hope it works for you! I'm curious how much reviewing effort that represents on your side :)

This is the first step towards #5. Keeping track of which language a node comes from will make it possible to store nodes coming from different languages in the same tree, while still being able to look up their commutative parent and signature definitions from the correct language profile. @ada4a I've tried to make sufficiently granular commits to ease the review, I hope it works for you! I'm curious how much reviewing effort that represents on your side :)

wetneb added 11 commits

2025-05-15 00:13:10 +02:00

Keep track of which language each node comes from 8fcb0d76be

Remove lang_profile arguments where unnecessary f5728a4474

Move has_signature_conflicts to AstNode 97a644750c

Remove lang_profile argument from ascii_tree 71aaafa40c

Remove lang_profile field from TreeMatcher 7c16d8e0c9

Propagate lang_profile to Leader 2341517873

Move extract_signature_from_merged_node to MergedTree 9ef61f0b3e

Move extract_signature_from_original_node to AstNode 3cfad0cf13

Remove lang_profile argument in merge_postprocessor 63d6f622b3

Add commutative_parent_definition on AstNode and Leader 3f8b4d172e

Remove lang_profile field from TreeBuilder and upstream

/ test (pull_request) Successful in 52s

Details

85078473be

ada4a requested changes

2025-05-15 08:56:16 +02:00

Dismissed

ada4a left a comment

Very nice! Having commits nicely split like this was indeed very helpful.

I didn't have too many things to add this time -- maybe more will come to mind when I take another look at this later

Very nice! Having commits nicely split like this was indeed very helpful. I didn't have _too_ many things to add this time -- maybe more will come to mind when I take another look at this later

src/ast.rs Outdated

					
				@ -705,0 +712,4 @@

				                    .children

				                    .iter()

				                    .copied()

				                    .filter_map(AstNode::signature)

ada4a commented

2025-05-15 08:29:34 +02:00

nice:)

src/matching.rs Outdated

					
				@ -37,6 +37,7 @@ impl<'tree> Matching<'tree> {

				    /// Is it possible to add this pair while keeping the matching consistent?

				    pub fn can_be_matched(&self, from: &AstNode<'tree>, to: &AstNode<'tree>) -> bool {

				        from.grammar_name == to.grammar_name

				            && from.lang_profile.name == to.lang_profile.name // TODO rely on a better identifier, as there could be name collisions? Just pointer equality?

ada4a commented

2025-05-15 08:12:52 +02:00

I'd say add a test to supported_langs.rs that there are no name collisions, and then continue using this comparison, yes.

Also it might be a good idea to have a helper for this, something like LangProfile::eq (see Iterator::eq for a precedent), which would initially just compare names, but which we'd be free to refactor/improve later, instead of needing to modify all the code everywhere

I'd say add a test to `supported_langs.rs` that there are no name collisions, and then continue using this comparison, yes. Also it might be a good idea to have a helper for this, something like `LangProfile::eq` (see [`Iterator::eq`](https://doc.rust-lang.org/std/iter/trait.Iterator.html#method.eq) for a precedent), which would initially just compare `name`s, but which we'd be free to refactor/improve later, instead of needing to modify all the code everywhere

👍 1

ada4a commented

2025-05-15 10:24:04 +02:00

Hm, not sure if this should be a PartialEq impl (I was thinking of a free-standing method), but since we've decided names are unique identifiers, this is probably okay?

Hm, not sure if this should be a `PartialEq` impl (I was thinking of a free-standing method), but since we've decided names are unique identifiers, this is probably okay?

src/merged_tree.rs Outdated

					
				@ -829,0 +837,4 @@

				        let definition = match self {

				            MergedTree::ExactTree { node, .. }

				            | MergedTree::MixedTree { node, .. }

				            | MergedTree::LineBasedMerge { node, .. } => node

ada4a commented

2025-05-15 08:51:07 +02:00

The following 3 lines (the ability to highlight multiple lines for a comment is something I always miss from GitLab), are basically AstNode::signature, but we can't call that because we have a Leader instead. Maybe it'd make sense to surface that method to Leader, like it was done with AstNode::commutative_parent_definition?

Though one might argue that it's less important in this case, since that newly created method would only really be used here, whereas Leader::commutative_parent_definition is used in at least 3 places

The following 3 lines (the ability to highlight multiple lines for a comment is something I always miss from GitLab), are basically `AstNode::signature`, but we can't call that because we have a `Leader` instead. Maybe it'd make sense to surface that method to `Leader`, like it was done with `AstNode::commutative_parent_definition`? Though one might argue that it's less important in this case, since that newly created method would only really be used here, whereas `Leader::commutative_parent_definition` is used in at least 3 places

ada4a commented

2025-05-15 09:01:15 +02:00

OTOH adding Leader::commutative_parent_definition would remove the only use of Leader::lang_profile, so we could be able to remove that at least? But maybe having access to a leader's language profile is helpful in some other occasions

OTOH adding `Leader::commutative_parent_definition` would remove the only use of `Leader::lang_profile`, so we could be able to remove that at least? But maybe having access to a leader's language profile is helpful in some other occasions

wetneb added 2 commits

2025-05-15 10:05:48 +02:00

Define LangProfile::eq and check that it's an identifier 55c470872c

Introduce signature_definition() methods

/ test (pull_request) Failing after 43s

Details

1721dca76d

wetneb added 1 commit

2025-05-15 10:10:32 +02:00

Add semicolon that my rustfmt doesn't add by itself, weird

/ test (pull_request) Successful in 57s

Details

b4af023f28

ada4a commented

2025-05-15 10:25:39 +02:00

I think the reason the semicolon wasn't added is that assert! technically returns (), so it could be a last expression of a block (or a function, in this case), defining the return type of the latter

I think the reason the semicolon wasn't added is that `assert!` technically returns `()`, so it could be a last expression of a block (or a function, in this case), defining the return type of the latter

ada4a commented

2025-05-15 10:26:26 +02:00

Do you mind if I leave this open for a bit longer? I'd like to take another look at it this evening