From 02ba41173c685aea881278696fed7a7b6a7b673c Mon Sep 17 00:00:00 2001 From: Pavel Koneski Date: Wed, 6 May 2026 15:09:33 -0700 Subject: [PATCH] Enable nullable annotations in Scripting.Metadata --- .../Microsoft.Scripting.Metadata/ClrStubs.cs | 4 +- .../MemoryMapping.V4.cs | 14 +- .../MetadataImport.cs | 125 ++++++++++-------- .../MetadataName.cs | 4 +- .../MetadataServices.cs | 7 +- .../MetadataTables.CCI.cs | 25 +++- .../MetadataTables.cs | 23 +++- .../Microsoft.Scripting.Metadata.csproj | 3 +- 8 files changed, 122 insertions(+), 83 deletions(-) diff --git a/src/core/Microsoft.Scripting.Metadata/ClrStubs.cs b/src/core/Microsoft.Scripting.Metadata/ClrStubs.cs index fa049031..1c817c9e 100644 --- a/src/core/Microsoft.Scripting.Metadata/ClrStubs.cs +++ b/src/core/Microsoft.Scripting.Metadata/ClrStubs.cs @@ -9,12 +9,12 @@ namespace Microsoft.Scripting.Metadata { internal static class ClrStubs { [System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Usage", "CA1801:ReviewUnusedParameters")] - internal static unsafe int GetCharCount(this Encoding encoding, byte* bytes, int byteCount, object nls) { + internal static unsafe int GetCharCount(this Encoding encoding, byte* bytes, int byteCount, object? nls) { return encoding.GetCharCount(bytes, byteCount); } [System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Usage", "CA1801:ReviewUnusedParameters")] - internal static unsafe void GetChars(this Encoding encoding, byte* bytes, int byteCount, char* chars, int charCount, object nls) { + internal static unsafe void GetChars(this Encoding encoding, byte* bytes, int byteCount, char* chars, int charCount, object? nls) { encoding.GetChars(bytes, byteCount, chars, charCount); } } diff --git a/src/core/Microsoft.Scripting.Metadata/MemoryMapping.V4.cs b/src/core/Microsoft.Scripting.Metadata/MemoryMapping.V4.cs index 72d33a12..a0b3746f 100644 --- a/src/core/Microsoft.Scripting.Metadata/MemoryMapping.V4.cs +++ b/src/core/Microsoft.Scripting.Metadata/MemoryMapping.V4.cs @@ -18,7 +18,7 @@ public sealed unsafe class MemoryMapping : CriticalFinalizerObject { [SecurityCritical] internal byte* _pointer; - private SafeMemoryMappedViewHandle _handle; + private SafeMemoryMappedViewHandle? _handle; internal long _capacity; [CLSCompliant(false)] @@ -51,11 +51,11 @@ public MemoryBlock GetRange(int start, int length) { [SecuritySafeCritical] public static MemoryMapping Create(string path) { - MemoryMappedFile file = null; - MemoryMappedViewAccessor accessor = null; - SafeMemoryMappedViewHandle handle = null; - MemoryMapping mapping = null; - FileStream stream = null; + MemoryMappedFile? file = null; + MemoryMappedViewAccessor? accessor = null; + SafeMemoryMappedViewHandle? handle = null; + MemoryMapping? mapping = null; + FileStream? stream = null; byte* ptr = null; try { @@ -95,7 +95,7 @@ public static MemoryMapping Create(string path) { // It is not safe to close the view at this point if there are any MemoryBlocks alive. // It's up to the user to ensure not to use them. Since you need unmanaged code priviledge to use them // this is not a security issue (it would be if this API was security safe critical). - _handle.ReleasePointer(); + _handle!.ReleasePointer(); // _handle is guaranteed to be non-null if _pointer is non-null, since we assign them together in Create. _pointer = null; } } diff --git a/src/core/Microsoft.Scripting.Metadata/MetadataImport.cs b/src/core/Microsoft.Scripting.Metadata/MetadataImport.cs index 6ecfda85..6a0cc009 100644 --- a/src/core/Microsoft.Scripting.Metadata/MetadataImport.cs +++ b/src/core/Microsoft.Scripting.Metadata/MetadataImport.cs @@ -4,6 +4,7 @@ using System; using System.Diagnostics; +using System.Diagnostics.CodeAnalysis; using System.Reflection; using System.IO; @@ -59,6 +60,7 @@ private void ReadOptionalHeaderDirectoryEntries(MemoryReader memReader) { memReader.SeekRelative(1 * 2 * sizeof(uint)); } + [MemberNotNull(nameof(_sectionHeaders))] private void ReadSectionHeaders(MemoryReader memReader) { if (memReader.RemainingBytes < _numberOfSections * PEFileConstants.SizeofSectionHeader) { throw new BadImageFormatException(); @@ -83,6 +85,7 @@ private void ReadSectionHeaders(MemoryReader memReader) { } } + [MemberNotNull(nameof(_sectionHeaders))] private void ReadPEFileLevelData() { if (_image.Length < PEFileConstants.BasicPEHeaderSize) { throw new BadImageFormatException(); @@ -150,7 +153,7 @@ internal MemoryBlock RvaToMemoryBlock(uint rva, uint size) { throw new BadImageFormatException(); } - private MemoryBlock DirectoryToMemoryBlock(DirectoryEntry directory) { + private MemoryBlock? DirectoryToMemoryBlock(DirectoryEntry directory) { if (directory.RelativeVirtualAddress == 0 || directory.Size == 0) { return null; } @@ -166,9 +169,9 @@ private MemoryBlock DirectoryToMemoryBlock(DirectoryEntry directory) { private StreamHeader[] _streamHeaders; private MemoryBlock _stringStream; - private MemoryBlock _blobStream; + private MemoryBlock? _blobStream; private MemoryBlock _guidStream; - private MemoryBlock _userStringStream; + private MemoryBlock? _userStringStream; private MetadataStreamKind _metadataStreamKind; private MemoryBlock _metadataTableStream; @@ -176,7 +179,7 @@ private MemoryBlock DirectoryToMemoryBlock(DirectoryEntry directory) { // private MemoryBlock _strongNameSignatureBlock; private void ReadCOR20Header() { - MemoryBlock memBlock = DirectoryToMemoryBlock(_optionalHeaderDirectoryEntries.COR20HeaderTableDirectory); + MemoryBlock? memBlock = DirectoryToMemoryBlock(_optionalHeaderDirectoryEntries.COR20HeaderTableDirectory); if (memBlock is null || memBlock.Length < _optionalHeaderDirectoryEntries.COR20HeaderTableDirectory.Size) { throw new BadImageFormatException(); } @@ -230,6 +233,7 @@ private void ReadStorageHeader(MemoryReader memReader) { _storageHeader.NumberOfStreams = memReader.ReadUInt16(); } + [MemberNotNull(nameof(_streamHeaders))] private void ReadStreamHeaders(MemoryReader memReader) { int numberOfStreams = _storageHeader.NumberOfStreams; _streamHeaders = new StreamHeader[numberOfStreams]; @@ -246,6 +250,9 @@ private void ReadStreamHeaders(MemoryReader memReader) { } } + [MemberNotNull(nameof(_stringStream))] + [MemberNotNull(nameof(_guidStream))] + [MemberNotNull(nameof(_metadataTableStream))] private void ProcessAndCacheStreams(MemoryBlock metadataRoot) { _metadataStreamKind = MetadataStreamKind.Illegal; @@ -310,15 +317,19 @@ private void ProcessAndCacheStreams(MemoryBlock metadataRoot) { } // mandatory streams: - if (_stringStream is null || _guidStream is null || _metadataStreamKind == MetadataStreamKind.Illegal) { + if (_stringStream is null || _guidStream is null || _metadataTableStream is null || _metadataStreamKind == MetadataStreamKind.Illegal) { throw new BadImageFormatException(); } } + [MemberNotNull(nameof(_streamHeaders))] + [MemberNotNull(nameof(_stringStream))] + [MemberNotNull(nameof(_guidStream))] + [MemberNotNull(nameof(_metadataTableStream))] private void ReadCORModuleLevelData() { ReadCOR20Header(); - MemoryBlock metadataRoot = DirectoryToMemoryBlock(_cor20Header.MetaDataDirectory); + MemoryBlock? metadataRoot = DirectoryToMemoryBlock(_cor20Header.MetaDataDirectory); if (metadataRoot is null || metadataRoot.Length < _cor20Header.MetaDataDirectory.Size) { throw new BadImageFormatException(); } @@ -339,53 +350,56 @@ private void ReadCORModuleLevelData() { #region Metadata private MetadataTableHeader _metadataTableHeader; - private int[] _tableRowCounts; - - internal ModuleTable ModuleTable; - internal TypeRefTable TypeRefTable; - internal TypeDefTable TypeDefTable; - internal FieldPtrTable FieldPtrTable; - internal FieldTable FieldTable; - internal MethodPtrTable MethodPtrTable; - internal MethodTable MethodTable; - internal ParamPtrTable ParamPtrTable; - internal ParamTable ParamTable; - internal InterfaceImplTable InterfaceImplTable; - internal MemberRefTable MemberRefTable; - internal ConstantTable ConstantTable; - internal CustomAttributeTable CustomAttributeTable; - internal FieldMarshalTable FieldMarshalTable; - internal DeclSecurityTable DeclSecurityTable; - internal ClassLayoutTable ClassLayoutTable; - internal FieldLayoutTable FieldLayoutTable; - internal StandAloneSigTable StandAloneSigTable; - internal EventMapTable EventMapTable; - internal EventPtrTable EventPtrTable; - internal EventTable EventTable; - internal PropertyMapTable PropertyMapTable; - internal PropertyPtrTable PropertyPtrTable; - internal PropertyTable PropertyTable; - internal MethodSemanticsTable MethodSemanticsTable; - internal MethodImplTable MethodImplTable; - internal ModuleRefTable ModuleRefTable; - internal TypeSpecTable TypeSpecTable; - internal ImplMapTable ImplMapTable; - internal FieldRVATable FieldRVATable; - internal EnCLogTable EnCLogTable; - internal EnCMapTable EnCMapTable; - internal AssemblyTable AssemblyTable; - internal AssemblyProcessorTable AssemblyProcessorTable; - internal AssemblyOSTable AssemblyOSTable; - internal AssemblyRefTable AssemblyRefTable; - internal AssemblyRefProcessorTable AssemblyRefProcessorTable; - internal AssemblyRefOSTable AssemblyRefOSTable; - internal FileTable FileTable; - internal ExportedTypeTable ExportedTypeTable; - internal ManifestResourceTable ManifestResourceTable; - internal NestedClassTable NestedClassTable; - internal GenericParamTable GenericParamTable; - internal MethodSpecTable MethodSpecTable; - internal GenericParamConstraintTable GenericParamConstraintTable; + + // all non-nullable fields assigned null! here below are unconditionally initialized to non-null by + // MetadataImport ctor -> ReadMetadataLevelData -> ProcessAndCacheMetadataTableBlocks + private int[] _tableRowCounts = null!; + + internal ModuleTable ModuleTable = null!; + internal TypeRefTable TypeRefTable = null!; + internal TypeDefTable TypeDefTable = null!; + internal FieldPtrTable FieldPtrTable = null!; + internal FieldTable FieldTable = null!; + internal MethodPtrTable MethodPtrTable = null!; + internal MethodTable MethodTable = null!; + internal ParamPtrTable ParamPtrTable = null!; + internal ParamTable ParamTable = null!; + internal InterfaceImplTable InterfaceImplTable = null!; + internal MemberRefTable MemberRefTable = null!; + internal ConstantTable ConstantTable = null!; + internal CustomAttributeTable CustomAttributeTable = null!; + internal FieldMarshalTable FieldMarshalTable = null!; + internal DeclSecurityTable DeclSecurityTable = null!; + internal ClassLayoutTable ClassLayoutTable = null!; + internal FieldLayoutTable FieldLayoutTable = null!; + internal StandAloneSigTable StandAloneSigTable = null!; + internal EventMapTable EventMapTable = null!; + internal EventPtrTable EventPtrTable = null!; + internal EventTable EventTable = null!; + internal PropertyMapTable PropertyMapTable = null!; + internal PropertyPtrTable PropertyPtrTable = null!; + internal PropertyTable PropertyTable = null!; + internal MethodSemanticsTable MethodSemanticsTable = null!; + internal MethodImplTable MethodImplTable = null!; + internal ModuleRefTable ModuleRefTable = null!; + internal TypeSpecTable TypeSpecTable = null!; + internal ImplMapTable ImplMapTable = null!; + internal FieldRVATable FieldRVATable = null!; + internal EnCLogTable EnCLogTable = null!; + internal EnCMapTable EnCMapTable = null!; + internal AssemblyTable AssemblyTable = null!; + internal AssemblyProcessorTable AssemblyProcessorTable = null!; + internal AssemblyOSTable AssemblyOSTable = null!; + internal AssemblyRefTable AssemblyRefTable = null!; + internal AssemblyRefProcessorTable AssemblyRefProcessorTable = null!; + internal AssemblyRefOSTable AssemblyRefOSTable = null!; + internal FileTable FileTable = null!; + internal ExportedTypeTable ExportedTypeTable = null!; + internal ManifestResourceTable ManifestResourceTable = null!; + internal NestedClassTable NestedClassTable = null!; + internal GenericParamTable GenericParamTable = null!; + internal MethodSpecTable MethodSpecTable = null!; + internal GenericParamConstraintTable GenericParamConstraintTable = null!; internal bool IsManifestModule { get { return AssemblyTable.NumberOfRows == 1; } @@ -748,6 +762,7 @@ internal MemoryBlock GetBlobBlock(uint blob) { return _blobStream.GetRange(dataOffset, size); } + [MemberNotNull(nameof(_blobStream))] internal int GetBlobDataOffset(uint blob, out int size) { if (_blobStream is null || blob >= _blobStream.Length) { throw new BadImageFormatException(); @@ -762,7 +777,7 @@ internal int GetBlobDataOffset(uint blob, out int size) { return offset + bytesRead; } - internal object GetBlobValue(uint blob, ElementType type) { + internal object? GetBlobValue(uint blob, ElementType type) { int offset = GetBlobDataOffset(blob, out int size); if (size < GetMinTypeSize(type)) { throw new BadImageFormatException(); @@ -1065,7 +1080,7 @@ internal MetadataName GetMetadataName(uint blob) { return _stringStream.ReadName(blob); } - internal object GetDefaultValue(MetadataToken token) { + internal object? GetDefaultValue(MetadataToken token) { int constantRid = ConstantTable.GetConstantRowId(token); if (constantRid == 0) { return Missing.Value; diff --git a/src/core/Microsoft.Scripting.Metadata/MetadataName.cs b/src/core/Microsoft.Scripting.Metadata/MetadataName.cs index 3f906ccf..e09eccab 100644 --- a/src/core/Microsoft.Scripting.Metadata/MetadataName.cs +++ b/src/core/Microsoft.Scripting.Metadata/MetadataName.cs @@ -35,7 +35,7 @@ public bool IsEmpty { // SECURITY: The method is actually not safe. We must make sure that this object is not leaked to partially-trusted code. [SecuritySafeCritical] - public override bool Equals(object obj) { + public override bool Equals(object? obj) { return obj is MetadataName name && Equals(name) || obj is MetadataNamePart part && Equals(part); } @@ -402,7 +402,7 @@ public override int GetHashCode() { // SECURITY: The method is actually not safe. We must make sure that this object is not leaked to partially-trusted code. [SecuritySafeCritical] - public override bool Equals(object obj) { + public override bool Equals(object? obj) { return obj is MetadataNamePart part && Equals(part) || obj is MetadataName name && Equals(name); } diff --git a/src/core/Microsoft.Scripting.Metadata/MetadataServices.cs b/src/core/Microsoft.Scripting.Metadata/MetadataServices.cs index 7b6455bc..c6540002 100644 --- a/src/core/Microsoft.Scripting.Metadata/MetadataServices.cs +++ b/src/core/Microsoft.Scripting.Metadata/MetadataServices.cs @@ -11,7 +11,7 @@ namespace Microsoft.Scripting.Metadata { public static class MetadataServices { // Stores metadata tables for each loaded non-dynamic assemblies. // The first module in the array is always a manifest module. - private static Dictionary _metadataCache; + private static Dictionary? _metadataCache; private static MetadataTables[] GetAsseblyMetadata(Assembly assembly) { _metadataCache ??= new Dictionary(); @@ -96,7 +96,10 @@ public static List> GetVisibleExtensionMethods(Assembl (tattrs & TypeAttributes.VisibilityMask) == TypeAttributes.NestedPublic) && (tattrs & TypeAttributes.Abstract) != 0 && (tattrs & TypeAttributes.Sealed) != 0) { - result.Add(new KeyValuePair(manifest.Module, mdef.Record.Token.Value)); + + // GetAsseblyMetadata (call above) always produces MetadataTables with elements (here: manifest) + // that have a defined (non-null) Module property. + result.Add(new KeyValuePair(manifest.Module!, mdef.Record.Token.Value)); } } } diff --git a/src/core/Microsoft.Scripting.Metadata/MetadataTables.CCI.cs b/src/core/Microsoft.Scripting.Metadata/MetadataTables.CCI.cs index 46d6a140..4190158d 100644 --- a/src/core/Microsoft.Scripting.Metadata/MetadataTables.CCI.cs +++ b/src/core/Microsoft.Scripting.Metadata/MetadataTables.CCI.cs @@ -7,6 +7,7 @@ using System.Security; using System.IO; using System.Diagnostics.Contracts; +using System.Diagnostics.CodeAnalysis; namespace Microsoft.Scripting.Metadata { [Serializable] @@ -38,7 +39,8 @@ public enum MetadataRecordType { } public partial class MetadataTables { - internal MetadataTables(MetadataImport import, string path, Module module) { + + internal MetadataTables(MetadataImport import, string? path, Module? module) { m_import = import; m_path = path; Module = module; @@ -48,7 +50,7 @@ internal MetadataTables(MetadataImport import, string path, Module module) { /// Gets the module whose metadata tables this instance represents. /// Null if the tables reflect unloaded module file. /// - public Module Module { get; } + public Module? Module { get; } public bool IsValidToken(MetadataToken token) { return m_import.IsValidToken(token); @@ -66,6 +68,15 @@ public static MetadataTables OpenFile(string path) { return new MetadataTables(CreateImport(path), path, null); } + /// + /// Opens metadata tables for a given module. + /// + /// + /// The module whose metadata tables are to be opened. Must not be null. + /// + /// A instance representing the metadata tables of the specified module. + /// Its property will be set to the specified module. + /// public static MetadataTables OpenModule(Module module) { if (module is null) { throw new ArgumentNullException(nameof(module)); @@ -268,7 +279,7 @@ public MemoryBlock Signature { /// O(log(#fields, parameters and properties with default value)). /// Returns if the field doesn't have a default value. /// - public object GetDefaultValue() { + public object? GetDefaultValue() { return m_record.Import.GetDefaultValue(m_record.Token); } @@ -277,7 +288,7 @@ public object GetDefaultValue() { /// If size is 0 the memory block will span over the rest of the data section. /// O(log(#fields with RVAs)). /// - public MemoryBlock GetData(int size) { + public MemoryBlock? GetData(int size) { if (size < 0) { throw new ArgumentOutOfRangeException(nameof(size)); } @@ -336,7 +347,7 @@ public MemoryBlock Signature { /// Returns a null reference iff the method has no body. /// If size is 0 the memory block will span over the rest of the data section. /// - public MemoryBlock GetBody() { + public MemoryBlock? GetBody() { // TODO: calculate size, decode method header and return MetadataMethodBody. uint rva = m_record.Import.MethodTable.GetRVA(m_record.Rid); return rva != 0 ? m_record.Import.RvaToMemoryBlock(rva, 0) : null; @@ -396,7 +407,7 @@ public MetadataName Name { /// O(log(#fields, parameters and properties with default value)). /// Returns if the field doesn't have a default value. /// - public object GetDefaultValue() { + public object? GetDefaultValue() { return m_record.Import.GetDefaultValue(m_record.Token); } @@ -570,7 +581,7 @@ public PropertyAccessors GetAccessors() { /// O(log(#fields, parameters and properties with default value)). /// Returns if the field doesn't have a default value. /// - public object GetDefaultValue() { + public object? GetDefaultValue() { return m_record.Import.GetDefaultValue(m_record.Token); } diff --git a/src/core/Microsoft.Scripting.Metadata/MetadataTables.cs b/src/core/Microsoft.Scripting.Metadata/MetadataTables.cs index 6edce8e1..e999abeb 100644 --- a/src/core/Microsoft.Scripting.Metadata/MetadataTables.cs +++ b/src/core/Microsoft.Scripting.Metadata/MetadataTables.cs @@ -79,7 +79,7 @@ public MetadataToken(MetadataRecordType type, int rowId) { // SECURITY: Nothing unsafe here. [SecuritySafeCritical] - public override bool Equals(object obj) => + public override bool Equals(object? obj) => obj is MetadataToken token && Equals(token); // SECURITY: Nothing unsafe here. @@ -139,16 +139,25 @@ namespace Microsoft.Scripting.Metadata { [DebuggerDisplay("{DebugView}")] public readonly partial struct MetadataRecord : IEquatable { internal readonly MetadataToken m_token; + + // Warning: MetadataTables is a reference type, but we want MetadataRecord to be a struct for perf reasons. + // This field will be null for default(MetadataRecord), e.g. element of an unpopulated array of MetadataRecord. internal readonly MetadataTables m_tables; - + internal MetadataRecord(MetadataToken token, MetadataTables tables) { - Contract.Assert(tables is not null); + Contract.Assert(tables as object is not null); m_token = token; m_tables = tables; } + /// + /// Gets the metadata tables associated with this record, provided as argument to the constructor. + /// + /// + /// Not null if the constructor has been called. + /// public MetadataTables Tables { - get { return m_tables; } + get { return m_tables ?? throw new InvalidOperationException("MetadataRecord constructor not called."); } } public MetadataToken Token { @@ -157,7 +166,7 @@ public MetadataToken Token { // SECURITY: Nothing unsafe here. [SecuritySafeCritical] - public override bool Equals(object obj) => + public override bool Equals(object? obj) => obj is MetadataRecord record && Equals(record); // SECURITY: Nothing unsafe here. @@ -249,14 +258,14 @@ public sealed partial class MetadataTables { internal readonly MetadataImport m_import; // path of an unloaded module, null for loaded modules and in-memory modules: - internal readonly string m_path; + internal readonly string? m_path; /// /// Gets the path of the module whose metadata tables this instance represents. /// Null for in-memory modules that are not backed by a file. /// /// The path is not accessible in partial trust. - public string Path { + public string? Path { get { return (Module is not null) ? Module.Assembly.Location : m_path; } } diff --git a/src/core/Microsoft.Scripting.Metadata/Microsoft.Scripting.Metadata.csproj b/src/core/Microsoft.Scripting.Metadata/Microsoft.Scripting.Metadata.csproj index 7adea98e..2a7de3cf 100644 --- a/src/core/Microsoft.Scripting.Metadata/Microsoft.Scripting.Metadata.csproj +++ b/src/core/Microsoft.Scripting.Metadata/Microsoft.Scripting.Metadata.csproj @@ -7,8 +7,9 @@ true $(DefineConstants);CCI True + enable - None + T:System.Diagnostics.CodeAnalysis.MemberNotNullAttribute;