Skip to content

Spec change around backing field being readonly#5575

Merged
CyrusNajmabadi merged 2 commits intodotnet:mainfrom
Youssef1313:patch-6
Dec 27, 2021
Merged

Spec change around backing field being readonly#5575
CyrusNajmabadi merged 2 commits intodotnet:mainfrom
Youssef1313:patch-6

Conversation

@Youssef1313
Copy link
Copy Markdown
Member

@Youssef1313 Youssef1313 commented Dec 25, 2021 •
edited
Loading

@Youssef1313 Youssef1313 requested a review from a team as a code owner December 25, 2021 21:44
Youssef1313
Youssef1313 commented Dec 25, 2021
View reviewed changes
proposals/semi-auto-properties.md Outdated

```diff
- If the auto-property has no set accessor, the backing field is considered `readonly` ([Readonly fields](classes.md#readonly-fields)). Just like a `readonly` field, a getter-only auto-property can also be assigned to in the body of a constructor of the enclosing class. Such an assignment assigns directly to the readonly backing field of the property.
+ If the auto-property has semicolon-only get accessor without a set accessor, the backing field is considered `readonly` ([Readonly fields](classes.md#readonly-fields)). Just like a `readonly` field, a semicolon-getter-only auto-property can also be assigned to in the body of a constructor of the enclosing class. Such an assignment assigns directly to the readonly backing field of the property.
Copy link
Copy Markdown
Member Author

@Youssef1313 Youssef1313 Dec 25, 2021

Choose a reason for hiding this comment

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

I'm not sure if "semicolon-getter-only" is clear enough.

Copy link
Copy Markdown
Contributor

@jnm2 jnm2 Dec 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

It is to me, or it could be an auto property which only has a semicolon getter. I'd nitpick:

  • has a semicolon-only get accessor
  • auto property not auto-property (I know, this was original). Hyphenation is for when the two words are acting in combination as a modifier (adjective) of some other noun, but here, 'property' is the noun.

Copy link
Copy Markdown
Contributor

@jnm2 jnm2 Dec 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I wonder if the spec should say something about getter-only properties being assignable in the body of a constructor even when the getter is not semicolon-only and therefore the backing field is not readonly. Example:

class C
{
    public C(string? p) { P = p; }
    
    public string P => field ??= CalculateP();
    
    private string CalculateP() => throw new NotImplementedException();
}

Copy link
Copy Markdown
Member Author

@Youssef1313 Youssef1313 Dec 25, 2021

Choose a reason for hiding this comment

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

Yup I just noticed an overlap and a possibility to divide this into two separate sentences depending on what the initializer behavior would be. Let me mark as a draft for now as it doesn't seem to be accurate.

Copy link
Copy Markdown
Member Author

@Youssef1313 Youssef1313 Dec 26, 2021

Choose a reason for hiding this comment

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

I think the wording is more accurate now?

Copy link
Copy Markdown
Contributor

@jnm2 jnm2 Dec 27, 2021

Choose a reason for hiding this comment

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

Lgtm!

@Youssef1313 Youssef1313 mentioned this pull request Dec 25, 2021
Merged
@Youssef1313 Youssef1313 marked this pull request as draft December 25, 2021 22:05
@Youssef1313 Youssef1313 marked this pull request as ready for review December 26, 2021 06:33
@CyrusNajmabadi CyrusNajmabadi merged commit 30582c1 into dotnet:main Dec 27, 2021
@CyrusNajmabadi
Copy link
Copy Markdown
Contributor

CyrusNajmabadi commented Dec 27, 2021

thanks! :)

@RikkiGibson
Copy link
Copy Markdown
Member

RikkiGibson commented Dec 27, 2021

It feels like if you had a get; init => field = Validate(value); property, you might expect that:

  1. Backing field is readonly
  2. Init accessor will be called when property is assigned in constructor

Does the spec account for this as written?

@Youssef1313 Youssef1313 deleted the patch-6 branch December 27, 2021 19:39
@Youssef1313
Copy link
Copy Markdown
Member Author

Youssef1313 commented Dec 27, 2021

The spec doesn't account for that currently. Let me open a new PR for that.

@Youssef1313 Youssef1313 mentioned this pull request Dec 27, 2021
Merged
333fred added a commit to 333fred/csharplang that referenced this pull request Jan 4, 2022
@333fred
Merge remote-tracking branch 'upstream/main' into required-members-up…
2f81e67 
…dates

* upstream/main:
  Add example of when field initializers will not be run for option 3.
  Correct link
  Added LDM notes for January 3rd, 2022.
  Update README.md
  Update LDM agenda
  Fix line wrapping (dotnet#5589)
  Update raw-string-literal.md
  'record struct' constructor requires 'this' initializer that calls primary constructor or explicit constructor (dotnet#5562)
  Tweak readonly backing field wording (dotnet#5583)
  Update semi auto props spec around readonly backing field for init-only (dotnet#5582)
  Spec change around backing field being readonly (dotnet#5575)
333fred added a commit to Youssef1313/csharplang that referenced this pull request Jan 12, 2022
@333fred
Merge remote-tracking branch 'upstream/main' into patch-5
21f90e1 
* upstream/main:
  Update LDM agenda
  Added LDM Notes for January 5th, 2022.
  Update the required members spec (dotnet#5566)
  Add example of when field initializers will not be run for option 3.
  Correct link
  Added LDM notes for January 3rd, 2022.
  Update README.md
  Update LDM agenda
  Fix line wrapping (dotnet#5589)
  Update raw-string-literal.md
  'record struct' constructor requires 'this' initializer that calls primary constructor or explicit constructor (dotnet#5562)
  Tweak readonly backing field wording (dotnet#5583)
  Update semi auto props spec around readonly backing field for init-only (dotnet#5582)
  Spec change around backing field being readonly (dotnet#5575)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jnm2 jnm2 jnm2 left review comments

@333fred 333fred 333fred approved these changes

+1 more reviewer

@CyrusNajmabadi CyrusNajmabadi CyrusNajmabadi approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@Youssef1313 @CyrusNajmabadi @RikkiGibson @333fred @jnm2