Validate invalid base type declarations in ILVerify

[pkuyo]

Fixes #119536

This adds ILVerify validation for invalid class base type declarations.

The original issue reports a case where ILAsm accepts a type declared with
extends object, but the resulting type cannot be loaded by the runtime.
ILVerify previously accepted that assembly without reporting an error.

This change reports InvalidBaseType for invalid base type declarations rather
than special-casing only the exact extends object spelling.

Tests added in BaseTypeTests.il:

• ObjectTypeSpecBase_InvalidType_InvalidBaseType

  • Covers the original repro: a class declared with extends object.
  • Expected result: InvalidBaseType.

• NilBaseInterface_ValidType_Valid

  • Ensures interfaces with no class base continue to verify successfully.

• GenericClassTypeSpecBase_ValidType_Valid

  • Ensures valid generic class TypeSpec bases are still accepted.

• ValueTypeBase_InvalidType_InvalidBaseType

  • Ensures a non-generic value type cannot be used as a class base.

• GenericValueTypeSpecBase_InvalidType_InvalidBaseType

  • Ensures a generic value type instantiation cannot be used as a class base.

[dotnet-policy-service]

Tagging subscribers to this area: @JulieLeeMSFT, @dotnet/jit-contrib
See info in area-owners.md if you want to be subscribed.

[pkuyo]

@dotnet-policy-service agree

[pkuyo]

Hi @jkotas, just checking in on this PR. I’ve addressed the feedback and made the requested updates. Please let me know if there’s anything else I should adjust. Thanks!

[pkuyo]

Thanks for the review. I updated the PR to address the latest feedback, The TypeSpec validation now follows the ECMA-335 augment behavior.

Please let me know if there is anything else I should adjust.

[MichalStrehovsky]

(Edited to fix issue number)

I wonder if we should approach this from an angle that also allows to address #4945

• Add ParseTypeSpec to EcmaSignatureParser.
• Do the validation in EcmaSignatureParser (this allows us getting rid of the ILVERIFICATION ifdef, because this could be a ReportInvalidTypeSpec partial method on EcmaSignatureParser: this matches more closely how invalid things are reported (e.g. in ILImporter.cs).

[jkotas]

• Add ParseTypeSpec to EcmaSignatureParser.
• Do the validation in EcmaSignatureParser (this allows us getting rid of the ILVERIFICATION ifdef, because this could be a ReportInvalidTypeSpec partial method on EcmaSignatureParser: this matches more closely how invalid things are reported (e.g. in ILImporter.cs).

Yes, I think it would look better. @pkuyo Could you please implement it?

[jkotas]

This should not be needed. It is covered by the resolvedBaseType.IsValueType || resolvedBaseType.IsInterface || !resolvedBaseType.IsDefType check below.

[pkuyo]

Thanks. I simplified this to a smaller TypeSpec-specific check.

The resolvedBaseType.IsValueType || resolvedBaseType.IsInterface || !resolvedBaseType.IsDefType check alone does not catch the #119536 case, because a TypeSpec that resolves to System.Object passes those resolved-type checks. The extra TypeSpec check is only for that context-specific extends validation.

[jkotas]

Ok, I see.

It does not feel right to have a different "spec" for typespecs depending on the context. Maybe we should be more restrictive in the default type spec parser like you had it originally and update the augments doc to match.

[jkotas]

@MichalStrehovsky Do you have opinion about this? Should we stick with the stricter definition of typespec to avoid bifurcating typespec spec?

[MichalStrehovsky]

Stricter sounds good to me. If we find out there are mainstream compilers breaking it, we could relax I guess. (MSVC doesn't count, it never even pretended to target ECMA-335.)

[pkuyo]

• Add ParseTypeSpec to EcmaSignatureParser.
• Do the validation in EcmaSignatureParser (this allows us getting rid of the ILVERIFICATION ifdef, because this could be a ReportInvalidTypeSpec partial method on EcmaSignatureParser: this matches more closely how invalid things are reported (e.g. in ILImporter.cs).

Yes, I think it would look better. @pkuyo Could you please implement it?

Sure, thanks. I’ll implement it this way and update the PR.

[pkuyo]

(Edited to fix issue number)

I wonder if we should approach this from an angle that also allows to address #4945

• Add ParseTypeSpec to EcmaSignatureParser.
• Do the validation in EcmaSignatureParser (this allows us getting rid of the ILVERIFICATION ifdef, because this could be a ReportInvalidTypeSpec partial method on EcmaSignatureParser: this matches more closely how invalid things are reported (e.g. in ILImporter.cs).

Thanks, I reworked this to use the EcmaSignatureParser approach.

The TypeSpec validation is now done through ParseTypeSpec / ReportInvalidTypeSpec, and I addedCoverage-relatedto #4945.

For CMOD_OPT/CMOD_REQ, I kept the check simple for now: it reports a TypeSpec reference while parsing another TypeSpec, but does not try to distinguish whether it actually forms a cycle. Full cycle detection would need extra tracking for the active TypeSpec resolution stack, since the current TypeSpec is not in the cache yet.

I wasn't sure that extra state was necessary for this PR, so I wanted to check before adding it.

[MichalStrehovsky]

For CMOD_OPT/CMOD_REQ, I kept the check simple for now: it reports a TypeSpec reference while parsing another TypeSpec, but does not try to distinguish whether it actually forms a cycle. Full cycle detection would need extra tracking for the active TypeSpec resolution stack, since the current TypeSpec is not in the cache yet.

We shouldn't allow TypeSpec as a modifier. It only causes problems. If the augment says we allow it, we should update the augment. We can capture the reason so that somebody doesn't try to relax it without thinking about the consequences (@jkotas do you agree?). I think this recently came up in #123819 and we decided not to handle TypeSpec either, but GitHub makes it really hard to find the relevant comment.

[jkotas]

We can capture the reason so that somebody doesn't try to relax it without thinking about the consequences (@jkotas do you agree?)

Yes

[jkotas]

This is not proposing any actual change anymore. Both original and proposed replacements are identical. If we want to propose any change here, it should be to add a sentence that explains why typespecs are not allowed.

[pkuyo]

Updated. I added a sentence explaining that TypeSpec tokens are not valid in this position because allowing them could form recursive TypeSpec encodings.

[jkotas]

Suggested change

	Allowing TypeSpec custom modifier types also creates a path for
	Allowing TypeSpec custom modifier types creates a path for

[jkotas]

Suggested change

	recursive TypeSpec encodings, which has caused problems in runtimes
	and tools. Such metadata should be rejected rather than cycle-detected.
	recursive TypeSpec encodings that are complicated to detect. Such metadata should be rejected rather than cycle-detected.

[jkotas]

Suggested change

	* https://github.com/dotnet/runtime/issues/4945
	* https://github.com/dotnet/runtime/pull/123819#discussion_r2810397721

[jkotas]

Nit: Could you please reformat this to have similar text width, and add new line before "Related issues"

[pkuyo]

Sure, I’ve updated it.