Skip to content

Support SetsRequiredMembersAttribute#60392

Merged
333fred merged 14 commits intodotnet:features/required-membersfrom
333fred:setsrequiredmembers
Apr 15, 2022
Merged

Support SetsRequiredMembersAttribute#60392
333fred merged 14 commits intodotnet:features/required-membersfrom
333fred:setsrequiredmembers

Conversation

@333fred
Copy link
Copy Markdown
Member

@333fred 333fred commented Mar 25, 2022

The SetsRequiredMembersAttribute prevents the compiler from checking the required member list of a type when calling that constructor, and suppresses any errors from a base type's list being invalid.

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

@333fred 333fred requested a review from a team as a code owner March 25, 2022 19:33
@ghost ghost added the Area-Compilers label Mar 25, 2022
@333fred 333fred requested review from RikkiGibson and jcouv March 25, 2022 19:33
@333fred
Copy link
Copy Markdown
Member Author

333fred commented Mar 25, 2022

@RikkiGibson @jcouv please take a look.

@333fred
Support SetsRequiredMembersAttribute
7761d6b 
The SetsRequiredMembersAttribute prevents the compiler from checking the required member list of a type when calling that constructor, and suppresses any errors from a base type's list being invalid.

Specification: https://github.com/dotnet/csharplang/blob/main/proposals/required-members.md
Test plan: dotnet#57046
@333fred 333fred force-pushed the setsrequiredmembers branch from 3d01290 to 7761d6b Compare March 25, 2022 19:41
@RikkiGibson RikkiGibson self-assigned this Mar 25, 2022
@333fred
Copy link
Copy Markdown
Member Author

333fred commented Mar 28, 2022

@jcouv @RikkiGibson please take a look

@runfoapp runfoapp bot mentioned this pull request Mar 29, 2022
Open
RikkiGibson
RikkiGibson reviewed Mar 29, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@RikkiGibson RikkiGibson left a comment

Choose a reason for hiding this comment

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

Just had a few comments so far. Haven't looked at the tests yet--possible those will hold answers to some of the questions anyway. Will review the tests a little later this afternoon.

@333fred
Copy link
Copy Markdown
Member Author

333fred commented Mar 29, 2022

@RikkiGibson @jcouv please take a look.

RikkiGibson
RikkiGibson approved these changes Mar 29, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@RikkiGibson RikkiGibson left a comment

Choose a reason for hiding this comment

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

LGTM aside from some minor comments.

@333fred
Copy link
Copy Markdown
Member Author

333fred commented Mar 29, 2022

@jcouv for the second review please.

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

@jcouv
Copy link
Copy Markdown
Member

jcouv commented Mar 30, 2022 •
edited
Loading

Just curious, what was the problem you ran into that the used assemblies leg detected?

In reply to: 1083559918

@333fred
Copy link
Copy Markdown
Member Author

333fred commented Mar 30, 2022

Just curious, what was the problem you ran into that the used assemblies leg detected?

d9da29b. It wasn't happy about mismatched collection of dependencies.

@333fred
Copy link
Copy Markdown
Member Author

333fred commented Apr 6, 2022

@RikkiGibson @jcouv please take another look.

RikkiGibson
RikkiGibson reviewed Apr 6, 2022
View reviewed changes
@jcouv jcouv requested a review from RikkiGibson April 7, 2022 18:30
@RikkiGibson
Copy link
Copy Markdown
Member

RikkiGibson commented Apr 7, 2022 •
edited
Loading

I was contemplating whether it's reasonable for constructors to delegate to 'SetRequiredMembers' constructors, without being 'SetsRequiredMembers' themselves. For example, test RequiredMemberSuppressesNullabilityWarnings_ChainedConstructor_08.

class Base
{
    public required string Prop1 { get; set; }

    [SetsRequiredMembers]
    public Base() { Prop1 = "a"; }
    
    public Base(int unused) { }
}

class Derived : Base
{
    public required string Prop2 { get; set; }

    // maybe compiler needs to emit [SetsRequiredMembersOf(typeof(Base))] here?
    public Derived() : base() { }

    // maybe compiler needs to emit [SetsRequiredMembersOf(typeof(Base))] here?
    public Derived(int unused) : this() { }
}

class Program
{
    static void Main()
    {
        // how do we realize the user doesn't need to set Prop1?
        var derived = new Derived() { Prop2 = "a" };
        
        // we seem even more at sea here :)
        var derived2 = new Derived(1) { Prop2 = "a" };
    }
}

The metadata encoding spec doesn't appear to be able to represent this as written. Did I miss something?

I could imagine complex type hierarchies peppering in 'SetsRequiredMembers' at arbitrary levels. It feels like representing that accurately would require tracking the most-derived type in the hierarchy which has the attribute. When the compiler binds a constructor it can determine which base constructor it ultimately delegates to, perhaps using a similar mechanism as is currently used to prevent cycles in constructor initializers.

However, it feels to me like it would be a very reasonable starting place to require any constructors which delegate to a 'SetsRequiredMembers' constructor to also have 'SetsRequiredMembers'. This would make it an all-or-nothing thing down the hierarchy to start with, and I think it leaves the space open for us to add something like what I described above later on.

