Add ILVerify option to validate all metadata references - #132362
Conversation
github-actions
Bot
added
the
area-Tools-ILVerification
dotnet-policy-service
Bot
added
the
community-contribution
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
| VerifyTypes(peReader, module, path, ref numErrors, ref verifiedTypeCounter, ref typeCounter); | ||
|
|
||
| if (Get(_command.VerifyAllDependencies)) | ||
| VerifyMetadataReferences(peReader, path, ref numErrors); |
There was a problem hiding this comment.
The feature request was to validate references and nothing else.
It is fine and desirable to validate all metadata references by default.
| VerifyTypes(peReader, module, path, ref numErrors, ref verifiedTypeCounter, ref typeCounter); | ||
|
|
||
| if (Get(_command.VerifyAllDependencies)) | ||
| VerifyMetadataReferences(peReader, path, ref numErrors); |
There was a problem hiding this comment.
Many of the missing metadata references are going to be duplicate in VerifyMethods and VerifyTypes. Would it make sense to do the metadata reference validation first, and then omit the duplicate errors?
There was a problem hiding this comment.
I just realized the approach in 12d8343 isn't quite right — suppressing metadata resolution errors from VerifyMethods / VerifyTypes can hide errors that aren't actually duplicates. I'll rework this to deduplicate the errors instead.
| public Option<bool> Tokens { get; } = | ||
| new("--tokens", "-t") { Description = "Include metadata tokens in error messages" }; | ||
| public Option<bool> MetadataOnly { get; } = | ||
| new("--metadata-only") { Description = "Only validate metadata references" }; |
There was a problem hiding this comment.
| new("--metadata-only") { Description = "Only validate metadata references" }; | |
| new("--metadata-references-only") { Description = "Only validate metadata references" }; |
Or just --references-only
| else if (metadataOnly) | ||
| WriteLine("All metadata references in " + path + " resolved."); | ||
| else | ||
| WriteLine("All Classes and Methods in " + path + " Verified."); |
There was a problem hiding this comment.
| WriteLine("All Classes and Methods in " + path + " Verified."); | |
| WriteLine("All types and methods in " + path + " verified."); |
(Nit) pre-existing
| { | ||
| WriteLine($"Types found: {typeCounter}"); | ||
| WriteLine($"Types verified: {verifiedTypeCounter}"); | ||
| WriteLine($"Metadata errors: {metadataErrorCounter}"); |
There was a problem hiding this comment.
I do not think printing this number is useful. We do not have to print any detailed statics at all when we are validating references only.
2 participants
Fixes #96458
Adds an opt-in
--verify-all-dependenciesoption to ILVerify.With this option enabled, ILVerify performs an additional pass over supported metadata references, resolving each one independently so that all failures are reported. (The pass skips P/Invoke
ModuleRefs, since they name native libraries rather than managed netmodules.)Diagnostics use the most specific available error identifier and include metadata tokens when
--tokensis specified.For example, running with both options produces output like this:
Tests cover all supported reference kinds, including unused invalid references and valid assembly, type, and managed netmodule references.