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

Support declaration and emit of required members in source #58507

Merged
merged 11 commits into from
Feb 4, 2022
Merged

Support declaration and emit of required members in source #58507

merged 11 commits into from
Feb 4, 2022

Conversation

333fred
Copy link
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

@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 jcouv and RikkiGibson January 27, 2022 00:28
@333fred
Copy link
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
src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs Outdated Show resolved Hide resolved
...Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol_ImplementationChecks.cs Outdated Show resolved Hide resolved
src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedContainer.cs Outdated Show resolved Hide resolved
src/Compilers/CSharp/Portable/Symbols/Symbol.cs Outdated Show resolved Hide resolved
src/Compilers/CSharp/Portable/Symbols/SymbolExtensions.cs Outdated Show resolved Hide resolved
src/Compilers/CSharp/Test/Symbol/Symbols/RequiredMembersTests.cs Show resolved Hide resolved
src/Compilers/CSharp/Test/Symbol/Symbols/RequiredMembersTests.cs Show resolved Hide resolved
src/Compilers/CSharp/Test/Symbol/Symbols/RequiredMembersTests.cs Outdated Show resolved Hide resolved
src/Compilers/CSharp/Test/Symbol/Symbols/RequiredMembersTests.cs Show resolved Hide resolved
src/ExpressionEvaluator/CSharp/Source/ExpressionCompiler/Symbols/DisplayClassVariable.cs Show resolved Hide resolved
@runfoapp runfoapp bot mentioned this pull request Jan 27, 2022
Closed
@333fred
Copy link
Member Author

333fred commented Jan 28, 2022

@RikkiGibson please take another look.

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

333fred commented Jan 31, 2022

@jcouv for a second review.

jcouv
jcouv reviewed Feb 2, 2022
View reviewed changes
src/Compilers/CSharp/Portable/Symbols/SubstitutedNamedTypeSymbol.cs
@@ -204,6 +204,8 @@ public sealed override ImmutableArray<NamedTypeSymbol> GetTypeMembers(string nam
return OriginalDefinition.GetTypeMembers(name, arity).SelectAsArray((t, self) => t.AsMember(self), this);
}

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

Choose a reason for hiding this comment

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

Unreachable?

Copy link
Member Author

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
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 8)

@333fred
Copy link
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
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