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

Fixup partial property API usage #74951

Merged
merged 2 commits into from
Sep 5, 2024

Conversation

RikkiGibson
Copy link
Contributor

@RikkiGibson RikkiGibson commented Aug 29, 2024

Related to #73772

This PR addresses the remaining issues in compiler layer.

I started to work through the remaining IDE issues in #74950, but struggled to determine the correct change to make in some cases, and also struggled to write and interpret results of tests in certain areas such as Find References.

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Aug 29, 2024
Comment on lines -2765 to -2767
// The partial definition part may include optional parameters whose default values we want to simulate assigning at the beginning of the method
// https://github.com/dotnet/roslyn/issues/73772: is this actually used/meaningful?
methodSymbol = methodSymbol.PartialDefinitionPart ?? methodSymbol;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -1591,6 +1591,15 @@ internal override void ForceComplete(SourceLocation? locationOpt, Predicate<Symb
}

DeclaringCompilation.SymbolDeclaredEvent(this);
if (this.IsPartialDefinition())
Copy link
Contributor Author

@RikkiGibson RikkiGibson Aug 30, 2024

Choose a reason for hiding this comment

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

This check is similar in purpose to the following check performed for ordinary methods. Essentially it seems that MethodCompiler will report the SymbolDeclaredEvents for implementation parts of partial methods (including property accessors). But for the definition parts, the symbol itself must do it.

protected sealed override void CompleteAsyncMethodChecksBetweenStartAndFinish()
{
if (IsPartialDefinition)
{
DeclaringCompilation.SymbolDeclaredEvent(this);
}
}

@RikkiGibson RikkiGibson marked this pull request as ready for review August 30, 2024 22:03
@RikkiGibson RikkiGibson requested a review from a team as a code owner August 30, 2024 22:03
@RikkiGibson
Copy link
Contributor Author

@dotnet/roslyn-compiler for review

@RikkiGibson RikkiGibson requested a review from a team September 4, 2024 23:40
@RikkiGibson
Copy link
Contributor Author

@dotnet/roslyn-compiler for second review

}
if (member is IMethodSymbol { PartialImplementationPart: { } methodImplementation })
memberSet.Add(methodImplementation);
else if (member is IPropertySymbol { PartialImplementationPart: { } propertyImplementation })
Copy link
Member

Choose a reason for hiding this comment

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

Will we need to fix this up for field, with property initializers on the implementation part?

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 don't think this code would need to do anything differently based on the implementation part having an initializer. We also don't expect any backing field to be included in the set because IsImplicitlyDeclared will always be true on it.

@RikkiGibson RikkiGibson merged commit 2ff1a47 into dotnet:main Sep 5, 2024
24 checks passed
@RikkiGibson RikkiGibson deleted the partial-props-api-cleanup branch September 5, 2024 18:25
@dotnet-policy-service dotnet-policy-service bot modified the milestones: 17.12, Next Sep 5, 2024
@akhera99 akhera99 modified the milestones: Next, 17.12 P3 Sep 26, 2024
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.

6 participants