fix: Avoid displaying spurious conflicts #371

wetneb · 2025-05-09T19:28:36+02:00

wetneb commented

2025-05-09 19:28:36 +02:00

Fixes #356.

Not including the original case as integration test case because I didn't manage to minimize it sufficiently. I think the unit test is enough.

Fixes #356. Not including the original case as integration test case because I didn't manage to minimize it sufficiently. I think the unit test is enough.

ada4a commented

2025-05-09 21:13:49 +02:00

Hm, I don't think this is the right approach to the problem – it's of course nice (especially for the end user) to graciously handle these spurious conflicts, but imo we should try to find the reason such conflicts appear in the first place, like with #170

ada4a commented

2025-05-09 21:32:34 +02:00

Or maybe we could merge this as is, and then I could investigate the issue when I have time?

wetneb commented

2025-05-10 02:59:35 +02:00

I agree that it feels unsatisfactory to have this check at such a downstream case, but to me it's a legitimate safety belt to add. Conflicts with identical sides can be created in at least two places:

when dealing with isomorphic nodes that are unmatched for one reason or another, in merged_tree.rs. We could add another check for isomorphism there, it's true (for now we only detect the left == right case there too). It's not clear to me if it would be much more efficient
but also in the highlighting of duplicate signatures (same)

Maybe the logic between those two places could be shared in a helper method that creates a Conflict or a list of ExactTree depending on whether any two of the three sides are isomorphic or not. I'd still keep the downstream check to make sure we never output spurious conflicts, it's cheap enough, no?

I agree that it feels unsatisfactory to have this check at such a downstream case, but to me it's a legitimate safety belt to add. Conflicts with identical sides can be created in at least two places: * when dealing with isomorphic nodes that are unmatched for one reason or another, in merged_tree.rs. We could add another check for isomorphism there, it's true (for now we only detect the `left == right` case there too). It's not clear to me if it would be much more efficient * but also in the highlighting of duplicate signatures (same) Maybe the logic between those two places could be shared in a helper method that creates a `Conflict` or a list of `ExactTree` depending on whether any two of the three sides are isomorphic or not. I'd still keep the downstream check to make sure we never output spurious conflicts, it's cheap enough, no?

wetneb force-pushed 356-spurious-conflicts from 3fba8c55f7 to 5fc178a961

2025-05-10 08:04:07 +02:00

Compare

wetneb force-pushed 356-spurious-conflicts from fe9036124e to 3e560bead7

2025-05-10 08:39:44 +02:00

Compare

wetneb force-pushed 356-spurious-conflicts from 3e560bead7 to cd351b0470

2025-05-10 08:42:33 +02:00

Compare

wetneb force-pushed 356-spurious-conflicts from cd351b0470 to 609133ae78

2025-05-10 08:56:41 +02:00

Compare

wetneb force-pushed 356-spurious-conflicts from 609133ae78 to 165400d47f

2025-05-10 09:40:37 +02:00

Compare

wetneb commented

2025-05-10 11:01:03 +02:00

I've added another commit to check for the isomorphism of sides earlier on, which can help catch some spurious conflicts in the presence of reformatting. Sadly, the method I introduced for that (MergedTree::new_conflict) can't be used in the other place where we create conflicts (the highlighting of duplicate signatures) because at that stage we need to check for isomorphism of merged trees directly, so it's not quite the same code.

I've added another commit to check for the isomorphism of sides earlier on, which can help catch some spurious conflicts in the presence of reformatting. Sadly, the method I introduced for that (`MergedTree::new_conflict`) can't be used in the other place where we create conflicts (the highlighting of duplicate signatures) because at that stage we need to check for isomorphism of merged trees directly, so it's not quite the same code.

ada4a reviewed

2025-05-10 14:38:02 +02:00

src/merged_tree.rs

					
				@ -115,0 +118,4 @@

				        left: Vec<&'a AstNode<'a>>,

				        right: Vec<&'a AstNode<'a>>,

				        class_mapping: &ClassMapping<'a>,

				    ) -> Vec<Self> {

ada4a commented

2025-05-10 14:38:02 +02:00

it's a bit weird that we return a vec in a method which says it should return a conflict normally, and I guess we could use Either<Self, Vec<Self>>, but the value of that would be... questionable

it's a bit weird that we return a vec in a method which says it should return _a_ conflict normally, and I guess we could use `Either<Self, Vec<Self>>`, but the value of that would be... questionable

ada4a commented

2025-05-10 14:38:46 +02:00

other than that, I really like that we've extracted this somewhat messy logic into a method!

wetneb commented

2025-05-10 14:40:46 +02:00

Yes it's a bit wonky… By the way, I originally tried to make this method return iterators directly, since all we do is just append those elements to another vector, but I convinced myself that the ownership rules make that unfeasible. Do you agree?

ada4a commented

2025-05-10 14:58:02 +02:00

Oh, thanks for the heads-up! I played a bit around with it, and did in fact manage to do that. Granted, I ended up needing the Either after all... I'll try opening a PR to your branch to demonstrate the steps, see if you like the end result

Oh, thanks for the heads-up! I played a bit around with it, and did in fact manage to do that. Granted, I ended up needing the `Either` after all... I'll try opening a PR to your branch to demonstrate the steps, see if you like the end result

ada4a marked this conversation as resolved

ada4a commented

2025-05-10 15:16:29 +02:00

It's not clear to me if it would be much more efficient

I'm not really concerned with efficiency in this case to be fair -- what I fear is that noticing the spurious conflict too late might lead to actual correctness issues, like with #170

I'd still keep the downstream check to make sure we never output spurious conflicts, it's cheap enough, no?

Yeah yeah I'm sure it'll be plenty cheap. Let's keep it

> It's not clear to me if it would be much more efficient I'm not really concerned with efficiency in this case to be fair -- what I fear is that noticing the spurious conflict too late might lead to actual correctness issues, like with #170 > I'd still keep the downstream check to make sure we never output spurious conflicts, it's cheap enough, no? Yeah yeah I'm sure it'll be plenty cheap. Let's keep it

wetneb added 6 commits

2025-05-10 19:24:18 +02:00

use into_iter 2aab49821d

turn into fn 6d97d27ffc

return impl Iterator 30520f9da5

doesn't compile, since the types of the last branch is technically
different from the rest

move to fix the ownership problem 82ddd32cfe

all we needed was to have `{first,second}_rev` inside the closure

use Either to avoid the "different types" error 92faed6fdf

restore iter::once to make the overall Either an Iterator

/ test (pull_request) Successful in 58s

Details

76fb75154f

could replace the `iter::Once` in the return type signature, but I think
it's actually nice, since it signifies that in that case, we return the
`MergedTree:Conflict`, _once_

ada4a approved these changes

2025-05-10 20:23:06 +02:00

wetneb merged commit a007e90952 into main

2025-05-10 21:11:47 +02:00

wetneb referenced this pull request from a commit

2025-05-10 21:11:48 +02:00

fix: Avoid displaying spurious conflicts (#371)

Sign in to join this conversation.

No reviewers