Skip to content

Support declaration and emit of required members in source#58507

Merged
333fred merged 11 commits intodotnet:features/required-membersfrom
333fred:declaration-errors
Feb 4, 2022
Merged

Support declaration and emit of required members in source#58507
333fred merged 11 commits intodotnet:features/required-membersfrom
333fred:declaration-errors

Conversation

@333fred
Copy link
Copy Markdown
Member

@333fred 333fred commented Dec 28, 2021 •
edited
Loading

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.

Test plan: #57046

@ghost ghost added the Area-Compilers label Dec 28, 2021
@333fred
Report errors on invalid required declarations.
7238484
@333fred
Add support for emitting RequiredMembersAttribute in source
e69a49e 
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 333fred force-pushed the declaration-errors branch from 2318cb4 to e69a49e Compare January 20, 2022 19:21
@333fred 333fred added the Feature - Required Members Required properties and fields label Jan 27, 2022
@333fred 333fred changed the title declaration errors Support declaration and emit of required members in source Jan 27, 2022
@333fred 333fred marked this pull request as ready for review January 27, 2022 00:28
@333fred 333fred requested a review from a team as a code owner January 27, 2022 00:28
@333fred 333fred requested review from RikkiGibson and jcouv January 27, 2022 00:28
@333fred
Copy link
Copy Markdown
Member Author

333fred commented Jan 27, 2022

@jcouv @RikkiGibson for review.

@RikkiGibson RikkiGibson self-assigned this Jan 27, 2022
RikkiGibson
RikkiGibson reviewed Jan 27, 2022
View reviewed changes
@runfoapp runfoapp bot mentioned this pull request Jan 27, 2022
Closed
@333fred
Copy link
Copy Markdown
Member Author

333fred commented Jan 28, 2022

@RikkiGibson please take another look.

@jcouv jcouv self-assigned this Jan 29, 2022
@333fred
Copy link
Copy Markdown
Member Author

333fred commented Jan 31, 2022

@jcouv for a second review.

jcouv
jcouv reviewed Feb 2, 2022
View reviewed changes
jcouv
jcouv reviewed Feb 2, 2022
View reviewed changes
jcouv
jcouv reviewed Feb 2, 2022
View reviewed changes
src/Compilers/CSharp/Portable/Symbols/SubstitutedNamedTypeSymbol.cs
return OriginalDefinition.GetTypeMembers(name, arity).SelectAsArray((t, self) => t.AsMember(self), this);
}

internal sealed override bool HasDeclaredRequiredMembers => OriginalDefinition.HasDeclaredRequiredMembers;
Copy link
Copy Markdown
Member

@jcouv jcouv Feb 2, 2022

Choose a reason for hiding this comment

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

Unreachable?

Copy link
Copy Markdown
Member Author

@333fred 333fred Feb 3, 2022

Choose a reason for hiding this comment

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

No?

jcouv
jcouv reviewed Feb 2, 2022
View reviewed changes
jcouv
jcouv reviewed Feb 2, 2022
View reviewed changes
jcouv
jcouv reviewed Feb 2, 2022
View reviewed changes
jcouv
jcouv reviewed Feb 2, 2022
View reviewed changes
jcouv
jcouv reviewed Feb 2, 2022
View reviewed changes
jcouv
jcouv reviewed Feb 2, 2022
View reviewed changes
jcouv
jcouv reviewed Feb 2, 2022
View reviewed changes
jcouv
jcouv reviewed Feb 2, 2022
View reviewed changes
jcouv
jcouv reviewed Feb 2, 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.

Done with review pass (iteration 8)

@333fred
Copy link
Copy Markdown
Member Author

333fred commented Feb 3, 2022

@jcouv please take another look

jcouv
jcouv approved these changes Feb 4, 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 10)

@runfoapp runfoapp bot mentioned this pull request Feb 4, 2022
Closed
@333fred 333fred enabled auto-merge (squash) February 4, 2022 18:27
@333fred 333fred merged commit 3004d67 into dotnet:features/required-members Feb 4, 2022
@333fred 333fred deleted the declaration-errors branch February 4, 2022 19:54
jcouv referenced this pull request Feb 11, 2022
@chsienki
Add type IL to verify attribute is actually emitted
4650f06
@333fred 333fred mentioned this pull request Mar 11, 2022
Closed
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

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@333fred @RikkiGibson @jcouv