Sysmagine / SemanticDiff

Community support for SemanticDiff, the programming language aware diff for Visual Studio Code and GitHub.
https://semanticdiff.com
49 stars 0 forks source link

Brackets reported as semantic changes #16

Closed Zwatotem closed 1 year ago

Zwatotem commented 1 year ago

Describe the Bug In C# removing the namespace block causes practically the whole file to be outdented one level. The outdent is threated as not meaningful (as expected), but not for the lines containing brackets.

To Reproduce Steps to reproduce the behavior:

  1. Get a piece of code with a class and several methods, where the class is enclosed in a namespace block.
  2. Replace the namespace block with file-scoped namespace declaration.
  3. Make a change to the internals of a class, e.g. add some lines to one of the methods. The changes don't have to (or even shouldn't) change the tree of braced blocks.
  4. Diff the initial version with the final version.

Expected Behavior Only the braces of namespace block and the internal changes are highlighted. Any other braces shouldn't be highlighted.

Actual Behavior The expected changes are highlighted, however the braces and flow control statements that were merely outdented are also highlighted despite not caring any semantic changes.

Screenshots image

Source Code

// Old
using System;
using System.Collections.Generic;
using System.Text;
using ChatModel;
using ChatModel.Util;

namespace ChatServer.HandleStrategies
{
    /// <summary>
    /// Class handling request to add user to conversation.
    /// </summary>
    class HandleAddToConversationStrategy : IHandleStrategy
    {
        public void handleRequest(List<IClientHandler> allHandlers, IServerChatSystem chatSystem, IClientHandler handlerThread, byte[] messageBytes)
        {
            Console.WriteLine("DEBUG: {0} request received", "add user to conversation");
            //decoding request - first 4 bytes are id of conversation to which the user is to be added, following bytes are user name
            int conversationId = BitConverter.ToInt32(messageBytes, 0);
            string nameToAdd = Encoding.UTF8.GetString(messageBytes, 4, messageBytes.Length - 4);
            Console.WriteLine("DEBUG: trying to add user to conversation");
            byte[] reply = new byte[1]; //boolean reply has only 1 byte
            lock (allHandlers) //prohibit other threads from interfering
            {
                if (chatSystem.addUserToConversation(nameToAdd, conversationId))
                {
                    reply[0] = 1; //if adding successful set reply byte to one
                    byte[] msg = messageBytes;
                    Conversation conversation = chatSystem.getConversation(conversationId);
                    //and broadcast the change to all active handlers handling users present in the conversation
                    foreach (var handler in allHandlers.FindAll(h => conversation.Users.Exists(u => u.Name == h.HandledUserName)))
                    {
                        if (handler.HandledUserName == nameToAdd) //newly added user has to receive the entire conversation
                        {
                            byte[] update = conversation.serialize(new ConcreteSerializer()).ToArray();
                            handler.sendMessage(5, update); //serialized conversation - type 5
                        }
                        else
                        {
                            handler.sendMessage(4, msg); //user added to conversation - type 4. Forwarding received request.
                        }
                    }
                }
                else
                {
                    reply[0] = 0; //else set reply byte to one
                }
            }
            handlerThread.sendMessage(1, reply);
        }
    }
}

/*
One of concrete strategies of the implemented strategy pattern.
This class has only one responsibility.
Complies with Liskov Substitution Principle - all interface methods are properly implemented.
*/
// New
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using ChatModel;
using ChatModel.Util;

namespace ChatServer.HandleStrategies;

/// <summary>
/// Class handling request to add user to conversation.
/// </summary>
class HandleAddToConversationStrategy : IHandleStrategy
{
    public void handleRequest(List<IClientHandler> allHandlers, IServerChatSystem chatSystem,
        IClientHandler handlerThread, byte[] messageBytes)
    {
        Console.WriteLine("DEBUG: {0} request received", "add user to conversation");
        //decoding request - first 16 bytes are id of conversation to which the user is to be added, following bytes are user name
        Guid conversationId = new Guid(messageBytes[0..16]);
        string nameToAdd = Encoding.UTF8.GetString(messageBytes, 16, messageBytes.Length - 16);
        Console.WriteLine("DEBUG: trying to add user to conversation");
        byte[] reply = new byte[1]; //boolean reply has only 1 byte
        lock (allHandlers) //prohibit other threads from interfering
        {
            if (chatSystem.AddUserToConversation(nameToAdd, conversationId))
            {
                reply[0] = 1; //if adding successful set reply byte to one
                byte[] msg = chatSystem.GetUser(nameToAdd).Serialize(new ConcreteSerializer()).ToArray();
                Conversation conversation = chatSystem.FindConversation(conversationId);
                //and broadcast the change to all active handlers handling users present in the conversation
                foreach (var handler in allHandlers.FindAll(h =>
                            conversation.Users.Any(u => u.Name == h.HandledUserName)))
                {
                    if (handler.HandledUserName == nameToAdd) //newly added user has to receive the entire conversation
                    {
                        ConversationUpdates conversationUpdates = conversation.GetUpdates(DateTime.MinValue);
                        byte[] update = conversationUpdates.Serialize(new ConcreteSerializer()).ToArray();
                        handler.sendMessage(5, update); //serialized conversation - type 5
                    }
                    else
                    {
                        handler.sendMessage(7, msg); //user added to conversation - type 4. Forwarding received request.
                    }
                }
            }
            else
            {
                reply[0] = 0; //else set reply byte to one
            }
        }

        handlerThread.sendMessage(1, reply);
    }
}

/*
One of concrete strategies of the implemented strategy pattern.
This class has only one responsibility.
Complies with Liskov Substitution Principle - all interface methods are properly implemented.
*/

SemanticDiff Version v0.8.3

VS Code Information

Version: 176,0 (user setup)
Commit: 92da9481 c0904c6adfe372c12da3b7748d74bdcb
Date 2023-03-01 (2 mos ago)
Electron: 19.1.11
Chromium: 102.0.5005.196
Node.js: 16.14.2
V8: 10.2.154.26-electron.0
OS: Windows NTx64 10.022624
Sandboxed: No

Additional Context This is C# 10 or so. Didn't check other versions/languages, neither did I mutate the code to check under what conditions the problem exactly occurs.

mmueller2012 commented 1 year ago

Thanks for the detailed bug report! :+1:

The underlying problem is most likely not specific to namespaces or C# in general. SemanticDiff isn't always great at matching statements between the old and new code when a lot of code is moved or a parent statement is removed. This seems to be the case here. It can't correctly match some of the brackets and statements, so they show up as changes.

We are aware of this issue and have some ideas on how to fix it. Hopefully we can resolve this in the next release. I will keep you updated.

mmueller2012 commented 1 year ago

We have just released SemanticDiff 0.8.5 which should fix this issue. Can you confirm this?

Zwatotem commented 1 year ago

We have just released SemanticDiff 0.8.5 which should fix this issue. Can you confirm this?

Sorry for the long time to reply. Yes, this looks to t fixed. Thank you very much :)