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.

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

Create partial-properties.md #8065

Merged
merged 5 commits into from
Apr 25, 2024
Merged

Create partial-properties.md #8065

merged 5 commits into from
Apr 25, 2024

Conversation

RikkiGibson
Copy link
Contributor

@RikkiGibson RikkiGibson commented Apr 19, 2024

@jcouv

The goal is to migrate the proposal details in #6420 nearer to its "final" location.

@RikkiGibson RikkiGibson requested a review from a team as a code owner April 19, 2024 23:24
proposals/partial-properties.md Show resolved Hide resolved
### Defining and implementing declarations
When a property declaration includes a *partial* modifier, that property is said to be a *partial property*. Partial properties may only be declared as members of partial types.

A *partial property* declaration is said to be a *defining declaration* when its accessors all have semicolon bodies, and it lacks the `extern` modifier. Otherwise, it is an *implementing declaration*. A *partial property* cannot be an auto-property.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
A *partial property* declaration is said to be a *defining declaration* when its accessors all have semicolon bodies, and it lacks the `extern` modifier. Otherwise, it is an *implementing declaration*. A *partial property* cannot be an auto-property.
A *partial property* declaration is said to be a *defining declaration* when its accessors all have semicolon bodies, and it lacks the `extern` modifier. Otherwise, it is an *implementing declaration*. A *partial property* definition cannot be an auto-property.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is correct as written. Neither a defining nor implementing declaration of a partial property will cause it to be an auto-property. I'll try and break this out to improve clarity.

### Matching signatures
The property declarations must have the same type and ref kind. The property declarations must have the same set of accessors.

The property declarations and their accessor declarations must have the same modifiers, though the modifiers may appear in a different order. This does not apply to the `extern` modifier, which may only appear on an *implementing declaration*.
Copy link
Member

Choose a reason for hiding this comment

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

The required modifier is a new consideration as it can't apply to partial methods. Does that pose any particular challenges here or does it just fall out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think usage of required partial together doesn't raise new questions. It will be supported, and have the same effect as if the partial modifier was removed and the defining declaration was merged into the implementing declaration.

```diff
property_declaration
- : attributes? property_modifier* type member_name property_body
+ : attributes? property_modifier* 'partial'? type member_name property_body
Copy link
Member

@jcouv jcouv Apr 23, 2024

Choose a reason for hiding this comment

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

  1. We'll probably want to confirm whether partial should still be in locked position for new features (that's something we considered relaxing for existing features)
  2. The structure for method_header is a bit different (partial is not inlined in the declaration, but rather inside the method_modifiers production). Should we spec it the same way for partial properties?
method_declaration
    : attributes? method_modifiers return_type method_header method_body
    | attributes? ref_method_modifiers ref_kind ref_return_type method_header
      ref_method_body
    ;

method_modifiers
    : method_modifier* 'partial'?
    ;
``` #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would like to address (1) in a subsequent PR, and to do so for both properties and methods in C# 13. I recall soft approval from LDM for this, and at the time I make the change I'll also follow up by email to confirm that approval.

I would prefer to not make changes for (2) at this time, as I think the spec is conveying what it is intending to with adequate clarity, and that we'll address the concern before the spec is committed to its final state for C# 13.

### Matching signatures
The property declarations must have the same type and ref kind. The property declarations must have the same set of accessors.

The property declarations and their accessor declarations must have the same modifiers, though the modifiers may appear in a different order. This does not apply to the `extern` modifier, which may only appear on an *implementing declaration*.
Copy link
Member

Choose a reason for hiding this comment

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

FWIW, it looks like partial methods allow some variation in scoped (see usage of CheckValidScopedOverride in PartialMethodChecks)
Some variation in tuple names is also allowed.

Copy link
Member

Choose a reason for hiding this comment

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

Were you able to dig up our rules for scoped differences?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Scoped is just a modifier, I am expecting the normal rules for modifier differences to apply.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This can be observed for partial methods here

partial class C
{
    public partial Span<int> M(Span<int> x);
    public partial Span<int> M(scoped Span<int> x) => default; // The 'scoped' modifier of parameter 'x' doesn't match partial method declaration.CS8988
    
    public partial Span<int> M2(scoped Span<int> x);
    public partial Span<int> M2(Span<int> x) => default; // The 'scoped' modifier of parameter 'x' doesn't match partial method declaration.CS8988
}

Copy link
Member

Choose a reason for hiding this comment

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

It looks like we allow some variations specifically for scoped on partial method, so the normal rule of "must have the same modifiers" doesn't seem right

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not aware of any permitted variations on scoped modifiers on partial methods. The method in the implementation which checks this is called CheckValidScopedOverride, which may be misleading, but it is also passing argument allowVariance: false. In other words, scoped-ness of parameters across partial methods must be exactly the same, otherwise an error (not a warning) occurs.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, thanks for catching that. Cool :-)

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

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 (iteration 5)

@jcouv jcouv self-assigned this Apr 25, 2024
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 5)

@RikkiGibson RikkiGibson merged commit 7333e46 into main Apr 25, 2024
1 check passed
@RikkiGibson RikkiGibson deleted the RikkiGibson-patch-2 branch April 25, 2024 22:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants