Fix #3230: do not emit base-list interfaces the base list cannot name

[siegfriedpammer]

Fixes #3230.

A class may name its own protected nested interface in its base list (class F : F.IFoo), but naming a protected interface nested in a base class there (class SubF : F, SubF.ISubFoo, F.IFoo) does not compile, even though F.IFoo is accessible inside the class body. The interface-impl metadata still lists such inherited interfaces, so the decompiler emitted uncompilable base lists.

TypeSystemAstBuilder.ConvertTypeDefinition now skips base-list entries that are neither nested within the current type's nesting chain nor accessible from the enclosing scope without the protected-through-inheritance privilege (MemberLookup.IsAccessible with allowProtectedAccess: false).

Includes the issue's repro as InterfaceTests fixtures (red before the fix: the decompiled base list contained the extra Issue3230_F.IFoo). Full PrettyTestRunner sweep: 1766 passed, 0 failed, 11 skipped.

🤖 Generated with Claude Code

[christophwille]

Note

This review was entirely AI generated (Claude Fable 5 via Claude Code) on behalf of @christophwille.

Overview: The fix for #3230 is correct for the reported case and lives in the right place: TypeSystemAstBuilder.ConvertTypeDefinition filters DirectBaseTypes entries the C# base list cannot legally name, using the type system's own accessibility model (MemberLookup). Fixtures reproduce the issue's F/SubF shapes with red-before/green-after and a full PrettyTestRunner sweep -- good TDD discipline.

Verdict: Three findings, attached inline at the respective locations. I'd address the allowProtectedAccess comment/regression-test finding before merge, and make a deliberate call on the base-class-dropping trade-off rather than inheriting it implicitly. Conventions (ASCII, en-US, self-contained comments, fixture placement, no header churn) are all consistent with the repo's rules.

[christophwille]

Finding: the filter also drops base classes, silently, for non-C#-authored IL.

This check runs on every DirectBaseTypes entry, including the base class. For C#-authored assemblies it is provably safe: anything the base list couldn't name could only have entered the metadata transitively (as in the issue's repro), so dropping it reconstructs the original source. But IL-authored or obfuscator-mangled assemblies can violate C# accessibility rules -- e.g. a cross-assembly internal base type whose InternalsVisibleTo was stripped. Previously the decompiler emitted an uncompilable but honest base list; now the base class (or a non-implied interface) disappears without a trace, and the type reads as deriving from object.

For interfaces this is a defensible trade-off; for the base class it hides a semantically critical fact. Suggestion: either restrict the skip to baseType.Kind == TypeKind.Interface, or leave a marker comment in the output when a base class is dropped. (The td == null -> return true default for unresolved types is the right conservative choice -- missing references are common and must not trigger the filter.)

Related, out of scope for #3230 but worth a follow-up note: the same uncompilable-base-list problem still exists one level down for inaccessible types used as type arguments of a base type (class SubF : F, IEnumerable<F.ProtNested>).

[christophwille]

Finding: allowProtectedAccess: false is inert here -- the comment attributes the behavior to the wrong mechanism.

MemberLookup.IsProtectedAccessible force-overrides the flag for type definitions (MemberLookup.cs:143-144):

// For static members and type definitions, we do not require the qualifying reference
// to be derived from the current class (allowProtectedAccess).
if (entity.IsStatic || entity.SymbolKind == SymbolKind.TypeDefinition)
    allowProtectedAccess = true;

Since this method only ever passes type definitions, the false argument has no effect. What actually rejects SubF : F, F.IFoo is the choice of lookup context: currentType.DeclaringTypeDefinition walks the enclosing type's nesting chain, which does not derive from F, so the protected check fails.

Importantly, the forced override is currently load-bearing in the right direction. This pattern is legal C# and must stay emitted:

class BaseOuter { protected interface I { } }
class Outer : BaseOuter
{
    class Inner : I { }   // legal: protected access via the enclosing type's inheritance
}

Here the nesting-chain check above fails (I is declared in BaseOuter, not in Inner/Outer), and it is precisely the forced allowProtectedAccess = true plus context Outer (which derives from BaseOuter) that keeps the entry. If someone ever "fixes" MemberLookup to honor the passed false for type definitions, this legal pattern silently regresses and nothing in the suite catches it.

Suggestion: reword the comment to say the enclosing-scope context is what models the rule (protected members of the enclosing types' base chains remain nameable; only the current type's own inheritance doesn't grant access), and pin the pattern above with a regression fixture.

Nit: the MemberLookup depends only on currentType and could be created once in ConvertTypeDefinition instead of per base-type entry -- negligible cost, purely cosmetic.

[christophwille]

Test coverage: the fixtures cover the issue's two shapes, but not the legal protected-through-enclosing-inheritance pattern that the fix must keep working (see the companion comment on BaseTypeAccessibleFrom -- class Outer : BaseOuter { class Inner : BaseOuter.ProtIface }). That case currently survives only via a subtle MemberLookup internal (the forced allowProtectedAccess for type definitions), so a fixture pinning it would guard against regressions in either place.

Minor: is the #if ROSLYN guard actually needed, i.e. does legacy csc reject this pattern? If it compiles there too, running it in the full matrix would widen coverage -- worth a quick check, not a blocker.

[christophwille]

Note

This re-review was entirely AI generated (Claude Fable 5 via Claude Code) on behalf of @christophwille.

Re-review after e6a4dde and 32e3ed8. All four findings from the previous review are addressed:

• The filter is now restricted to TypeKind.Interface, with a self-contained comment stating the transitivity invariant and why base classes are exempt.
• The BaseTypeAccessibleFrom comment now attributes the behavior to the enclosing-scope lookup context and documents that MemberLookup grants protected access for type definitions regardless of the allowProtectedAccess argument.
• The fixture matrix covers the accessibility spectrum (private, protected internal, private protected under CS72, generics) and -- importantly -- Issue3230_Outer2.Inner2 : IP pins the legal protected-through-enclosing-inheritance pattern that previously survived only via an undocumented MemberLookup internal.
• Type arguments are now filtered recursively, including arrays and tuples, with the Issue3230_TypeArg/Issue3230_SubTypeArg fixtures covering both keep and drop directions. The MemberLookup is hoisted out of the loop.

I traced the new fixture matrix through the filter logic (protected-internal kept via same-module internal access, private-protected dropped for SubPrivProt but kept for the nested ISub base list, generic INested<T> kept via the nesting-chain shortcut, IWrap3230<IFoo> dropped for SubTypeArg) -- all outcomes match the C# rules.

Two remaining findings, inline: one real gap that is still constructible from pure C# (the declared-accessibility check does not consider the declaring chain of the named type), and one minor blind spot for nullability-annotated type arguments. The first is the same bug class as #3230 and worth closing before merge; the second is contrived enough to be a judgment call.

Process notes: the two follow-ups are substantive enough to stand as separate commits, though squashing "Filter base-list interfaces with unnameable type arguments too" into the main fix would give a cleaner history per the repo's fixup convention -- maintainer's call. The question from the previous review whether legacy csc really rejects the fixture pattern (i.e. whether #if ROSLYN is needed) remains open.

[christophwille]

Finding: the check ignores the declaring chain of the named type -- a public interface nested in an inaccessible type passes the filter.

IsAccessible tests only the entity's own declared accessibility (its Private case walks the current type's nesting chain, but a Public entity returns true unconditionally). Naming A.I in a base list also requires A to be nameable. This is still constructible from pure C#, same flavor as #3230:

class Outer
{
    private class A { public interface I { } }
    internal interface IPub : A.I { }   // legal: A is accessible inside Outer
}

class X : Outer.IPub { }   // metadata lists IPub and, transitively, Outer.A.I

Decompiling X emits class X : Outer.IPub, Outer.A.I, which fails with CS0122 on A -- and dropping Outer.A.I is safe by the same transitivity invariant the fix already relies on.

Suggested fix: after the lookup.IsAccessible(td, false) check passes, recurse into the declaring type: return TypeDefinitionNameableInBaseList(td.DeclaringTypeDefinition, currentType, lookup); -- the nesting-chain shortcut at the top of the method already handles the exempted case where an enclosing type of td sits on the current type's own chain. (Accessibility.EffectiveAccessibility() exists at Accessibility.cs:132 but only intersects declared accessibilities; it cannot model the protected-through-enclosing-inheritance rule, so recursion through this method is the better fit.)

Worth a fixture in the matrix either way -- red before, green after.

[christophwille]

Minor: nullability-annotated type arguments fall through to this branch and skip their inner type arguments.

NullabilityAnnotatedType derives from DecoratedType, not ParameterizedType or TypeWithElementType, so a base entry like IWrap<IList<F.IFoo>?> matches default: -- only IList's definition is checked and the inaccessible F.IFoo inside is missed. Unwrapping at the top of BaseTypeAccessibleFrom (e.g. baseType = baseType.WithoutNullability();, TypeSystemExtensions.cs:799) closes it for one line.

Admittedly contrived (a nullable-annotated generic type argument wrapping a protected nested type), so leaving it with a comment would also be defensible -- but the one-line unwrap is cheaper than the comment.

[siegfriedpammer]

Use a TypeVisitor instead of handling all special cases?