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

Update the required members spec #5566

Merged
merged 3 commits into from
Jan 4, 2022

Conversation

333fred
Copy link
Member

@333fred 333fred commented Dec 23, 2021

Adds some additional details on various topics, including OHI and metadata. Also adds new questions for LDM to address in a full session in early Jan.

Relates to test plan dotnet/roslyn#57046

Adds some additional details on various topics, including OHI and metadata. Also adds new questions for LDM to address in a full session in early Jan.
@333fred 333fred requested a review from a team as a code owner December 23, 2021 00:37
@333fred 333fred requested review from jcouv and RikkiGibson December 23, 2021 00:38
@333fred
Copy link
Member Author

333fred commented Dec 23, 2021

@jcouv @RikkiGibson for review.

333fred added a commit to 333fred/roslyn that referenced this pull request Dec 28, 2021
Adding `required` to a member now results in the type having a `RequiredMembersAttribute` emitted with the name of that member as the contents. Reading this data from metadata is not yet supported, nor is adding the requisite `ObsoleteAttribute` to constructors that depend on such contracts. The rules for when required is allowed and when it is disallowed are documented in dotnet/csharplang#5566, pending LDM review.

Test plan: dotnet#57046
}
```

It is an error to hide a `required` member, as that member can no longer be set by a consumer.
Copy link
Member

Choose a reason for hiding this comment

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

nit: I don't know if Mads or someone else has created a proposal for addressing members of specific types (something like x.(Type.Method)(argument)). If there's one, let's add a note there about this scenario.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

Done with review pass (iteration 1)

Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks (iteration 2)

…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 333fred merged commit 85212d8 into dotnet:main Jan 4, 2022
@333fred 333fred deleted the required-members-updates branch January 4, 2022 22:43
333fred added a commit to Youssef1313/csharplang that referenced this pull request Jan 12, 2022
* 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)
333fred added a commit to 333fred/roslyn that referenced this pull request Jan 20, 2022
Adding `required` to a member now results in the type having a `RequiredMembersAttribute` emitted with the name of that member as the contents. Reading this data from metadata is not yet supported, nor is adding the requisite `ObsoleteAttribute` to constructors that depend on such contracts. The rules for when required is allowed and when it is disallowed are documented in dotnet/csharplang#5566, pending LDM review.

Test plan: dotnet#57046
333fred added a commit to dotnet/roslyn that referenced this pull request Feb 4, 2022
Adding `required` to a member now results in the type having a `RequiredMemberAttribute` emitted on the member and the containing type. Reading this data from metadata is not yet supported, nor is adding the requisite `ObsoleteAttribute` to constructors that depend on such contracts. The rules for when required is allowed and when it is disallowed are documented in dotnet/csharplang#5566.

Test plan: #57046
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants