Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename tracking doesn't work on positional members #44560

Closed
jcouv opened this issue May 26, 2020 · 7 comments
Closed

Rename tracking doesn't work on positional members #44560

jcouv opened this issue May 26, 2020 · 7 comments
Assignees
Labels
Area-IDE Concept-Design Debt Engineering Debt, Design Debt, or poor product code quality Feature - Records Records Feature - Rename
Milestone

Comments

@jcouv
Copy link
Member

jcouv commented May 26, 2020

image

Scenario 1 (trigger rename from parameter list in declaration):

record Person(string First, string Second); // change `Second` to `Renamed`

// corresponding synthesized code
class Person
{
     public Person(string First, string Second) { this.First = First; this.Second = Second; } // synthesized primary constructor
     string First { get; init; } 
     string Second { get; init; } 
     ...
}

Expected:

Person p = new Person(First: "", Second: ""); // `Second` should be renamed here (works!)
_ = p.Second; // property `Second` should be renamed too

record Student(string First, string Second, int Id) : Base(First: First, Second: Second); // `Second:` should renamed too, like `Student.Second` should be renamed as well (same property, but good to confirm with IDE design)

Scenario 2 (trigger rename from parameter list in declaration, with user-defined property):

record Person(string First, string Second) // `Second` here is just a parameter, no longer refers to property
{
    public string Second { get { ... } init { ... } } = Second /* parameter */;
}

Expected:

// same changes as in scenario 1
// we should rename the user-defined property as well (need to confirm with IDE design review)

Scenario 3 (trigger from usage site):

Person p = new Person(First: "", Second: ""); // trigger the rename from `Second:`
_ = p.Second; // trigger the rename from usage of the property
record Person(string First, string Second)
{
    public string Second { get { ... } init { ... } } = Second /* parameter */; // trigger rename from property declaration
}

Misc

How do link parameter and property symbols?

  1. add a compiler API (for example .AssociateSymbol, problematic because of language design and metadata)
  2. do it manually (syntax shows that we have "record", you can tell which constructor is primary, allows us to find corresponding properties)

Relates to #40726 (test plan for "records")

@jinujoseph jinujoseph added the Concept-Design Debt Engineering Debt, Design Debt, or poor product code quality label May 27, 2020
@jinujoseph jinujoseph added this to the 16.7 milestone May 27, 2020
@jinujoseph
Copy link
Contributor

cc @ryzngard

@ryzngard ryzngard self-assigned this May 27, 2020
@jcouv
Copy link
Member Author

jcouv commented Jun 6, 2020

@ryzngard I suspect we'll need to fix FAR as part of fixing this. Issue for FAR: #44559 (need to cascade various record symbols together)
When you're ready to look at this, ping me and I'll walk you through the records concepts.

@TahirAhmadov
Copy link

@jcouv the linked #44559 was fixed in February 2022, but unfortunately this is still open...

@jcouv
Copy link
Member Author

jcouv commented Jan 16, 2024

@TahirAhmadov I don't think this issue was fixed yet.
I just tested the following scenario and it still behaves as described in OP.

record Person(string First, string Second); // change `Second` to `Renamed`
Person p = new Person(First: "", Second: ""); // `Second` should be renamed here (works!)
_ = p.Second; // property `Second` should be renamed too (but isn't)

@ryzngard ryzngard removed their assignment Jan 16, 2024
@ryzngard
Copy link
Contributor

Removing myself since I'm no longer on the team (and don't have dedicated time to fix this issue). @Cosifne heads up :) I'll let you decide who this should be assigned to

@TahirAhmadov
Copy link

@jcouv correct, this issue has not been fixed; I was referring to the dependency issue #44559.

But more importantly I misdirected my "follow up" at you, the reporter, and not @ryzngard, who had been the assignee up until an hour ago. :)

@jcouv jcouv added the untriaged Issues and PRs which have not yet been triaged by a lead label Jan 17, 2024
@Cosifne Cosifne self-assigned this Jul 3, 2024
@Cosifne Cosifne removed the untriaged Issues and PRs which have not yet been triaged by a lead label Jul 3, 2024
@Cosifne
Copy link
Member

Cosifne commented Jul 8, 2024

I tested the build after #74168
All secnario are working

@Cosifne Cosifne closed this as completed Jul 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE Concept-Design Debt Engineering Debt, Design Debt, or poor product code quality Feature - Records Records Feature - Rename
Projects
None yet
Development

No branches or pull requests

6 participants