Cannot rename named tuples

[jmarolf]

(Updated - jcouv: a more detailed spec-in-progress below)

Steps to Reproduce:

1. Create a new C# project
2. Have some code like:

var tuple = (number: 5, greeting: "Hello");
var number = tuple.number;
var greeting = tuple.greeting;

1. select greeting in tuple.greeting
2. attempt to rename greeting

Expected Behavior: Inline rename offered for tuple.greeting

Actual Behavior:

[jcouv]

@jaredpar brought up an interesting one more complexity with rename with tuples: renaming in scenarios with OHI (overriding, hiding, implementing).

For instance:

virtual void M((int a, int b) c) {...}
override void M((int a, int b) x) {...}

Renaming a should affect both signatures (they need to match).

[rik-smeets]

Since I noticed too that renaming named tuples is currently not possible, I looked into this issue a bit.

It looks like some of the work done on value tuples since this issue was opened might make enabling rename refactoring possible soon.

With just some minor changes, I can get the rename refactoring to work in most scenarios, except the one with OHI as mentioned by @jcouv above. A rename can be performed in that scenario, but the signature in the base class is not changed. This results in an error, which is easily recoverable.

The scenarios I tested are:

private void TupleRename()
{
    // amount and text can be renamed, updating declaration as well
    var tuple = (amount: 6, text: "Hello");
    var amount = tuple.amount;
    var text = tuple.text;
}

public void CallMethodWithNamedTuple()
{
    TupleRenameParameter((amount: 5, anotherAmount: 10));
}

private void TupleRenameParameter((int amount, int anotherAmount) tuple)
{
    // amount and text can be renamed, updating declaration in parameter as well 
    // and in callers to method
    var amount = tuple.amount;
    var text = tuple.anotherAmount;
}

private void TupleRenameTernary()
{
    // Rename of 'a' renames all declarations of 'a' (i.e. three times)
    var ternary = true ? (a: 1, b: 2) : (a: 11, b: 22);
    var ternaryResult = ternary.a;
}

private void TupleRenameInvalidTernary()
{
    // Rename of 'ternary.a' is not allowed because locations cannot be determined due to warnings on tuple declarations
    var invalidTernary = true ? (a: 1, b: 2) : (a: 1, c: 3);
    var invalidTernaryResult = invalidTernary.a;
}

As you can see, I also tested the examples of #16566 by @kuhlenh.

BTW: In the changes I did, #19578 is resolved too. The NullRef is caused by trying to rename a tuple of which the current declaration is invalid, causing the exception when trying to get the locations to rename.

Please let me know what you think. I can of course open a pull request with my changes if you'd like, but I wanted to hear your opinion first, especially since I don't know how to resolve the OHI scenario. Thanks in advance!

[sharwell]

We actually have a design proposal that is mostly complete that hasn't been posted here yet. If we can make that public it would certainly allow someone to complete the initial implementation.

[TessenR]

I guess there would be some problems when tuples cross methods' boundaries Would it work in this case for example? Can I start refactoring from both the type argument and the tuple component's usage?

T Get<T>() => default;
void M()
{
  var x = Get<(int a, int b)>();
  Console.WriteLine(x.a);
}

with type inference?

T Get<T>(T t) => default;
void M()
{
  var x = (a: 1, b: 2);
  var y = Get(x);
  Console.WriteLine(y.a);
}

would it track type arguments?

void M()
{
  var abs = new List<(int a, int b)>();
  foreach (var ab in abs)
    Console.WriteLine(ab.a);
}

multiple affected type members and their usages?

struct Pair<T, V>
{
  T First => default;
  V Second => default;
  public void Deconstruct(out T t, out V v) => throw null;
  void M()
  {
    var pair = new Pair<(int a, int b), (int x, int y)>(); // rename here?
    var (abs, xys) = pair;
    Console.WriteLine(abs.a + pair.First.a);  // or here?
    Console.WriteLine(xys.x + pair.Second.x); // or here?
  }
}

also I believe that renaming tuples in return types should affect return statements in cases like this:

(bool firstParam, string secondParam) M()
{
  if (SomeCondition())
    return (firstParam: false, secondParam: null);

  if (SomeOtherCondition())
    return (firstParam: true, secondParam: null);
}

as they are clearly intended to provide hints for the constant expressions used in tuple expressions under returns. Actually mismatching names here would result in compiler warninings (and errors if 'warnings as errors' option is enabled)

