Skip to content

Partial events and constructors: merge and check symbols #76970

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.

Sign up for GitHub

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

Jump to bottom
Merged
jjonescz merged 15 commits into from
Feb 8, 2025
Merged

Partial events and constructors: merge and check symbols #76970

jjonescz merged 15 commits into from
Feb 8, 2025

Conversation

@jjonescz
Copy link
Member

@jjonescz jjonescz commented Jan 30, 2025

Test plan: #76859

@ghost ghost added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Jan 30, 2025
@jjonescz jjonescz marked this pull request as ready for review January 30, 2025 04:14
@jjonescz jjonescz requested a review from a team as a code owner January 30, 2025 04:14
@jjonescz jjonescz requested review from RikkiGibson and cston January 30, 2025 04:14
@RikkiGibson RikkiGibson self-assigned this Jan 30, 2025
RikkiGibson
RikkiGibson reviewed Jan 31, 2025
View reviewed changes
RikkiGibson
RikkiGibson reviewed Jan 31, 2025
View reviewed changes
cston
cston reviewed Jan 31, 2025
View reviewed changes
cston
cston reviewed Jan 31, 2025
View reviewed changes
@jjonescz jjonescz requested review from RikkiGibson and cston February 4, 2025 19:43
jjonescz
jjonescz commented Feb 5, 2025
View reviewed changes
src/Compilers/CSharp/Portable/Emitter/Model/MethodSymbolAdapter.cs Outdated
Debug.Assert((object)AdaptedMethodSymbol.PartialDefinitionPart == null); // must be definition
if (AdaptedMethodSymbol.PartialDefinitionPart is { } definition)
{
return definition.GetCciAdapter().ResolvedMethodImpl(context);
Copy link
Member Author

@jjonescz jjonescz Feb 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This change was needed to avoid crash in the sequence point test (added in the same commit). Similar test would also crash for partial methods/properties. Basically the VerifyMethodBody test helper calls GetResolvedMethod on the implementation Cci symbol - that probably doesn't happen normally during emit, that's why the assert was fine for normal scenarios.

Copy link
Member

@RikkiGibson RikkiGibson Feb 6, 2025

Choose a reason for hiding this comment

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

Is there any risk that this is adding a new capability to users of the public API, that they may come to depend on, and we didn't intend?

Copy link
Member Author

@jjonescz jjonescz Feb 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Are Cci adapters public API? Anyway I think this was simply wrong before. And users of public APIs would probably not hit the assert (because they use Release bits), so they would just get the wrong result.

Copy link
Contributor

@AlekseyTs AlekseyTs Feb 6, 2025

Choose a reason for hiding this comment

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

This change doesn't look right. I think the API is not supposed to return a different instance, it should either return an original instance, or null. Basically, for a pair of partial declarations, we should never "create" two different adapters, translation layer should make sure of that. Symbols should never be casted to CCI interfaces, they should be translated.

Copy link
Contributor

@AlekseyTs AlekseyTs Feb 6, 2025

Choose a reason for hiding this comment

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

If I remember correctly, PEModuleBuilder performs the translation. There are APIs called "Translate..."

Copy link
Member Author

@jjonescz jjonescz Feb 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Okay, thanks, I think I can skip the failing test for now and look at this separately. Or better yet, I can simply fix this inside the test utility.

@jjonescz
Copy link
Member Author

jjonescz commented Feb 5, 2025

@RikkiGibson for another look, thanks

@jjonescz
Copy link
Member Author

jjonescz commented Feb 6, 2025

@RikkiGibson for a second review, thanks

RikkiGibson
RikkiGibson reviewed Feb 6, 2025
View reviewed changes
Copy link
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.

Had some minor comments but otherwise LGTM.

src/Compilers/CSharp/Portable/Emitter/Model/MethodSymbolAdapter.cs Outdated
Debug.Assert((object)AdaptedMethodSymbol.PartialDefinitionPart == null); // must be definition
if (AdaptedMethodSymbol.PartialDefinitionPart is { } definition)
{
return definition.GetCciAdapter().ResolvedMethodImpl(context);
Copy link
Member

@RikkiGibson RikkiGibson Feb 6, 2025

Choose a reason for hiding this comment

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

Is there any risk that this is adding a new capability to users of the public API, that they may come to depend on, and we didn't intend?

src/Compilers/CSharp/Portable/Symbols/Source/SourceFieldLikeEventSymbol.cs
isIterator: false,
isNullableAnalysisEnabled: ev.DeclaringCompilation.IsNullableAnalysisEnabledIn(ev.CSharpSyntaxNode),
isExpressionBodied: false)
{
Copy link
Member

@RikkiGibson RikkiGibson Feb 6, 2025

Choose a reason for hiding this comment

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

Consider asserting ev.IsPartialDefinition.

Copy link
Member Author

@jjonescz jjonescz Feb 7, 2025

Choose a reason for hiding this comment

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

Thanks, turns out that wasn't true for extern events (because the extern partial part is the implementation part), so I fixed that as well.

src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs

static void mergeAccessors(ArrayBuilder<Symbol> nonTypeMembers, SourceEventAccessorSymbol? currentAccessor, SourceEventAccessorSymbol? prevAccessor)
{
if (currentAccessor?.IsPartialImplementation == true)
Copy link
Member

@RikkiGibson RikkiGibson Feb 6, 2025

Choose a reason for hiding this comment

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

I guess this is an intentional simplification compared to how properties handle it? Since there isn't a need to detect a condition where one of the accessors is missing on the definition part. For a property, I think we take care to keep the implementation part accessor in the member list in that case.

There is no way to elide an accessor on the definition part here, though, so this implementation is fine.

Copy link
Member Author

@jjonescz jjonescz Feb 6, 2025

Choose a reason for hiding this comment

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

Yes, that's correct.

src/Compilers/CSharp/Test/Emit3/PartialEventsAndConstructorsTests.cs
class [mscorlib]System.Action 'value'
) cil managed
{
} // end of method C::add_E
Copy link
Member

@RikkiGibson RikkiGibson Feb 6, 2025

Choose a reason for hiding this comment

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

I am confused by this. Is this saying that these methods have implementations in this assembly? I thought extern means they would not.

Copy link
Member Author

@jjonescz jjonescz Feb 6, 2025

Choose a reason for hiding this comment

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

I think they have only signatures in the metadata, not bodies. The IL decompiler shows it like this though.

src/Compilers/CSharp/Test/Emit3/PartialEventsAndConstructorsTests.cs Outdated
}
}
""";
// PROTOTYPE: Mismatch between accessibility modifiers of parts should be reported.
Copy link
Member

@RikkiGibson RikkiGibson Feb 6, 2025

Choose a reason for hiding this comment

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

I assume this is still tracked to be completed in a future PR.

Copy link
Member Author

@jjonescz jjonescz Feb 6, 2025

Choose a reason for hiding this comment

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

Yes, just not in this test anymore, I changed it so there is no mismatch so it actually tests what it's supposed to test.

@jjonescz jjonescz requested a review from RikkiGibson February 7, 2025 12:27
@jjonescz
Copy link
Member Author

jjonescz commented Feb 7, 2025

@RikkiGibson can you take another look? thanks

@jjonescz jjonescz merged commit 7c3501e into dotnet:features/PartialEventsCtors Feb 8, 2025
24 checks passed
@jjonescz jjonescz deleted the PartialEventsCtors-03-Symbols branch February 8, 2025 14:17
This was referenced Mar 7, 2025
Closed
Closed
This was referenced Mar 12, 2025
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AlekseyTs AlekseyTs AlekseyTs left review comments

@RikkiGibson RikkiGibson RikkiGibson approved these changes

+1 more reviewer

@cston cston cston approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@RikkiGibson RikkiGibson

Labels

Area-Compilers Feature - Partial Events and Constructors untriaged Issues and PRs which have not yet been triaged by a lead

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jjonescz @RikkiGibson @AlekseyTs @cston