Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the Compatibility/GenAPI pipeline to correctly handle explicit interface events, ensuring generated reference source includes explicit event implementations (and filters them appropriately) and adds regression tests to cover the scenario.
Changes:
- Add GenAPI syntax generation for non-abstract explicit interface event implementations (emit
event IFoo.E { add { } remove { } }). - Extend symbol classification/filtering to treat events as explicit interface implementations and avoid emitting explicit interface event accessor methods.
- Add regression tests for explicit interface event generation and for exclusion when the implemented interface is internal and filtered out.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/Microsoft.DotNet.GenAPI.Tests/CSharpFileBuilderTests.cs | Adds regression coverage for explicit interface event emission and filtering when internal interfaces are excluded. |
| src/Compatibility/Microsoft.DotNet.ApiSymbolExtensions/SymbolExtensions.cs | Extends explicit interface implementation detection to include events. |
| src/Compatibility/Microsoft.DotNet.ApiSymbolExtensions/Filtering/ImplicitSymbolFilter.cs | Extends implicit-symbol filtering to exclude explicit-interface event accessors. |
| src/Compatibility/GenAPI/Microsoft.DotNet.GenAPI/SyntaxGeneratorExtensions.cs | Adds explicit-interface event declaration generation path for non-abstract events. |
| src/Compatibility/GenAPI/Microsoft.DotNet.GenAPI/CSharpAssemblyDocumentGenerator.cs | Filters explicit-interface events whose implemented interfaces are excluded by symbol filtering. |
| // If the method is an explicitly implemented getter or setter, exclude it. | ||
| // https://github.com/dotnet/roslyn/issues/53911 | ||
| if (method.MethodKind == MethodKind.ExplicitInterfaceImplementation && | ||
| method.ExplicitInterfaceImplementations.Any(m => m is { MethodKind: MethodKind.PropertyGet or MethodKind.PropertySet })) | ||
| method.ExplicitInterfaceImplementations.Any(m => m is { MethodKind: MethodKind.PropertyGet or MethodKind.PropertySet or MethodKind.EventAdd or MethodKind.EventRemove })) | ||
| { |
| if (member is IPropertySymbol property && !property.ExplicitInterfaceImplementations.IsEmpty && | ||
| property.ExplicitInterfaceImplementations.Any(m => !_options.SymbolFilter.Include(m.ContainingSymbol))) | ||
| { |
| if (member is IEventSymbol @event && !@event.ExplicitInterfaceImplementations.IsEmpty && | ||
| @event.ExplicitInterfaceImplementations.Any(m => !_options.SymbolFilter.Include(m.ContainingSymbol))) | ||
| { |
Skill vs. default Copilot review: dotnet#54492This PR is a faithful reproduction of the exact commit Copilot reviewed on dotnet/sdk#54492. The base branch carries the code-review skill, so the review here reflects skill-enhanced behavior. The original PR was reviewed by default Copilot Code Review. Same diff, two reviewers. The change adds explicit interface event handling parallel to the existing property and method handling, spread across 4 source files — a setup where correctness depends on keeping several parallel constructs in sync. Results
The skill review was a strict superset — it caught the one issue the default found, plus two more. Why the two extra findings matterBoth reviewers correctly cross-referenced the pre-existing method filter (
Both extra findings come from the same discipline: when a change introduces a set of parallel constructs, check every member of the set against the established pattern — not just the one nearest the changed line. That systematic completeness is what the skill adds here. |
microsoft/testfx#10387 split TerminalTestReporter.Summary.cs into focused partials, adding Coverage, FlakyTests, SlowestTests and TestDiscovery. Upstream includes the folder with a glob, so nothing flagged the addition here and edits to those four files were invisible to the drift detector (microsoft/testfx#10390). Append the four paths to the dotnet-test-terminal-reporter entry, baselined at the split commit acb5bafaa2. They are appended rather than sorted in because the tracking-issue marker keys on the source index, and #7 (Summary) and #8 (TestCompletion) have open issues (dotnet#55472, dotnet#55473) that inserting would orphan. Those two keep their old baselines: their drift is real, unported feature work (retry/flaky accounting, in-process retry attribution), not just the move. Also bump source #0 (TerminalTestReporter.cs): the only upstream change since its baseline is a <remarks> comment describing upstream's partial layout, which needs no port into the SDK's single-file fork. Document both gotchas in eng/vendored-files.md. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3328d629-8443-45a7-8ee2-97d3ce23dee5
Faithful reproduction of the exact commit Copilot reviewed on dotnet#54492. Base branch carries the code-review skill so Copilot's review here reflects skill-enhanced behavior.