Skip to content

Fix #3909: emit nullable override disambiguators#3910

Open
sailro wants to merge 3 commits into
icsharpcode:masterfrom
sailro:fix-3909-default-constraint
Open

Fix #3909: emit nullable override disambiguators#3910
sailro wants to merge 3 commits into
icsharpcode:masterfrom
sailro:fix-3909-default-constraint

Conversation

@sailro

@sailro sailro commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Fixes #3909.

Problem

C# normally inherits generic constraints on overrides and explicit interface implementations, so
ILSpy correctly avoids restating them. Nullable annotations introduce one exception: when a
method type parameter appears as T?, C# requires class, struct, or default to distinguish
a nullable annotation from Nullable<T>.

Omitting that discriminator can change the signature and produce CS0115 / CS0453.

Solution

For overrides and explicit interface implementations:

  • collect method type parameters that appear with a nullable annotation anywhere in the return or
    parameter types;
  • skip value-type-constrained parameters, where T? already means Nullable<T>;
  • emit class for an inherited reference-type constraint, otherwise emit default.

Roslyn records the inherited reference/value constraint flags on the implementation method's own
type parameters, so this remains exact even when the base assembly cannot be resolved. Other
inherited constraints remain omitted, as required by C#.

Tests

Added a Roslyn 3+ Pretty fixture covering:

  • unconstrained and notnull parameters requiring default;
  • class and class? contracts requiring plain class;
  • struct and non-nullable-signature controls;
  • return-only and nested-array nullable annotations;
  • an explicit interface implementation.

Validation:

  • Issue3909 Pretty matrix: 6 passed

  • related generic / nullable Pretty matrices: 26 passed

  • full Pretty suite: 1,919 passed, 5 existing skips

  • type-system unit tests: 174 passed

  • Release solution build: 0 warnings, 0 errors

  • full solution test suite: 4,927 passed, 15 existing skips

  • At least one test covering the code changed

Override constraints are normally inherited and omitted, but nullable type
parameters still require class or default to distinguish annotations from
Nullable<T>. Derive that legal discriminator from the method metadata.

Assisted-by: Copilot:gpt-5.6-sol:GitHub Copilot CLI
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 86d2918e-5a24-48b4-9a86-41d331ec3720
@siegfriedpammer

Copy link
Copy Markdown
Member

Which version of c# was this feature introduced? Is there a specification?

@sailro

sailro commented Jul 25, 2026

Copy link
Copy Markdown
Contributor Author

C# 9.0. There is a specification, though the situation is worth spelling out, because the standard has not caught up with it.

Version

The compiler is explicit about it. Compiling the fixture with LangVersion 8.0:

error CS8400: Feature 'default type parameter constraints' is not available in C# 8.0. Please use language version 9.0 or greater.
error CS8627: A nullable type parameter must be known to be a value type or non-nullable reference type unless language version '9.0' or greater is used.

The second one matters here: T? on a type parameter that is not constrained to class or struct is itself a C# 9 feature, so the annotation and the constraint always appear together. C# 8 already allowed restating class / struct on an override for this purpose; C# 9 only added default.

Specification

The normative text is the csharplang speclet Unconstrained type parameter annotations (champion issue dotnet/csharplang#3297, design meetings LDM 2019-11-25 and LDM 2020-06-17). Its "default constraint" section gives the two rules, both enforced by Roslyn:

  • it is an error to use default other than on a method override or explicit interface implementation - CS8823
  • it is an error to use default when the corresponding type parameter of the overridden or interface method is constrained to a reference type or a value type - CS8822 (with CS8665 as the converse guard for class: "... is not a reference type")

ECMA-334 does not cover the feature. The published 7th edition (December 2023) standardizes C# 7, and even the committee's C# 9 draft (dotnet/csharpstandard, branch draft-v9) has not incorporated it yet: in standard/classes.md, the 15.2.5 grammar is still

primary_constraint
    : class_type nullable_type_annotation?
    | 'class' nullable_type_annotation?
    | 'struct'
    | 'notnull'
    | 'unmanaged'
    ;

and 15.6.1 still reads "Such declarations may only have type_parameter_constraints_clauses containing the primary_constraints class and struct". So for now the speclet and the Roslyn implementation are the only authorities. The user documentation is where (generic type constraint).

A defect in this PR, found while re-deriving the rule from the speclet

Please hold off reviewing the current head - answering your question made me check the patch against the speclet rather than against my own test cases, and it is not correct for one class of constraints. I would rather report that myself than have you find it.

Measuring what the compiler actually requires, for a base method public virtual void M<T>(T? v) with the given constraint and an override with the given disambiguator:

base constraint required on the override
none, notnull, Enum, an interface, T : U (U class-constrained or unconstrained) where T : default
class, class?, a class type (e.g. Stream), Delegate, MulticastDelegate, T : U (U : Stream) where T : class
struct, unmanaged nothing

The patch derives the discriminator from ITypeParameter.HasReferenceTypeConstraint, which reflects only the metadata ReferenceTypeConstraint flag, that is, the class / class? keyword. For a class-type constraint it therefore emits default, which is CS8822, and the output still does not recompile. The restated class leaves no trace to read back either - for a base declaring where T : Stream, the override's type parameter is flags=None, constraints=[Stream] - so the discriminator has to be derived, not looked up.

ILSpy already has the right predicate: the tri-state IType.IsReferenceType, whose AbstractTypeParameter implementation carries the Object / ValueType / Enum carve-out and chains through T : U via EffectiveBaseClass.

This mirrors the wording of the compiler's own diagnostics and reproduces every row of the table above. I will push that shortly, together with fixture cases for a class-type constraint, Delegate, Enum, an interface constraint and both T : U directions, plus a nested-position case (the requirement applies to any occurrence of T? in the signature - T?[], List<T?>, a tuple element, ref / out, or the return type). I will also key the collector on type-parameter identity rather than owner kind plus index, so that a SpecializedMethod signature carrying a foreign type parameter of the same index cannot produce a spurious clause.

One residual I would leave unless you prefer it hardened: for constraint shapes only IL can express (a concrete delegate, array, struct or enum constraint - CS0701 / CS0706 in C#), IsReferenceType can differ from Roslyn's classification.

Language version

The clause is only emitted where a T? annotation on a non-value-type-constrained parameter is already being rendered, which is itself C# 9 and only happens when NullableReferenceTypes is enabled. Gating the constraint alone would therefore trade CS8400 for CS8627 / CS0453 and gain nothing. If you would like the setting to report honestly, the coherent change is for NullableReferenceTypes to imply C# 9 in GetMinimumRequiredVersion() - it maps to C# 8 today, so ILSpy can already emit C# 9-only T?. Happy to do that here or in a separate PR, whichever you prefer, and there is no urgency on my side.

@sailro
sailro force-pushed the fix-3909-default-constraint branch from 008cf7a to 65aef35 Compare July 25, 2026 14:35
@sailro

sailro commented Jul 25, 2026

Copy link
Copy Markdown
Contributor Author

Pushed the correction.

What changed. The discriminator is now derived from the tri-state ITypeParameter.IsReferenceType instead of the HasReferenceTypeConstraint / HasValueTypeConstraint metadata flags:

bool? isReferenceType = tp.IsReferenceType;
if (isReferenceType == false)
	continue;
c.BaseTypes.Add(new PrimitiveType(isReferenceType == true ? "class" : "default"));

AbstractTypeParameter.IsReferenceType already implements the classification this needs, including the Object / ValueType / Enum carve-out and the chain through a type-parameter constraint via EffectiveBaseClass, so the three outcomes line up with the wording of the compiler's own diagnostics ("is not a reference type" in CS8665, "is constrained to a reference type or a value type" in CS8822).

The collector now matches on type-parameter identity rather than owner kind plus index, so a specialized signature carrying a foreign type parameter that shares an index with one of the method's own cannot produce a spurious clause.

Test coverage. The Issue3909 fixture gained a class-type constraint, Delegate, Enum, an interface constraint, both directions of a T : TOuter chain (TOuter : Node requiring class, TOuter : class requiring default), a List<T?> nested annotation, a two-type-parameter method where only one is annotated, and a control where the annotated parameter belongs to the containing type while the method has its own parameter at the same index.

Checked as a red/green pair: with the previous implementation the fixture fails on exactly the three class-type rows, for every compiler in the matrix:

-  public override T? ClassType<T>(T? value) where T : class
+  public override T? ClassType<T>(T? value) where T : default
-  public override T? DelegateType<T>(T? value) where T : class
+  public override T? DelegateType<T>(T? value) where T : default
-  public override T? Chained<T>(T? value) where T : class
+  public override T? Chained<T>(T? value) where T : default

Validation

  • Issue3909 Pretty matrix: 6 passed
  • full Pretty suite: 1,919 passed, 5 existing skips
  • full solution test suite: 4,927 passed, 15 existing skips, 0 failed
  • Release solution build: 0 warnings, 0 errors

A class-type constraint such as Stream or Delegate sets no
ReferenceTypeConstraint flag, so keying the disambiguator off that flag gave
those overrides the default constraint, which is CS8822, and the output still
did not recompile. The restated disambiguator leaves no metadata trace of its
own, so the choice has to follow from whether the inherited constraints make
the type parameter a reference type, a value type, or neither.

Matching the annotated type parameters by identity rather than by owner kind
and index also keeps a specialized signature from contributing a foreign type
parameter that happens to share an index.

Assisted-by: Copilot:claude-opus-5:GitHub Copilot CLI
@sailro
sailro force-pushed the fix-3909-default-constraint branch from 65aef35 to 0e60038 Compare July 25, 2026 14:37
IsReferenceType is a bool?, so choosing between class, default and no
constraint at all is a three-state decision. Spelling those states out keeps
that visible where the choice is made, rather than leaving it implied by a
comparison against true.

Assisted-by: Copilot:claude-opus-5:GitHub Copilot CLI
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Generic override loses where T : default and does not recompile

2 participants