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

Partial properties: attributes #73437

Merged

Conversation

RikkiGibson
Copy link
Contributor

@RikkiGibson RikkiGibson commented May 11, 2024

Test plan: #73090
Related spec change: dotnet/csharplang#8127

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels May 11, 2024
@@ -1087,10 +1093,29 @@ private CustomAttributesBag<CSharpAttributeData> GetAttributesBag()
return bag;
}

// The property is responsible for completion of the backing field
_ = BackingField?.GetAttributes();
var copyFrom = this.BoundAttributesSource;
Copy link
Contributor Author

@RikkiGibson RikkiGibson May 14, 2024

Choose a reason for hiding this comment

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

@RikkiGibson RikkiGibson marked this pull request as ready for review May 15, 2024 01:51
@RikkiGibson RikkiGibson requested a review from a team as a code owner May 15, 2024 01:51
@RikkiGibson RikkiGibson requested review from 333fred and jcouv May 15, 2024 01:52
@RikkiGibson
Copy link
Contributor Author

@jcouv @333fred for review

@jcouv

This comment was marked as resolved.

@jcouv
Copy link
Member

jcouv commented May 15, 2024

        SyntaxList<AttributeListSyntax> indexerNameAttributeLists,

Not related to this PR: it seems the only use we have for indexerNameAttributeLists is to check whether it is empty or not. We should consider passing a boolean flag instead of forcing some callers to allocate empty lists...


Refers to: src/Compilers/CSharp/Portable/Symbols/Source/SourcePropertySymbolBase.cs:92 in 3ce6777. [](commit_id = 3ce6777, deletion_comment = False)

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

@RikkiGibson
Copy link
Contributor Author

    /// Returns the definition part of a partial method implementation, 

Not related to this PR: is this comment correct? (and the one on SourcePartialDefinition above)

Refers to: src/Compilers/CSharp/Portable/Symbols/Source/SourceOrdinaryMethodSymbol.cs:351 in 3ce6777. [](commit_id = 3ce6777, deletion_comment = False)

For those outside of CodeFlow, context is:

/// <summary>
/// Returns the implementation part of a partial method definition,
/// or null if this is not a partial method or it is the definition part.
/// </summary>
internal SourceOrdinaryMethodSymbol SourcePartialDefinition
{
get
{
return this.IsPartialImplementation ? this.OtherPartOfPartial : null;
}
}
/// <summary>
/// Returns the definition part of a partial method implementation,
/// or null if this is not a partial method or it is the implementation part.
/// </summary>
internal SourceOrdinaryMethodSymbol SourcePartialImplementation
{
get
{
return this.IsPartialDefinition ? this.OtherPartOfPartial : null;
}
}

These comments seem wrong, it feels most reasonable to me to just delete them.

@RikkiGibson RikkiGibson requested a review from jcouv May 15, 2024 22:50
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 13)

@RikkiGibson
Copy link
Contributor Author

@333fred for second review

@RikkiGibson RikkiGibson requested a review from 333fred May 16, 2024 19:34
@RikkiGibson
Copy link
Contributor Author

179203f is all green and the next commit b190153 is comment only changes, so I will go ahead and force merge for expediency.

@RikkiGibson
Copy link
Contributor Author

@jcouv feel free to follow up in this PR if you had more comments on the more recent changes, I can be sure to address in subsequent PR.

@RikkiGibson RikkiGibson merged commit 28c886c into dotnet:features/partial-properties May 16, 2024
6 of 16 checks passed
@RikkiGibson RikkiGibson deleted the partial-properties-5 branch May 16, 2024 21:49
@RikkiGibson RikkiGibson restored the partial-properties-5 branch May 16, 2024 21:51
@RikkiGibson RikkiGibson deleted the partial-properties-5 branch May 16, 2024 21:51
@Sergio0694
Copy link
Contributor

Hey @RikkiGibson, couldn't find this specific bit in the spec, is this planned to work?

// User code
[field: Foo]
public partial string Name { get; set; }

// Generated.g.cs
private string _name;

public string Name { get => _name; set => _name = value; }

Or would this not work because Roslyn wouldn't be able to know what backing field the partial property implementation would be using? IIRC we discussed allowing [field:] on partial properties before, but it's not clear to me whether that was only expected to work if the partial property implementation would be using the field keyword (which won't be in C# 13 anyway). Just making sure I'm following so that I can also plan what I'll need to support and/or work around in my generators. Thank you! 😄

@RikkiGibson
Copy link
Contributor Author

Or would this not work because Roslyn wouldn't be able to know what backing field the partial property implementation would be using?

That's right.

It's possible that field keyword in properties will also land this release, which might make your generator's job easier, but otherwise, you'll be looking at suppressing the compiler warning about a bad attribute target, and "forwarding" the attribute to the field, as I think you've done before from a field to a property.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers New Feature - Partial Properties untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants