From 5ad7ee0cead5d0b94a08f6afa1845952cae8da63 Mon Sep 17 00:00:00 2001 From: Daniel Grunwald Date: Sun, 19 Apr 2020 02:05:59 +0200 Subject: [PATCH] Fix #1629: Add support for `[module: NullablePublicOnly]` If this attribute is in use, private/internal members lack nullability annotations. Previously in such cases, we ended up inheriting the nullability from the `[NullableContext]`, which could cause us to display a misleading nullability for primary methods. In debug builds, it could also trigger an assertion when trying to apply the "nullable reference type" marking to to value types. Of note is that properties and events are a special case: they do not explicitly store Accessibility in metadata. For properties computing the accessibility requires decoding the signature (to find overridden base properties). So these two only check the declaring type's accessibility instead; private properties may still carry nullability despite `[NullablePublicOnly]`. However, the property accessors won't store nullability, so we need to read the `returnTypeAttributes` from the property itself. --- .../TestCases/Pretty/NullableRefTypes.cs | 12 +++++ .../TypeSystem/Accessibility.cs | 13 +++++ .../Implementation/KnownAttributes.cs | 2 + .../Implementation/MetadataEvent.cs | 5 +- .../Implementation/MetadataField.cs | 2 +- .../Implementation/MetadataMethod.cs | 26 ++++++---- .../Implementation/MetadataProperty.cs | 18 +++++-- .../Implementation/MetadataTypeParameter.cs | 2 +- .../TypeSystem/MetadataModule.cs | 50 ++++++++++++++++++- 9 files changed, 113 insertions(+), 17 deletions(-) diff --git a/ICSharpCode.Decompiler.Tests/TestCases/Pretty/NullableRefTypes.cs b/ICSharpCode.Decompiler.Tests/TestCases/Pretty/NullableRefTypes.cs index baf5e6184..1d808ec6a 100644 --- a/ICSharpCode.Decompiler.Tests/TestCases/Pretty/NullableRefTypes.cs +++ b/ICSharpCode.Decompiler.Tests/TestCases/Pretty/NullableRefTypes.cs @@ -19,6 +19,18 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Pretty private string[]?[] field_array; private Dictionary<(string, string?), (int, string[]?, string?[])> field_complex; + public (string A, dynamic? B) PropertyNamedTuple { + get { + throw new NotImplementedException(); + } + } + + public (string A, dynamic? B) this[(dynamic? C, string D) weirdIndexer] { + get { + throw new NotImplementedException(); + } + } + public int GetLength1(string[] arr) { return field_string.Length + arr.Length; diff --git a/ICSharpCode.Decompiler/TypeSystem/Accessibility.cs b/ICSharpCode.Decompiler/TypeSystem/Accessibility.cs index 9cee2d55e..25091ddc8 100644 --- a/ICSharpCode.Decompiler/TypeSystem/Accessibility.cs +++ b/ICSharpCode.Decompiler/TypeSystem/Accessibility.cs @@ -115,5 +115,18 @@ namespace ICSharpCode.Decompiler.TypeSystem return b; } } + + /// + /// Gets the effective accessibility of the entity. + /// For example, a public method in an internal class returns "internal". + /// + public static Accessibility EffectiveAccessibility(this IEntity entity) + { + Accessibility accessibility = entity.Accessibility; + for (ITypeDefinition typeDef = entity.DeclaringTypeDefinition; typeDef != null; typeDef = typeDef.DeclaringTypeDefinition) { + accessibility = Intersect(accessibility, typeDef.Accessibility); + } + return accessibility; + } } } diff --git a/ICSharpCode.Decompiler/TypeSystem/Implementation/KnownAttributes.cs b/ICSharpCode.Decompiler/TypeSystem/Implementation/KnownAttributes.cs index 872f31af0..0e7b33a7b 100644 --- a/ICSharpCode.Decompiler/TypeSystem/Implementation/KnownAttributes.cs +++ b/ICSharpCode.Decompiler/TypeSystem/Implementation/KnownAttributes.cs @@ -41,6 +41,7 @@ namespace ICSharpCode.Decompiler.TypeSystem TupleElementNames, Nullable, NullableContext, + NullablePublicOnly, Conditional, Obsolete, IsReadOnly, @@ -111,6 +112,7 @@ namespace ICSharpCode.Decompiler.TypeSystem new TopLevelTypeName("System.Runtime.CompilerServices", nameof(TupleElementNamesAttribute)), new TopLevelTypeName("System.Runtime.CompilerServices", "NullableAttribute"), new TopLevelTypeName("System.Runtime.CompilerServices", "NullableContextAttribute"), + new TopLevelTypeName("System.Runtime.CompilerServices", "NullablePublicOnlyAttribute"), new TopLevelTypeName("System.Diagnostics", nameof(ConditionalAttribute)), new TopLevelTypeName("System", nameof(ObsoleteAttribute)), new TopLevelTypeName("System.Runtime.CompilerServices", "IsReadOnlyAttribute"), diff --git a/ICSharpCode.Decompiler/TypeSystem/Implementation/MetadataEvent.cs b/ICSharpCode.Decompiler/TypeSystem/Implementation/MetadataEvent.cs index e85a559d3..cd59290b7 100644 --- a/ICSharpCode.Decompiler/TypeSystem/Implementation/MetadataEvent.cs +++ b/ICSharpCode.Decompiler/TypeSystem/Implementation/MetadataEvent.cs @@ -82,7 +82,10 @@ namespace ICSharpCode.Decompiler.TypeSystem.Implementation var declaringTypeDef = DeclaringTypeDefinition; var context = new GenericContext(declaringTypeDef?.TypeParameters); var nullableContext = declaringTypeDef?.NullableContext ?? Nullability.Oblivious; - returnType = module.ResolveType(ev.Type, context, ev.GetCustomAttributes(), nullableContext); + // The event does not have explicit accessibility in metadata, so use its + // containing type to determine whether nullability applies to this type. + var typeOptions = module.OptionsForEntity(declaringTypeDef); + returnType = module.ResolveType(ev.Type, context, typeOptions, ev.GetCustomAttributes(), nullableContext); return LazyInit.GetOrSet(ref this.returnType, returnType); } } diff --git a/ICSharpCode.Decompiler/TypeSystem/Implementation/MetadataField.cs b/ICSharpCode.Decompiler/TypeSystem/Implementation/MetadataField.cs index 18f513939..0e24e62c2 100644 --- a/ICSharpCode.Decompiler/TypeSystem/Implementation/MetadataField.cs +++ b/ICSharpCode.Decompiler/TypeSystem/Implementation/MetadataField.cs @@ -186,7 +186,7 @@ namespace ICSharpCode.Decompiler.TypeSystem.Implementation ty = mod.ElementType; } ty = ApplyAttributeTypeVisitor.ApplyAttributesToType(ty, Compilation, - fieldDef.GetCustomAttributes(), metadata, module.TypeSystemOptions, + fieldDef.GetCustomAttributes(), metadata, module.OptionsForEntity(this), DeclaringTypeDefinition?.NullableContext ?? Nullability.Oblivious); } catch (BadImageFormatException) { ty = SpecialType.UnknownType; diff --git a/ICSharpCode.Decompiler/TypeSystem/Implementation/MetadataMethod.cs b/ICSharpCode.Decompiler/TypeSystem/Implementation/MetadataMethod.cs index 87f7eb4c9..ab02b8252 100644 --- a/ICSharpCode.Decompiler/TypeSystem/Implementation/MetadataMethod.cs +++ b/ICSharpCode.Decompiler/TypeSystem/Implementation/MetadataMethod.cs @@ -166,7 +166,7 @@ namespace ICSharpCode.Decompiler.TypeSystem.Implementation try { var nullableContext = methodDef.GetCustomAttributes().GetNullableContext(module.metadata) ?? DeclaringTypeDefinition.NullableContext; var signature = methodDef.DecodeSignature(module.TypeProvider, genericContext); - (returnType, parameters) = DecodeSignature(module, this, signature, methodDef.GetParameters(), nullableContext); + (returnType, parameters) = DecodeSignature(module, this, signature, methodDef.GetParameters(), nullableContext, module.OptionsForEntity(this)); } catch (BadImageFormatException) { returnType = SpecialType.UnknownType; parameters = Empty.Array; @@ -175,11 +175,13 @@ namespace ICSharpCode.Decompiler.TypeSystem.Implementation LazyInit.GetOrSet(ref this.parameters, parameters); } - internal static (IType, IParameter[]) DecodeSignature(MetadataModule module, IParameterizedMember owner, MethodSignature signature, ParameterHandleCollection? parameterHandles, Nullability nullableContext) + internal static (IType, IParameter[]) DecodeSignature(MetadataModule module, IParameterizedMember owner, + MethodSignature signature, ParameterHandleCollection? parameterHandles, + Nullability nullableContext, TypeSystemOptions typeSystemOptions, + CustomAttributeHandleCollection? returnTypeAttributes = null) { var metadata = module.metadata; int i = 0; - CustomAttributeHandleCollection? returnTypeAttributes = null; IParameter[] parameters = new IParameter[signature.RequiredParameterCount + (signature.Header.CallingConvention == SignatureCallingConvention.VarArgs ? 1 : 0)]; IType parameterType; @@ -187,8 +189,14 @@ namespace ICSharpCode.Decompiler.TypeSystem.Implementation foreach (var parameterHandle in parameterHandles) { var par = metadata.GetParameter(parameterHandle); if (par.SequenceNumber == 0) { - // "parameter" holds return type attributes - returnTypeAttributes = par.GetCustomAttributes(); + // "parameter" holds return type attributes. + // Note: for properties, the attributes normally stored on a method's return type + // are instead stored as normal attributes on the property. + // So MetadataProperty provides a non-null value for returnTypeAttributes, + // which then should be preferred over the attributes on the accessor's parameters. + if (returnTypeAttributes == null) { + returnTypeAttributes = par.GetCustomAttributes(); + } } else if (par.SequenceNumber > 0 && i < signature.RequiredParameterCount) { // "Successive rows of the Param table that are owned by the same method shall be // ordered by increasing Sequence value - although gaps in the sequence are allowed" @@ -196,14 +204,14 @@ namespace ICSharpCode.Decompiler.TypeSystem.Implementation // Fill gaps in the sequence with non-metadata parameters: while (i < par.SequenceNumber - 1) { parameterType = ApplyAttributeTypeVisitor.ApplyAttributesToType( - signature.ParameterTypes[i], module.Compilation, null, metadata, module.TypeSystemOptions, nullableContext); + signature.ParameterTypes[i], module.Compilation, null, metadata, typeSystemOptions, nullableContext); parameters[i] = new DefaultParameter(parameterType, name: string.Empty, owner, referenceKind: parameterType.Kind == TypeKind.ByReference ? ReferenceKind.Ref : ReferenceKind.None); i++; } parameterType = ApplyAttributeTypeVisitor.ApplyAttributesToType( signature.ParameterTypes[i], module.Compilation, - par.GetCustomAttributes(), metadata, module.TypeSystemOptions, nullableContext); + par.GetCustomAttributes(), metadata, typeSystemOptions, nullableContext); parameters[i] = new MetadataParameter(module, owner, parameterType, parameterHandle); i++; } @@ -211,7 +219,7 @@ namespace ICSharpCode.Decompiler.TypeSystem.Implementation } while (i < signature.RequiredParameterCount) { parameterType = ApplyAttributeTypeVisitor.ApplyAttributesToType( - signature.ParameterTypes[i], module.Compilation, null, metadata, module.TypeSystemOptions, nullableContext); + signature.ParameterTypes[i], module.Compilation, null, metadata, typeSystemOptions, nullableContext); parameters[i] = new DefaultParameter(parameterType, name: string.Empty, owner, referenceKind: parameterType.Kind == TypeKind.ByReference ? ReferenceKind.Ref : ReferenceKind.None); i++; @@ -222,7 +230,7 @@ namespace ICSharpCode.Decompiler.TypeSystem.Implementation } Debug.Assert(i == parameters.Length); var returnType = ApplyAttributeTypeVisitor.ApplyAttributesToType(signature.ReturnType, - module.Compilation, returnTypeAttributes, metadata, module.TypeSystemOptions, nullableContext); + module.Compilation, returnTypeAttributes, metadata, typeSystemOptions, nullableContext); return (returnType, parameters); } #endregion diff --git a/ICSharpCode.Decompiler/TypeSystem/Implementation/MetadataProperty.cs b/ICSharpCode.Decompiler/TypeSystem/Implementation/MetadataProperty.cs index 7ba165859..83596cb51 100644 --- a/ICSharpCode.Decompiler/TypeSystem/Implementation/MetadataProperty.cs +++ b/ICSharpCode.Decompiler/TypeSystem/Implementation/MetadataProperty.cs @@ -129,23 +129,33 @@ namespace ICSharpCode.Decompiler.TypeSystem.Implementation try { var signature = propertyDef.DecodeSignature(module.TypeProvider, genericContext); var accessors = propertyDef.GetAccessors(); + var declTypeDef = this.DeclaringTypeDefinition; ParameterHandleCollection? parameterHandles; Nullability nullableContext; if (!accessors.Getter.IsNil) { var getter = module.metadata.GetMethodDefinition(accessors.Getter); parameterHandles = getter.GetParameters(); nullableContext = getter.GetCustomAttributes().GetNullableContext(module.metadata) - ?? DeclaringTypeDefinition?.NullableContext ?? Nullability.Oblivious; + ?? declTypeDef?.NullableContext ?? Nullability.Oblivious; } else if (!accessors.Setter.IsNil) { var setter = module.metadata.GetMethodDefinition(accessors.Setter); parameterHandles = setter.GetParameters(); nullableContext = setter.GetCustomAttributes().GetNullableContext(module.metadata) - ?? DeclaringTypeDefinition?.NullableContext ?? Nullability.Oblivious; + ?? declTypeDef?.NullableContext ?? Nullability.Oblivious; } else { parameterHandles = null; - nullableContext = DeclaringTypeDefinition?.NullableContext ?? Nullability.Oblivious; + nullableContext = declTypeDef?.NullableContext ?? Nullability.Oblivious; } - (returnType, parameters) = MetadataMethod.DecodeSignature(module, this, signature, parameterHandles, nullableContext); + // We call OptionsForEntity() for the declaring type, not the property itself, + // because the property's accessibilty isn't stored in metadata but computed. + // Otherwise we'd get infinite recursion, because computing the accessibility + // requires decoding the signature for the GetBaseMembers() call. + // Roslyn uses the same workaround (see the NullableTypeDecoder.TransformType + // call in PEPropertySymbol). + var typeOptions = module.OptionsForEntity(declTypeDef); + (returnType, parameters) = MetadataMethod.DecodeSignature(module, this, signature, + parameterHandles, nullableContext, typeOptions, + returnTypeAttributes: propertyDef.GetCustomAttributes()); } catch (BadImageFormatException) { returnType = SpecialType.UnknownType; parameters = Empty.Array; diff --git a/ICSharpCode.Decompiler/TypeSystem/Implementation/MetadataTypeParameter.cs b/ICSharpCode.Decompiler/TypeSystem/Implementation/MetadataTypeParameter.cs index 73b72d147..aef3b2db4 100644 --- a/ICSharpCode.Decompiler/TypeSystem/Implementation/MetadataTypeParameter.cs +++ b/ICSharpCode.Decompiler/TypeSystem/Implementation/MetadataTypeParameter.cs @@ -145,7 +145,7 @@ namespace ICSharpCode.Decompiler.TypeSystem.Implementation Nullability LoadNullabilityConstraint() { - if ((module.TypeSystemOptions & TypeSystemOptions.NullabilityAnnotations) == 0) + if (!module.ShouldDecodeNullableAttributes(Owner)) return Nullability.Oblivious; var metadata = module.metadata; diff --git a/ICSharpCode.Decompiler/TypeSystem/MetadataModule.cs b/ICSharpCode.Decompiler/TypeSystem/MetadataModule.cs index 1fb75d51d..5d8d4bd3a 100644 --- a/ICSharpCode.Decompiler/TypeSystem/MetadataModule.cs +++ b/ICSharpCode.Decompiler/TypeSystem/MetadataModule.cs @@ -67,7 +67,9 @@ namespace ICSharpCode.Decompiler.TypeSystem this.AssemblyName = metadata.GetString(moddef.Name); this.FullAssemblyName = this.AssemblyName; } - this.NullableContext = metadata.GetModuleDefinition().GetCustomAttributes().GetNullableContext(metadata) ?? Nullability.Oblivious; + var customAttrs = metadata.GetModuleDefinition().GetCustomAttributes(); + this.NullableContext = customAttrs.GetNullableContext(metadata) ?? Nullability.Oblivious; + this.minAccessibilityForNRT = FindMinimumAccessibilityForNRT(metadata, customAttrs); this.rootNamespace = new MetadataNamespace(this, null, string.Empty, metadata.GetNamespaceDefinitionRoot()); if (!options.HasFlag(TypeSystemOptions.Uncached)) { @@ -739,5 +741,51 @@ namespace ICSharpCode.Decompiler.TypeSystem || att == MethodAttributes.FamORAssem; } #endregion + + #region Nullability Reference Type Support + readonly Accessibility minAccessibilityForNRT; + + static Accessibility FindMinimumAccessibilityForNRT(MetadataReader metadata, CustomAttributeHandleCollection customAttributes) + { + // Determine the minimum effective accessibility an entity must have, so that the metadata stores the nullability for its type. + foreach (var handle in customAttributes) { + var customAttribute = metadata.GetCustomAttribute(handle); + if (customAttribute.IsKnownAttribute(metadata, KnownAttribute.NullablePublicOnly)) { + CustomAttributeValue value; + try { + value = customAttribute.DecodeValue(Metadata.MetadataExtensions.MinimalAttributeTypeProvider); + } catch (BadImageFormatException) { + continue; + } catch (EnumUnderlyingTypeResolveException) { + continue; + } + if (value.FixedArguments.Length == 1 && value.FixedArguments[0].Value is bool includesInternals) { + return includesInternals ? Accessibility.ProtectedAndInternal : Accessibility.Protected; + } + } + } + return Accessibility.None; + } + + internal bool ShouldDecodeNullableAttributes(IEntity entity) + { + if ((options & TypeSystemOptions.NullabilityAnnotations) == 0) + return false; + if (minAccessibilityForNRT == Accessibility.None || entity == null) + return true; + return minAccessibilityForNRT.LessThanOrEqual(entity.EffectiveAccessibility()); + } + + internal TypeSystemOptions OptionsForEntity(IEntity entity) + { + var opt = this.options; + if ((opt & TypeSystemOptions.NullabilityAnnotations) != 0) { + if (!ShouldDecodeNullableAttributes(entity)) { + opt &= ~TypeSystemOptions.NullabilityAnnotations; + } + } + return opt; + } + #endregion } }