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.

Sign up for GitHub

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

Jump to bottom

Error when required members are not set #60101

Merged
merged 8 commits into from
Mar 17, 2022
Merged

Error when required members are not set #60101

merged 8 commits into from
Mar 17, 2022

Conversation

333fred
Copy link
Member

@333fred 333fred commented Mar 11, 2022

Implements reading the required members list of a type, and enforces that required members are all set by an initializer on the constructor. Required members must be initialized with values, not with nested object initializers.

Test plan #57046.
Specification https://github.com/dotnet/csharplang/blob/main/proposals/required-members.md

@333fred
Error when required members are not set
cf88693 
Implements reading the required members list of a type, and enforces that required members are all set by an initializer on the constructor. Required members must be initialized with values, not with nested object initializers.

Test plan dotnet#57046.
Specification https://github.com/dotnet/csharplang/blob/main/proposals/required-members.md
@333fred 333fred requested a review from a team as a code owner March 11, 2022 02:17
@333fred 333fred requested review from jcouv and RikkiGibson March 11, 2022 02:17
@333fred
Copy link
Member Author

333fred commented Mar 11, 2022

@jcouv @RikkiGibson please take a look.

@RikkiGibson RikkiGibson self-assigned this Mar 11, 2022
@333fred
Copy link
Member Author

333fred commented Mar 11, 2022 •
edited
Loading

As a general note, I scoured the codebase for other places where new BoundObjectCreation occurs, and everything else is invocations of the Nullable constructor or types we synthesize ourselves. I don't believe we care about Nullable<T> being defined with required members, so I didn't bother to insert checks elsewhere for it.

@333fred 333fred added the Feature - Required Members Required properties and fields label Mar 11, 2022
@333fred
PR Feedback:
740c4ed 
* Support checking attribute constructors
* Suppress some duplicate diagnostics
* Minor other feedback.
@333fred
Copy link
Member Author

333fred commented Mar 11, 2022

@RikkiGibson addressed your feedback. Please take another look.

@333fred
Copy link
Member Author

333fred commented Mar 11, 2022

@jcouv for a second review.

jcouv
jcouv reviewed Mar 15, 2022
View reviewed changes
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 2). Will look at tests this afternoon

333fred added 2 commits March 15, 2022 12:25
@333fred
Address PR feedback.
2ca360a
@333fred
Merge remote-tracking branch 'upstream/features/required-members' int…
07c1413 
…o enforce-setting

* upstream/features/required-members: (78 commits)
  [main] Update dependencies from dotnet/roslyn (dotnet#59792)
  Annotate BreakpointSpans and fix NREs (dotnet#59846)
  Editor namespaces refactoring - part 1 (dotnet#59907)
  [main] Update dependencies from dotnet/source-build-externals (dotnet#59845)
  Update PublishData.json (dotnet#59952)
  ...and the comment
  Revert the last change in the compiler layer
  Force SVsExtensionManager to load before InProcComponent creation
  Handle nameof situation, moved logic back to workspaces layer
  Add back OmniSharpInlineHints (dotnet#59941)
  Initialize AsyncCompletionTracker in a fire-and-forget manner
  Try to load the extension manager from the main thread as well
  Use the correct GUID for SVsUserNotificationsService
  Update CodeStyle/format/SDK versions (dotnet#59903)
  Create attribute default arguments during binding (dotnet#59750)
  Undo refactoring part 2
  Undo refactoring
  Move logic of symbol filtering to compiling layer. Refactoring
  Clean up diagnostic and solution crawler options - take 2 (dotnet#59233)
  Add quotes in string for clarity
  ...
@jcouv jcouv self-assigned this Mar 16, 2022
333fred added 2 commits March 16, 2022 16:21
@333fred
Add ability for ImmutableSegmentedDictionary to have sentinel values.
31157e3 
Currently, ImmutableSegmentedDictionary cannot be used as a Sentinel value, because there is no way to make a sentinel that is different from Empty or default. This adds the ability to make a new empty dictionary, as well as a way to compare two ImmutableSegmentedDictionaries for reference equality.
@333fred
PR Feedback.
aa419fb
@333fred
Copy link
Member Author

333fred commented Mar 16, 2022

@jcouv addressed feedback

@sharwell please review 31157e3 for changes to ImmutableSegmentedDictionary.

jcouv
jcouv approved these changes Mar 17, 2022
View reviewed changes
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 6) with one test to consider

jcouv
jcouv approved these changes Mar 17, 2022
View reviewed changes
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 7)

@333fred 333fred enabled auto-merge (squash) March 17, 2022 18:22
@333fred 333fred merged commit 47c81fe into dotnet:features/required-members Mar 17, 2022
@333fred 333fred deleted the enforce-setting branch March 17, 2022 22:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Youssef1313 Youssef1313 Youssef1313 left review comments

@AlekseyTs AlekseyTs AlekseyTs left review comments

@sharwell sharwell sharwell left review comments

@jcouv jcouv jcouv approved these changes

@RikkiGibson RikkiGibson RikkiGibson approved these changes

Assignees

@RikkiGibson RikkiGibson

@jcouv jcouv

Labels
Area-Compilers Feature - Required Members Required properties and fields
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@333fred @sharwell @RikkiGibson @AlekseyTs @jcouv @Youssef1313