Skip to content

Conversation

@Soreloser2
Copy link
Contributor

Extension of #63320

Completion list copied with new items bolded/italicized:

  • Implement quick action with positional parameters
    • Move Properties to positional parameters
      • With modified Attributes
      • With modified documentation comments
        • Single line
        • Multi line (too hard/not much support from syntax generators 😢)
      • Structs and readonly structs
      • Tests
    • Analyzing statements to delete members
      • Constructor conflicting with primary constructor
      • Equality operator overloads
      • Equals method(s)
      • HashCode method
      • Clone method
      • ToString (optional? Need design discussion)
      • Copy constructor
      • Tests
    • Analyzing declarations to modify members (mostly accessibility stuff)
      • EqualityContract, PrintMembers
        • not really necessary
      • Non-primary constructors (analyze and provide values instead of this(default, default...))
      • Modify order of positional params based on primary constructor order
      • Supply default values to primary constructor from removed primary constructor
      • Tests
    • Inheritance
      • Basic (copy previous list)
      • If base is record w/ positional params, give defaults
      • Modify members if they have a base initializer already
      • Tests (not for base handling)
    • Refactoring other locations
      • Turn property constructor into positional params
      • Tests
    • Minimum/Maximum # of props to move to positional params
      • Need design discussion (Is it reasonable to move 1 property or more than like 10?)
      • Tests
  • Implement quick action w/o positional parameters
    • Abstract and modify analysis portion so we can use same code to delete things
    • Allow positional records to convert to non-positional and vice versa
      • Documentation comments
      • Attributes
      • This initializer
      • Refactoring (convert back to property initializer)
    • Tests

Soren Dahl added 30 commits July 5, 2022 16:57
Add a couple more params to the new record type
- Removed some extra spaces
- Added generic and type constraint tests
- Edited some comments for clarity
- Added check for != operator content (+ tests)
- Moved Resource strings to CSharp features resources
- Reworked a default to make it null instead, then null check
- Wrapped some long lines
- Made sealed
- Copied Base Type, still need tests
Just Clone and PrintMembers currently
Allow readonly props to move
Changed equalization string
Added Selection tests
Removed Normalize whitespace
Formatting fixes
refactor no-body case
add and clarify comments
Some tests failing
@Soreloser2 Soreloser2 requested a review from a team as a code owner August 12, 2022 23:43
@Soreloser2 Soreloser2 mentioned this pull request Aug 12, 2022
49 tasks
@Soreloser2 Soreloser2 changed the base branch from features/ConvertToRecord to main August 19, 2022 16:56
@Soreloser2 Soreloser2 changed the base branch from main to features/ConvertToRecord August 19, 2022 16:56
=> (invocation.Instance == null ||
IsSafeAssignment(invocation.Instance)) &&
invocation.Arguments.All(arg => IsSafeAssignment(arg.Value)),
IUnaryOperation unary => IsSafeAssignment(unary.Operand),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have a generic catch-all that just walks the child operations without having to special case all of these?

private static bool IsSafeAssignment(IOperation operation)
=> operation.WalkDownConversion() switch
{
if (operation is ILocalReferenceOperation)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't still need to check for IMemberReferenceOperation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right, I forgot about the case where we access static members/methods. Any other member reference or invocation should have the instance and the member as ChildOperations so if a member reference of a local we should still hit it here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My main concern was whether a this reference counted as a local reference or as something else.

@Soreloser2 Soreloser2 merged commit 0678cc8 into dotnet:features/ConvertToRecord Aug 22, 2022
@Soreloser2 Soreloser2 deleted the ConvertToRecordPrs/ConstructorOverloads branch August 22, 2022 22:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants