Fix cross-assembly source generation accessibility

[Amaury Levé (Evangelink)]

Summary

• make reflection-free member accessibility checks account for the declaring assembly
• prevent inaccessible nearer members from exposing hidden ancestor members during model construction
• add two-assembly regression coverage for inherited methods and properties

Testing

• build.cmd -test -projects test\UnitTests\MSTest.SourceGeneration.UnitTests\MSTest.SourceGeneration.UnitTests.csproj -bl

[github-actions]

🧪 Expert test review — PR #11014

Grade	Test	Mutation	Notes	How to improve
A (90–100)	new MSTestReflectionMetadataGeneratorTests. Generator_ ExcludesInaccessibleMembersFromBaseTypeInAnotherAssembly	4/4 killed	Covers the derived-first dedup regression: an inaccessible derived-type member correctly shadows and excludes an otherwise-accessible grandparent member, plus verifies the inaccessible-getter diagnostic and absence of compile errors.	—

_{This advisory comment was generated automatically. Grades are heuristic
and informational — they do not block merging. Suggestions on the Files
changed tab can be applied with one click. Re-run with
/review-tests.}

🤖 Automated content by GitHub Copilot. Generated by the Test Reviewer on PR (on open / sync) workflow. · auto · 43.1 AIC · ⌖ 2.47 AIC · ⊞ 16.9K · [◷]( · ◷)

[github-actions]

Note

🤖 Automated review by GitHub Copilot. Generated by the Expert Code Review workflow. To request a follow-up action, reply by tagging @copilot directly.

✅ 22/22 dimensions clean — no findings.

Review notes:

• Algorithmic Correctness: Moving the dedup check (seenMethodKeys.Add) before the accessibility check is correct — derived-first iteration means the derived declaration wins for new-shadowed members. The hasUnsupportedTestMethod flag is still set on the first (derived) occurrence when applicable. For properties, the flag is set unconditionally before dedup/accessibility, so no regression there either.
• The core fix (IsAccessibleFromConsumer now taking consumingAssembly) correctly addresses the bug where Internal and ProtectedOrInternal members from external assemblies were incorrectly treated as accessible. The delegation to SymbolReferenceabilityHelper.IsMemberAccessibleFrom is consistent with how DynamicDataSourceBuilder and AttributeMaterializationHelper already handle this.
• HasGettableValue now uses the same assembly-aware check via IsMemberAccessibleFrom(getter.DeclaredAccessibility, getter.ContainingType, consumingAssembly), replacing the hardcoded accessibility enum check — consistent and correct.
• Dictionary→HashSet refactor reduces memory by not storing values that were never read back, while preserving the same dedup semantics.
• Test coverage: The new Generator_ExcludesInaccessibleMembersFromBaseTypeInAnotherAssembly test exercises the cross-assembly scenario with multi-level inheritance, new-shadowing, protected internal inaccessibility, and inaccessible getters — good coverage of the fix.

[Copilot]

Copilot review overview

🟡 Changes recommended

Two critical hiding defects remain, and cross-assembly internal coverage is incomplete.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review tier: Balanced
Findings: 2 · 1

New issues introduced by this change (3)

Severity	Finding
	src/​Analyzers/​MSTest.SourceGeneration/​Generators/​TestClassModelBuilder.cs — These kind-specific sets still miss cross-kind hiding. C# member lookup hides base members by name…
	src/​Analyzers/​MSTest.SourceGeneration/​Generators/​TestClassModelBuilder.cs — BuildMethodSignatureKey distinguishes static and instance methods, but staticness is not part of…
	src/​Analyzers/​MSTest.SourceGeneration/​Generators/​TestMemberValidationHelper.cs — The assembly-sensitive predicate now covers both Internal and ProtectedOrInternal, but the…

What changed in this PR

Updates source generation to account for cross-assembly accessibility and inherited-member hiding.

Changes:

• Adds assembly-aware accessibility checks.
• Revises inherited-member filtering.
• Adds two-assembly regression coverage.

File	Review
test/​UnitTests/​MSTest.SourceGeneration.UnitTests/​MSTestReflectionMetadataGeneratorTests.cs	Adds cross-assembly tests, but lacks direct internal member coverage.
src/​Analyzers/​MSTest.SourceGeneration/​Generators/​TestMemberValidationHelper.cs	Adds assembly-sensitive checks. Moderate: add cross-assembly internal method, property, and getter regressions.
src/​Analyzers/​MSTest.SourceGeneration/​Generators/​TestClassModelBuilder.cs	Critical: method hiding incorrectly distinguishes staticness and does not account for cross-kind hiding, potentially generating uncompilable code.

---

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Copilot review overview

🔵 Needs a closer look

The two model-construction issues can silently omit inherited tests.

Review tier: Balanced
Findings: None

Issues resolved since last review (3)

Severity	Finding
	src/​Analyzers/​MSTest.SourceGeneration/​Generators/​TestMemberValidationHelper.cs — The assembly-sensitive predicate now covers both Internal and ProtectedOrInternal, but the… View resolved comment
	src/​Analyzers/​MSTest.SourceGeneration/​Generators/​TestClassModelBuilder.cs — BuildMethodSignatureKey distinguishes static and instance methods, but staticness is not part of… View resolved comment
	src/​Analyzers/​MSTest.SourceGeneration/​Generators/​TestClassModelBuilder.cs — These kind-specific sets still miss cross-kind hiding. C# member lookup hides base members by name… View resolved comment

Suppressed comments (2)

src/Analyzers/MSTest.SourceGeneration/Generators/TestClassModelBuilder.cs:122

• This hiding check runs before accessor attributes update hasUnsupportedTestMethod. A base getter/setter carrying [TestMethod] remains visible to the reflection fallback even when a derived method has the property's name; declaring descriptors complete here prevents that fallback and loses the accessor test. Move the existing accessor check ahead of this early exit.

                        if (methodNamesInDerivedTypes.Contains(property.Name)
                            || nonMethodNamesInDerivedTypes.Contains(property.Name))
                        {
                            break;

src/Analyzers/MSTest.SourceGeneration/Generators/TestClassModelBuilder.cs:89

• This early exit drops a cross-kind-hidden ancestor test without marking descriptor generation incomplete. Reflection's Type.GetMethods still returns a base method when a nearer field/property merely has the same name, so AreGeneratedDescriptorsComplete can remain true and TypeEnumerator skips the legacy scan, silently losing that test. Record a skipped [TestMethod] here; the regression should also verify completeness becomes false.

                        if (nonMethodNamesInDerivedTypes.Contains(method.Name))
                        {
                            break;

[github-actions]

🧪 Expert test review — PR #11014

No new or modified test methods were identified in the changed regions of this PR (the pre-step test-file extraction produced no data to review).

_{Re-run with /review-tests.}

🤖 Automated content by GitHub Copilot. Generated by the Test Reviewer on PR (on open / sync) workflow. · auto · 44.6 AIC · ⌖ 2.45 AIC · ⊞ 16.9K · [◷]( · ◷)

[Copilot]

Copilot review overview

🟡 Changes recommended

Critical member-hiding and signature-normalization cases can still generate uncompilable invocations.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review tier: Balanced
Findings: 2

New issues introduced by this change (2)

Severity	Finding
	src/​Analyzers/​MSTest.SourceGeneration/​Generators/​TestClassModelBuilder.cs — The deduplication still considers only exact signatures, but generated invokers access members…
	src/​Analyzers/​MSTest.SourceGeneration/​Generators/​TestMemberValidationHelper.cs — Removing staticness fixes only one signature-equivalence case: this textual key still distinguishes…

[Copilot]

Copilot review overview

🔵 Needs a closer look

Differing-signature ancestor methods must be suppressed when hidden by a derived same-name method group.

Review tier: Balanced
Findings: None

Issues resolved since last review (2)

Severity	Finding
	src/​Analyzers/​MSTest.SourceGeneration/​Generators/​TestMemberValidationHelper.cs — Removing staticness fixes only one signature-equivalence case: this textual key still distinguishes… View resolved comment
	src/​Analyzers/​MSTest.SourceGeneration/​Generators/​TestClassModelBuilder.cs — The deduplication still considers only exact signatures, but generated invokers access members… View resolved comment

Suppressed comments (1)

src/Analyzers/MSTest.SourceGeneration/Generators/TestClassModelBuilder.cs:92

• Accessible methods declared on a derived level also hide the entire same-name method group from ancestor levels, even when the signatures differ. This condition ignores methodNamesInDerivedTypes, so the existing DerivedTests.Op(string) / BaseTests.Op(int) shape still emits the base invoker as ((DerivedTests)instance).Op(int), which cannot bind to the hidden base method and makes the generated compilation fail. Suppress differing-signature ancestor methods after exact-signature deduplication (while preserving overloads declared in the same type), and update the overload regression to compile the generated output rather than expecting both inherited entries.

                        if (nonMethodNamesInDerivedTypes.Contains(method.Name)
                            || inaccessibleMethodNamesInDerivedTypes.Contains(method.Name))

[github-actions]

🧪 Expert test review — PR #11014

Grade	Test	Mutation	Notes	How to improve
C (70–79)	new MSTestReflectionMetadataGeneratorTests. Generator_ PropertyHidingTestMethod_ MarksDescriptorsIncomplete	1/2 killed	Only asserts the completeness flag flips; doesn't verify the hidden test method's Invoke entry is actually absent from the registry.	Add an assertion that the registry contains no Invoke entry calling Hidden() as a method, to directly confirm exclusion rather than inferring it from the flag.
C (70–79)	new MSTestReflectionMetadataGeneratorTests. Generator_ MethodHidingTestAttributedPropertyAccessor_ MarksDescriptorsIncomplete	1/2 killed	Same as the sibling case: only the completeness flag is checked, not that the [TestMethod]-attributed getter was actually excluded.	Assert directly that no method/property entry for the hidden accessor is registered, instead of relying solely on the flag.
B (80–89)	new MSTestReflectionMetadataGeneratorTests. Generator_ DynamicAndObjectParameters_ HaveSameSignatureKey	2/2 killed	Correctly verifies dynamic and object parameters collapse to one signature key via a count assertion.	Also assert which overload survives (e.g. that the retained entry has the object parameter type), not just the count.
A (90–100)	new MSTestReflectionMetadataGeneratorTests. Generator_ ExcludesInaccessibleMembersFromBaseTypeInAnotherAssembly	10/10 killed	Comprehensive: separately verifies every hiding/inaccessibility combination (hidden test, internal test, property-hides-method, static-hides-instance, inaccessible overload, method-hides-property, inaccessible/internal context) plus getter-diagnostics and zero compiler errors.	—

Scope note: Only one test file changed in this PR (test/UnitTests/MSTest.SourceGeneration.UnitTests/MSTestReflectionMetadataGeneratorTests.cs), adding 4 new [TestMethod]s. No inline suggestions were posted: the two C-grade findings need an assertion whose exact literal text depends on the generator's emitted registry format, which could not be confidently confirmed to compile as a one-line suggestion without risking a false match against the sibling Properties = ... entry that shares the same hidden member name.

_{This advisory comment was generated automatically. Grades are heuristic and informational — they do not block merging. Re-run with /review-tests.}

🤖 Automated content by GitHub Copilot. Generated by the Test Reviewer on PR (on open / sync) workflow. · auto · 98 AIC · ⌖ 2.56 AIC · ⊞ 16.9K · [◷]( · ◷)

[Copilot]

Copilot review overview

🟡 Changes recommended

Compile-diagnostic regression tests are missing for field, event, and nested-type hiding cases.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review tier: Balanced
Findings: 1

New issues introduced by this change (1)

Severity	Finding
	src/​Analyzers/​MSTest.SourceGeneration/​Generators/​TestClassModelBuilder.cs — The new field, event, and nested-type hiding branches have no regression coverage; the added…

Issues resolved since last review (3)

Severity	Finding
	src/​Analyzers/​MSTest.SourceGeneration/​Generators/​TestMemberValidationHelper.cs — The new return-type distinction is not covered by a negative case: removing this condition still… View resolved comment
	src/​Analyzers/​MSTest.SourceGeneration/​Generators/​TestMemberValidationHelper.cs — dynamic still falls through to SymbolEqualityComparer.Default, which does not equate it with… View resolved comment
	src/​Analyzers/​MSTest.SourceGeneration/​Generators/​TestMemberValidationHelper.cs — This comparison ignores substitutions on a nested generic type's containing type. For example,… View resolved comment

[github-actions]

🧪 Expert test review — PR #11014

No new or modified test methods were identified in the changed regions of this PR (the automated test-file extraction step produced no output to review).

_{Re-run with /review-tests.}

🤖 Automated content by GitHub Copilot. Generated by the Test Reviewer on PR (on open / sync) workflow. · auto · 38.7 AIC · ⌖ 2.67 AIC · ⊞ 16.9K · [◷]( · ◷)

[Copilot]

Copilot review overview

🔵 Needs a closer look

One or more issues must be addressed before approval.

Review tier: Balanced
Findings: None

Issues resolved since last review (1)

Severity	Finding
	src/​Analyzers/​MSTest.SourceGeneration/​Generators/​TestClassModelBuilder.cs — The new field, event, and nested-type hiding branches have no regression coverage; the added… View resolved comment

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/Analyzers/MSTest.SourceGeneration/Generators/TestMemberValidationHelper.cs:129

• Method type-parameter names are not part of a CLR signature, so Run<T>(T) and Run<U>(U) must compare equal. Comparing Name makes this helper return false for equivalent generic signatures, contrary to its runtime-signature contract and the ordinal-based normalization already used in DependsOnShouldBeValidAnalyzer.cs:827-830. Compare the type-parameter ordinal instead, and add a direct regression with differently named method type parameters.

[Copilot]

Copilot review overview

🟡 Changes recommended

Generic method signatures must compare type parameters by kind and ordinal, not by name.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review tier: Balanced
Findings: 1

New issues introduced by this change (1)

Severity	Finding
	src/​Analyzers/​MSTest.SourceGeneration/​Generators/​TestMemberValidationHelper.cs — Generic method parameter names are not part of a runtime signature; their ordinal is. Consequently…

[github-actions]

🧵 Parallel-safety audit — PR #11014

Parallelization — audited assemblies:

Test assembly	Scope	Workers	Analyzer coverage
MSTest.SourceGeneration.UnitTests	off (no [assembly: Parallelize] found)	n/a	n/a
MSTest.Acceptance.IntegrationTests	MethodLevel	CPU count (Workers = 0)	coverable once the parallel-safety analyzers ship (attribute-based opt-in)

⚠️ MSTest.SourceGeneration.UnitTests has parallelization OFF — any note below for that assembly is a readiness checklist, not a live bug.

Findings: A (global-state) 0 · B (paths) 0 · C (declaration) 0 · D (over-serialization) 0 — by severity: Critical 0 · High 0 · Warning 0 · Info 1.

Both changed test files stay inside the finding taxonomy's blind spot in a good way — neither introduces a new lifecycle mutation, shared path, or [ResourceLock]/[DoNotParallelize] change:

• test/UnitTests/MSTest.SourceGeneration.UnitTests/MSTestReflectionMetadataGeneratorTests.cs (+528/-8) adds new [TestMethod]s that only build in-memory Roslyn Compilations, run the source generator, and assert on the emitted source text (registry.Should().Contain(...)) or reflect over a System.Reflection.Assembly.Load-ed in-memory sample assembly. No Environment.*, Directory.SetCurrentDirectory, Console.Set*, culture mutation, or filesystem writes were added.
• test/IntegrationTests/MSTest.Acceptance.IntegrationTests/SourceGenerationNonAotTests.cs changes a single StringAssert.Contains expected-string literal (the new DeclaringType parameter in the generated ResolveMethod call) — no behavioral or fixture change.

[Info · Low confidence] test/UnitTests/MSTest.SourceGeneration.UnitTests/MSTestReflectionMetadataGeneratorTests.cs — pre-existing (not introduced by this PR) helper CreateCompilation/GetMaterializedDataRows always names the in-memory sample assembly "TestSample" and loads it via System.Reflection.Assembly.Load, which registers into the process-wide loaded-assemblies table (a category-A-flavored process-global resource). Under the current off scope this is not a live race — it is only readiness context: if this project ever opts into [assembly: Parallelize], concurrently-running tests using the same synthetic assembly name could collide in AppDomain/assembly-resolution state. This PR's new tests reuse the existing helper rather than introducing the pattern, so no action is required today. Fix, if/when parallelization is enabled here: give each test/compilation a unique assembly name (e.g. suffixed with a GUID or test name).

No Warning/High/Critical findings.

_{Advisory only — heuristic, non-blocking. Re-run with /parallel-audit. This audit answers "is it parallel-safe?"; for testability, smells, or flakiness see the detect-static-dependencies / test-smell-detection / test-anti-patterns analyses.}

🤖 Automated content by GitHub Copilot. Generated by the Parallel-safety audit on PR (on open / sync) workflow. · auto · 112.9 AIC · ⌖ 9.99 AIC · ⊞ 24.8K · [◷]( · ◷)

[Copilot]

Copilot review overview

🟢 Approval recommended

The reviewed changes have regression coverage and no unresolved approval-blocking issues.

Review tier: Balanced
Findings: None

Issues resolved since last review (1)

Severity	Finding
	src/​Analyzers/​MSTest.SourceGeneration/​Generators/​TestMemberValidationHelper.cs — Generic method parameter names are not part of a runtime signature; their ordinal is. Consequently… View resolved comment

[Amaury Levé (Evangelink)]

/backport to rel/4.4

[github-actions]

Started backporting to rel/4.4: https://github.com/microsoft/testfx/actions/runs/34945341709