These are just the simplest examples immediately springing to mind. Basically it should support all kinds of type inference and type substitution provided by the language.

[sharwell]

@TessenR Thanks for the additional examples. I need to write up the proposal and highlight a number of cases that we need to figure out. Then we can talk about which ones need to be handled in the initial work and which ones can wait for later.

[TessenR]

Just for the test suite there are also things like best common type inference, i.e. (same with array creation expressions)

    T M<T>(T t1, T t2)
    {
      var ab = (a: 1, b: 2); // renaming a here
      var ac = (a: 1, c: 2); // or here
      var x = M(ab, ac);
      Console.WriteLine(x.a); // should change this to Item1
      return default;
    }

[sharwell]

To expand the brainstorming for examples here are two more:

public (int c, int d) void M(int c) // Rename parameter 'c' to 'a'
{
  var t = (c, 2);

  Console.WriteLine(t.c);

  return (t.c, 3);
}

struct V1 { public int x; }
struct V2 { public int y; } // Rename 'y' to 'x'

void M(V1 v1, V2 v2) {
  var tuple = (v1.x, v2.y);
  Console.WriteLine(tuple.x);
}

[sharwell]

As a general concept, the shapes of tuples contained within the same method is considered when renaming a named tuple element. For example:

T M<T>(T t1, T t2)
{
  var ab = (a: 1, b: 2); // rename 'a' to 'd'
  var ac = (a: 1, c: 2);
  var x = M(ab, ac);
  Console.WriteLine(x.a);
  return default;
}

Should produce the following result:

T M<T>(T t1, T t2)
{
  var ab = (d: 1, b: 2);
  var ac = (d: 1, c: 2);
  var x = M(ab, ac);
  Console.WriteLine(x.d);
  return default;
}

[TessenR]

But you can't always rename the source of a tuple component's name... How about this? Do you expect renaming a component of ab to change the parameter's name? upd: well I guess you can add an explicit name for ac. Seems not intuitive though

T M<T>(T a, T t2)
{
  var ab = (a: 1, b: 2);
  var ac =(a, c: 2);
  var x = M(ab, ac);
  Console.WriteLine(x.a);
  return default;
}

[sharwell]

Example 1: Rename explicit named element

Principles:

• Propagate forward at a name inference boundary
• Consider tuples with the same shape in the same method
• Identifiers which are treated as an implicit name trigger renaming of both the tuple element and the target of the reference

T M<T>(T a, T t2)
{
  T t1 = default;
  var ab = (a: t1, b: t2); // Rename 'a' to 'd'
  var ac =(a, c: t2);
  var x = M(ab, ac);
  Console.WriteLine(x.a);
  return default;
}

Produces this:

T M<T>(T a, T t2)
{
  T t1 = default;
  var ab = (d: t1, b: t2);
  var ac =(d: a, c: t2);
  var x = M(ab, ac);
  Console.WriteLine(x.d);
  return default;
}

Example 2: Rename parameter

Principles:

• Propagate forward at a name inference boundary
• Consider tuples with the same shape in the same method

T M<T>(T a, T t2) // Rename 'a' to 'd'
{
  T t1 = default;
  var ab = (a: t1, b: t2);
  var ac =(a, c: t2);
  var x = M(ab, ac);
  Console.WriteLine(x.a);
  return default;
}

Produces this:

T M<T>(T d, T t2)
{
  T t1 = default;
  var ab = (d: t1, b: t2);
  var ac =(d, c: t2);
  var x = M(ab, ac);
  Console.WriteLine(x.d);
  return default;
}

Example 3: Rename implicit named element

Principles:

• Propagate forward at a name inference boundary
• Consider tuples with the same shape in the same method
• Identifiers which are treated as an implicit name trigger renaming of both the tuple element and the target of the reference

T M<T>(T a, T t2)
{
  T t1 = default;
  var ab = (a: t1, b: t2);
  var ac =(a, c: t2); // Rename 'a' to 'd'
  var x = M(ab, ac);
  Console.WriteLine(x.a);
  return default;
}

Produces this:

T M<T>(T d, T t2)
{
  T t1 = default;
  var ab = (d: t1, b: t2);
  var ac =(d, c: t2);
  var x = M(ab, ac);
  Console.WriteLine(x.d);
  return default;
}

Example 4: Rename usage of implicit named element

