Roslyn: Change signature can misplace comments, whitespace

Created on 1 Mar 2019  路  4Comments  路  Source: dotnet/roslyn

Version Used: 2019 Preview 4

Steps to Reproduce:

```C#
class Program
{
static void F(int a, int b, int c, int d)
{
}

    static void Main(string[] args)
    {
        F(1,
          2,// 2
          3,
          4);
    }
}

Remove parameter `int a` using Change Signature.

**Expected Behavior**: Comments stay with their logically related argument/parameter

**Actual Behavior**: 

Comments can end up in unexpected locations, sometimes logically related to the wrong entry.
Indentation is not preserved.

```C#
namespace ConsoleApp3
{
    class Program
    {
        static void F(int b, int c, int d)
        {
        }

        static void Main(string[] args)
        {
            F(2,
                3,// 2
                4);
        }
    }
}
Area-IDE Bug IDE-CodeStyle help wanted

Most helpful comment

@dpoeschl Turns out I still have my branch fix1017-inline-temp-comments where I added a ITriviaLogicalOwnershipAssignmentService interface in commit 2bc2bd60. (Pardon all the "updates" commits.)

a looooong time ago

Just shy of 5 years ago, NBD.

All 4 comments

@jinujoe @dpoeschl We should prioritize fixing this. It makes the refactoring useless when the call-sites use indented arguments or comments -- it is easier to update the signature manually then to use the refactoring and then fix up all the messed up whitespace and misplaces comments.

@tmat @dpoeschl In the following scenario, if parameters a and b are switched, how/where would you expect // a to show up? I'm looking into this bug now, but there seems to be some edge cases that I'm unclear on how to resolve.
One alternative could be to switch // a to /* a */, but this wouldn't work in VB.
C# public void M(int a, // a /* b */ int b, int c /* c */) { }

Proposal:

  1. Associate comments with parameters
  2. Rearrange ignoring comments
  3. Add comments back (transforming // into /**/ when needed in C#, or adding newlines where necessary in VB)

So in this case
1)
a: //a
b: /* b */
c: /* c */

2)
```C#
public void M(int b,
int a, int c);


3) In C#
```C#
public void M(/* b */ int b,
                    int a /* a */, int c /* c */);

3) In VB

Public Sub M(int b, ' b
                    int a, ' a
                    int c) 'c

Open to other thoughts for sure.

Tagging @brettfo since we talked about doing a comment association service a looooong time ago. :)

@dpoeschl Turns out I still have my branch fix1017-inline-temp-comments where I added a ITriviaLogicalOwnershipAssignmentService interface in commit 2bc2bd60. (Pardon all the "updates" commits.)

a looooong time ago

Just shy of 5 years ago, NBD.

Was this page helpful?
0 / 5 - 0 ratings