@333fred
Copy link
Copy Markdown
Member Author

333fred commented Apr 12, 2022

@RikkiGibson we could do that. My initial thought was, however, not to do any form of restrictions. In short, in your example, there is no way to know that Prop2 should not be set from the derived constructors. I'm ok with considering a simplification of this, however, as that would significantly reduce the complexity of the changes in NullableWalker here.

333fred added 2 commits April 12, 2022 13:27
@333fred
Merge remote-tracking branch 'upstream/features/required-members' int…
06acf5f 
…o setsrequiredmembers

* upstream/features/required-members: (808 commits)
  Update for definite assignment changes
  Remove duplicate package references (dotnet#60658)
  Formatting and code generation options (dotnet#60127)
  Trim unnessasary leading lines when removing usings (dotnet#60672)
  Pass options to FixAllAsync, simplify CodeAction registration (dotnet#60665)
  Fallout
  Lint
  Restore CodeStyle test projects
  Update struct field definite assignment tests
  Global indentation options - take 2 (dotnet#60565)
  Run continuation to dispose of cancellation token source (dotnet#60653)
  Fixup
  Update tests
  Cleanup
  Cleanup
  move properties
  Delay starting the work to scan for todo-items
  Simplify
  Clean up syntax context
  Clean up syntax context
  ...
@333fred
Add restriction that SetsRequiredMembersAttribute must be applied whe…
4cfdf57 
…n chaining to a constructor with the attribute. Add more tests.
@333fred
Copy link
Copy Markdown
Member Author

333fred commented Apr 14, 2022

@RikkiGibson @jcouv please take another look.

jcouv
jcouv reviewed Apr 15, 2022
View reviewed changes
jcouv
jcouv reviewed Apr 15, 2022
View reviewed changes
jcouv
jcouv approved these changes Apr 15, 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 12)

@jcouv jcouv self-assigned this Apr 15, 2022
333fred added 2 commits April 14, 2022 17:41
@333fred
Merge remote-tracking branch 'upstream/features/required-members' int…
728b6da 
…o setsrequiredmembers

* upstream/features/required-members: (66 commits)
  Fix dotnet#55183: Add SymbolVisitor<TArgument, TResult> (dotnet#56530)
  Simplifier options (dotnet#60174)
  Remove duplicated asset
  Do not try to refcount solution syncing when communicating with OOP
  Delay symbol-search index updating until solution is fully loaded.
  add more miscellaneous tests for checked operators (dotnet#60727)
  Support checked operators in explicit interface implementation (dotnet#60715)
  Avoid formatting diagnostics with raw strings (dotnet#60655)
  Make heading levels for warning waves documentation consistent (dotnet#60721)
  Clean up IDiagnosticService extension methods
  Remove #nullable enable
  Add integration test to flag MEF composition breaks
  Generate static abstract interface members correctly (dotnet#60618)
  Merge release/dev17.2 to main (dotnet#60682)
  Fix FAR on checked operators (dotnet#60698)
  Add implement interface support for checked operators and cast operators (dotnet#60719)
  Update csc.dll path in launch.json (dotnet#60663)
  Grab bag of UTF8 string support in IDE features (dotnet#60599)
  Allow code actions to retrieve options for any language (dotnet#60697)
  Fix flaky VSTypeScriptHandlerTests  (dotnet#60706)
  ...
@333fred
PR Feedback.
106378e
RikkiGibson
RikkiGibson reviewed Apr 15, 2022
View reviewed changes
src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs
}
else if (initializerKind == (int)SyntaxKind.BaseConstructorInitializer)
{
// If we chained to a `base` constructor, a SetsRequiredMembers attribute applies to the base type's required members only, and the current type's required members
Copy link
Copy Markdown
Member

@RikkiGibson RikkiGibson Apr 15, 2022

Choose a reason for hiding this comment

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

It seems like the code paths which handle SetsRequiredMembers on base but not this could be eliminated.

Copy link
Copy Markdown
Member Author

@333fred 333fred Apr 15, 2022

Choose a reason for hiding this comment

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

I'm not sure what you mean by this? Can you elaborate?

Copy link
Copy Markdown
Member

@RikkiGibson RikkiGibson Apr 15, 2022

Choose a reason for hiding this comment

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

I was wrong about this. I recall now that you're still expected to get a different initial state when the 'this()' constructor you chain to has SetsRequiredMembers, versus the 'base()' constructor you chain to having it.

For some reason I expected a further simplification in NullableWalker to fall out of the decision to require SetsRequiredMembers to be used on constructors which chaining to a SetsRequiredMembers constructor.

RikkiGibson
RikkiGibson approved these changes Apr 15, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@RikkiGibson RikkiGibson left a comment

Choose a reason for hiding this comment

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

LGTM with one comment which can be addressed in a subsequent PR if you prefer.

@333fred 333fred merged commit 53ce8d4 into dotnet:features/required-members Apr 15, 2022
@333fred 333fred deleted the setsrequiredmembers branch April 15, 2022 22:07
@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

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@333fred @jcouv @RikkiGibson