Skip to content

Handle error scenarios with duplicate required fields#62172

Merged
333fred merged 2 commits intodotnet:mainfrom
333fred:duplicate-properties
Jun 28, 2022
Merged

Handle error scenarios with duplicate required fields#62172
333fred merged 2 commits intodotnet:mainfrom
333fred:duplicate-properties

Conversation

@333fred
Copy link
Copy Markdown
Member

@333fred 333fred commented Jun 27, 2022

Fixes #62062.

@333fred 333fred requested a review from a team as a code owner June 27, 2022 23:56
@ghost ghost added the Area-Compilers label Jun 27, 2022
@333fred
Copy link
Copy Markdown
Member Author

333fred commented Jun 27, 2022

@jcouv @RikkiGibson for review.

jcouv
jcouv approved these changes Jun 28, 2022
View reviewed changes
Copy link
Copy Markdown
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)

@jcouv jcouv self-assigned this Jun 28, 2022
@333fred
Copy link
Copy Markdown
Member Author

333fred commented Jun 28, 2022

@RikkiGibson for the second review.

@RikkiGibson RikkiGibson self-assigned this Jun 28, 2022
@RikkiGibson
Copy link
Copy Markdown
Member

RikkiGibson commented Jun 28, 2022 •
edited
Loading

roslyn/src/Compilers/CSharp/Portable/Symbols/NamedTypeSymbol.cs

Lines 577 to 585 in c5baf96

private bool TryCalculateRequiredMembers(ref ImmutableSegmentedDictionary<string, Symbol>.Builder? requiredMembersBuilder)
{
var lazyRequiredMembers = _lazyRequiredMembers;
if (_lazyRequiredMembers == RequiredMembersErrorSentinel)
{
if (lazyRequiredMembers.IsDefault)
{
return false;
}

Unrelated, sorry, but was this read on line 580 meant to be lazyRequiredMembers instead of _lazyRequiredMembers? Or maybe lock-free multithreaded code is just weird 😄.

@333fred
Copy link
Copy Markdown
Member Author

333fred commented Jun 28, 2022

Unrelated, sorry, but was this read on line 580 meant to be lazyRequiredMembers instead of _lazyRequiredMembers?

You're correct, that should be lazyRequiredMembers.

333fred added a commit to 333fred/roslyn that referenced this pull request Jun 28, 2022
@333fred
Fix typo
6d4a017 
Fix typo pointed out by @RikkiGibson in dotnet#62172 (comment).
@333fred 333fred mentioned this pull request Jun 28, 2022
Merged
@333fred
Copy link
Copy Markdown
Member Author

333fred commented Jun 28, 2022

Submitted #62192 to fix that typo.

@333fred 333fred enabled auto-merge (squash) June 28, 2022 18:57
@333fred 333fred merged commit 0b79488 into dotnet:main Jun 28, 2022
@ghost ghost added this to the Next milestone Jun 28, 2022
@333fred 333fred deleted the duplicate-properties branch June 28, 2022 19:46
@RikkiGibson RikkiGibson modified the milestones: Next, 17.3 P3 Jun 28, 2022
333fred added a commit that referenced this pull request Jun 28, 2022
@333fred
Fix typo (#62192)
da7e855 
* Fix typo

Fix typo pointed out by @RikkiGibson in #62172 (comment).

* Fix logic as well.
@ryzngard ryzngard mentioned this pull request Jul 27, 2022
Merged
@333fred 333fred added the Feature - Required Members Required properties and fields label Sep 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@RikkiGibson RikkiGibson RikkiGibson approved these changes

@jcouv jcouv jcouv approved these changes

Assignees

@RikkiGibson RikkiGibson

@jcouv jcouv

Labels

Area-Compilers Feature - Required Members Required properties and fields

Projects

None yet

Milestone

17.3 P3

Development

Successfully merging this pull request may close these issues.

TryCalculateRequiredMembers throws exceptions if there's duplicate required members

3 participants

@333fred @RikkiGibson @jcouv