dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
18.92k stars 4.01k forks source link

Convert tuple does not change camelCase element names to PascalCase field names #50505

Open tmat opened 3 years ago

tmat commented 3 years ago

Version Used: Version 16.9.0 Preview 3.0 [30904.5.main]

Steps to Reproduce:

    class Program
    {
        static (int a, int b) s_field;  // invoke convert tuple to type here

        static void Main(string[] args)
        {
            Console.WriteLine($"{s_field.a} {s_field.b}");
        }
    }

Expected Behavior:

class Program
    {
        static NewStruct s_field;

        static void Main(string[] args)
        {
            Console.WriteLine($"{s_field.A} {s_field.B}");
        }
    }

    internal struct NewStruct
    {
        public int A;
        public int B;

        public NewStruct(int a, int b)
        {
            A = a;
            B = b;
        }

        ...
    }

Actual Behavior:

class Program
    {
        static NewStruct s_field; 

        static void Main(string[] args)
        {
            Console.WriteLine($"{s_field.a} {s_field.b}");
        }
    }

    internal struct NewStruct
    {
        public int a;
        public int b;

        public NewStruct(int a, int b)
        {
            this.a = a;
            this.b = b;
        }

        ...
    }
dotnet-issue-labeler[bot] commented 3 years ago

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

CyrusNajmabadi commented 3 years ago

this was intentional. to match tuple semantics exactly, these are emitted as fields, not properties. Being fields, they are named appropriately. If there is a desire to change things to properties, then 'encapsulate field' can be used.

Note: i am amenable to 'convert to struct' having an option to 'use properties' instead of defaulting to fields (with the potential that that may break something).

tmat commented 3 years ago

Public fields should also be PascalCased.

CyrusNajmabadi commented 3 years ago

I'm not strongly opposed to that. If someone does implement htis, the main issue will be about how to make the naming change. We don't have any facility for bulk renaming. Doing that efficiently is non-trivial. one inefficient approach to doing this would be to:

  1. make the change to spit out hte struct as we do today.
  2. get SymbolKeys to all the fields in the new struct
  3. perform a series of renames against each field, using the symbolkey to find the old field in forked solution.

Because of hte cost of this, this should likely be an opt-in choice for people using this feature.

sharwell commented 3 years ago

Blocked on #14115