Principles:

• Qualify inputs rather than propagate in reverse

T M<T>(T a, T t2)
{
  T t1 = default;
  var ab = (a: t1, b: t2);
  var ac =(a, c: t2);
  var x = M(ab, ac);
  Console.WriteLine(x.a); // Rename 'a' to 'd'
  return default;
}

Produces this:

T M<T>(T a, T t2)
{
  T t1 = default;
  var ab = (d: t1, b: t2);
  var ac =(d: a, c: t2);
  var x = M(ab, ac);
  Console.WriteLine(x.d);
  return default;
}

[rik-smeets]

Great brainstorming. Another thing to consider is the following:

    class Test
    {
        void A()
        {
            var tupleA = (a: "", b: "");
        }
    }
    class Test2
    {
        void B()
        {
            var tupleB = (a: "", b: "");
        }
    }

Right now, selecting a or b also selects the tuple in the other method in the other class (which is then also renamed). I don't think that's desired.

[TessenR]

What would this produce I wonder?

class C<T>
{
  T Property;
  void M(C<(int a, int b)> c)
  {
     var q = new[] { c.Property, (a: 1, c: 2) }; // rename 'a' here
     System.Console.WriteLine(q[0].a);
  }
}

And just in case it's still supposed to rename the type argument, how about this?

class C
{
  (int a, int b) Property;
  void M(C c)
  {
     var q = new[] { c.Property, (a: 1, c: 2) }; // rename 'a' here
     System.Console.WriteLine(q[0].a);
  }
}

[sharwell]

What would this produce I wonder?

:memo: It matters where the refactoring is triggered and what the new name is.

Note that I updated my previous comment to show all the cases for identifiers a.

[sharwell]

Proposal for Tuple Renaming

🚧 This post is a work in progress. I will add to it as examples are added to reveal specific behaviors.

Fundamentals

• When renaming an identifier that represents both an implicit tuple element name and a reference to a symbol defined elsewhere, the rename operation is treated equivalently to a rename operation of the referenced element. This equivalence forms the basis of "propagate forward" and "propagate backward" used below, and explains the output difference between Example 1 and Example 3 of https://github.com/dotnet/roslyn/issues/14115#issuecomment-388105614.
• At name inference boundaries, rename operations propagate forward.
• At name inference boundaries, rename operations qualify inputs when propagating in reverse.
• When a tuple element is renamed, all tuples with the same shape in the same method are examined. For all such tuples with the same element name in the same position, the element is renamed. To restrict most rename operations to code within a method, this consideration does not include tuple types in the member signature (return values or parameters).
  • TODO: Consider limiting "same method" to "same or nested scope", which naturally covers the case of signatures.

[effyteva]

Any updates on this one? It seems to be forgotten for almost a year now...

[jnm2]

• When a tuple element is renamed, all tuples with the same shape in the same method are examined. For all such tuples with the same element name in the same position, the element is renamed. To restrict most rename operations to code within a method, this consideration does not include tuple types in the member signature (return values or parameters).
  • TODO: Consider limiting "same method" to "same or nested scope", which naturally covers the case of signatures.

It makes sense to consider local functions the same as methods in this regard because they have signatures, right? Whereas anonymous methods and lambdas would just be treated the same as any other nested scopes?

These rules don't mention what should happen when a tuple element is renamed but it's not within a method. E.g. in an initializer, I'd assume all tuples with the same shape in the same initializer are examined:

class Foo
{
    // Rename any 'A' in the initializer to 'D'
    private readonly (int A, int B) bar = ((A: 1, B: 2).A + (A: 1, B: 2).B);
}

class Foo
{
    private readonly (int A, int B) bar = ((D: 1, B: 2).D + (D: 1, B: 2).B);
}

[jnm2]

Also, would renames inside : this(...) flow to occurrences in the body and vice versa?

[davidwengier]

I don't understand why I would want the behavior in example 2. Is there a real world example that came from? Why would renaming a parameter that happens to have the same name and type as an otherwise unrelated tuple field propagate onto the tuple field? Is that supposed to be a tuple parameter on M()?

[AraHaan]

Jetbrains Rider recently shipped with this, and ReSharper as well.

Perhaps the implementation could be implemented however JetBrains does it into Roslyn as well.

[CyrusNajmabadi]

Dupe of https://github.com/dotnet/roslyn/issues/20115