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: merging declarations and emit of simple cases #72999

Merged

Conversation

RikkiGibson
Copy link
Contributor

@RikkiGibson RikkiGibson commented Apr 12, 2024

Test plan: #73090

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Apr 12, 2024
@RikkiGibson RikkiGibson force-pushed the partial-properties branch 3 times, most recently from 2332072 to 71d62ff Compare April 15, 2024 21:04
@RikkiGibson RikkiGibson changed the title Partial properties 1 Partial properties: merging declarations and emit of simple cases Apr 18, 2024
@RikkiGibson RikkiGibson changed the base branch from main to features/partial-properties April 18, 2024 22:47
@RikkiGibson RikkiGibson marked this pull request as ready for review April 19, 2024 03:02
@RikkiGibson RikkiGibson requested a review from a team as a code owner April 19, 2024 03:02
@RikkiGibson RikkiGibson added New Feature - Partial Properties and removed untriaged Issues and PRs which have not yet been triaged by a lead labels Apr 19, 2024
@@ -545,22 +545,28 @@ internal static bool IsExplicitInterfaceImplementation(this Symbol member)
}
}

internal static bool IsPartialMethod(this Symbol member)
internal static bool IsPartialMember(this Symbol member)
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 did consider introducing some new members on a common base type, or introducing an IPartialMemberSymbol interface or something similar. But ultimately I think extension methods with type tests is an appropriate level of abstraction for this feature.

I am meaning to add a debug assertion that these methods are only used with original definition symbols. I'll try and add a check for that in future PR.

@jcouv jcouv self-assigned this Apr 23, 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.

Done with review pass (iteration 1)

@RikkiGibson RikkiGibson requested a review from jcouv April 26, 2024 00:31
@RikkiGibson
Copy link
Contributor Author

I have addressed the feedback, please take another look at your convenience @jcouv.

@RikkiGibson RikkiGibson requested a review from jcouv April 26, 2024 19:00
@RikkiGibson
Copy link
Contributor Author

I have addressed the next round of feedback, please take another look at your convenience @jcouv.

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

Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

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

Leaving my feedback on the impl now. Still need to review tests.

Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

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

Done review pass.

@RikkiGibson
Copy link
Contributor Author

@333fred @jcouv I believe I have addressed all the comments. Please take another look at your convenience.

// not the implementation (member.Locations includes both parts). If the
// span is in fact in the implementation, return that member instead.
if (sym switch
#pragma warning disable format
Copy link
Member

Choose a reason for hiding this comment

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

nit: could we link to a tracking issue? Is the problem what's described in #46464?

Copy link
Member

Choose a reason for hiding this comment

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

Alternatively, take the switch out of the if?

    Symbol implementation = sym switch
    {
        MethodSymbol method => method.PartialImplementationPart,
        SourcePropertySymbol property => property.PartialImplementationPart,
        _ => throw ExceptionUtilities.UnexpectedValue(sym)
    };

    if (implementation is { } && InSpan(implementation.GetFirstLocation(), this.syntaxTree, memberSpan))
    {
        result = implementation;
        return true;
    }

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 sure, I feel like it is more likely to have the same cause as #73251

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 be willing to do either including a link or extracting the switch in a follow-up PR. I almost feel a little defiant toward keeping the current factoring since I want the formatter to be fixed :)

Copy link
Member

Choose a reason for hiding this comment

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

I'd like it fixed too :-) But all the related issues are currently marked as Backlog :-/
It would help to add links and screenshots to get a fix prioritized

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

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 6) with a minor comment to consider

@RikkiGibson RikkiGibson merged commit c3f13e3 into dotnet:features/partial-properties May 1, 2024
24 checks passed
@RikkiGibson RikkiGibson deleted the partial-properties branch May 1, 2024 00:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants