[C#] Prefer highlighting inner generic brackets

[Ririshi]

Not sure if this is a bug or just a design choice, but I have some feedback on which pair of generic brackets should be highlighted when changing a generic type.

To Reproduce
Steps to reproduce the behavior:

1. Open a diff for the below source code snippets.
2. See "wrongly" highlighted generic type diff.

Expected Behavior
The inner generic brackets should be highlighted. Conceptually, this is what was added: I wrapped my original return type inside a generic class. One could even argue that the whole generic Nullable<int> should be highlighted instead of just the Nullable<> part, because a Nullable<int> might have been be something completely different from the bare int.

Actual Behavior
The outer generic brackets are highlighted.

Screenshots
If applicable, add screenshots to help explain your problem.

Source Code
Old:

namespace Test;

public class TestClass
{
    public IEnumerable<int> TestProperty => [0];
}

New:

namespace Test;

public class TestClass
{
    public IEnumerable<Nullable<int>> TestProperty => [0];
}

Of course you wouldn't actually write this, but I just wanted to show a (presumably?) compilable snippet.

SemanticDiff Version
0.10.0

VS Code Information
Simply click Help -> About -> Copy in VS Code and paste the information:

Version: 1.96.4 (user setup)
Commit: cd4ee3b1c348a13bafd8f9ad8060705f6d4b9cba
Date: 2025-01-16T00:16:19.038Z
Electron: 32.2.6
ElectronBuildId: 10629634
Chromium: 128.0.6613.186
Node.js: 20.18.1
V8: 12.8.374.38-electron.0
OS: Windows_NT x64 10.0.19045

Edit: I accidentally opened this ticket well before I was finished typing. I edited it completely to add in everything I meant to add originally. Sorry about that!

[Ririshi]

Another example:

Intuitively I would expect the List/IEnumerable's brackets not to be highlighted. I would expect IEnumerable and Simulated<> to be highlighted (but not BrickData). But perhaps the fact that List and IEnumerable are similar is just a human insight and there might not be a way to correctly highlight this without explicitly adding rules about such cases. I assume it would be hard to detect that the part that was added is Simulated being "inserted" in into the existing type.

[mmueller2012]

In the case of your first example this is not an intentional design choice. Our algorithm finds both solutions but considers them equally good and picks one of them. I understand that in such a simple case it might be better to highlight the brackets as you described. However, I am not sure if it is possible to add a generic rule for this that would also work for more complex cases.

In the second example, SemanticDiff actually detects that List has been changed to IEnumerable. Not because SemanticDiff knows that both types have a similar meaning, but because they are located at the same position in the abstract syntax tree and renaming a node is considered less costly than adding/removing it completely. However, in this case I am not sure which solution is actually better. Your idea would better represent the structure of the code, but it would interrupt the highlighted code block for a single character. This adds some visual noise and is easy to overlook.

@slackner: What is your opinion? Can/should we do something about this?

[Ririshi]

I think a straightforward fix would be to prefer highlighting matching brace levels when a new node is added. In the first example, Nullable is a new node, which comes with a pair of brackets that goes after it, surrounding int. If the algorithm always prefers the matching brace pair as a tie-breaker for otherwise "equal" solutions, it should lead to a (more) correct highlighting of type changes.

For the second example, the current implementation's highlighting wrongly suggests that List was renamed to Simulated and IEnumerable was added as a new node. I feel like the fix I just suggested would also fix this one. The changes would be:

• Rename List → IEnumerable (no brace highlighting required),
• Add new node Simulated, highlight the matching brace pair (around BrickData).

I do not believe that interrupting the highlighting would introduce more noise in this case. To my eyes, an interruption signals a separate change, which is accurate since the change consists of a rename (type change) and an added level of generic nesting.

That said, there are cases where interrupting the highlighting would cause noise and simply highlighting the whole type makes more sense. For example, if the change was List<int> → Simulated<BrickData>, there are no matching elements and the meaning of the new type is entirely different. In this case, since both levels changed, I would say it makes sense to highlight the entire type including the brackets.

Perhaps this concept of whether a parent or child node has an element matching the old code can be used to decide how much to highlight? Beginning at the innermost node, check if the node changed, and work your way up until you encounter a node that stayed the same. Then, highlight all of the code that matches that "chunk" of changes.

[slackner]

Hello @Ririshi,

thank you for the bug report. 👍

I was able to identify what was causing the incorrect matching, and have corrected the issue. It resolved both of the examples you provided, which now match the expected result.

This issue will be updated once we have released a version that includes the fix.

Best Regards,
Sebastian