Skip to content

Commit

Permalink
[bgen] Improve nullability detection to detect the nullability attrib…
Browse files Browse the repository at this point in the history 
…utes the C# compiler generates. Fixes #17130. (#21099)

Fixes #17130.
  • Loading branch information
@rolfbjarne
rolfbjarne authored Aug 26, 2024
1 parent 0d4defc commit 0679876
Show file tree
Hide file tree
Showing 6 changed files with 84 additions and 31 deletions.
37 changes: 37 additions & 0 deletions src/bgen/AttributeManager.cs
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
using System;
using System.Collections.Generic;
using System.Collections.ObjectModel;
using System.Linq;
using System.Reflection;
#if !NET
Expand Down Expand Up @@ -672,4 +673,40 @@ public bool IsStatic (ICustomAttributeProvider provider)
}
return false;
}

public bool IsNullable (ICustomAttributeProvider provider)
{
var attributes = GetAttributes (provider);
if (attributes is null)
return false;

// first check if any of the attributes are [NullAllowed]
foreach (var attrib in attributes) {
var attribType = attrib.GetAttributeType ();
if (attribType.Name == "NullAllowedAttribute")
return true;
}

// then check for [Nullable]
foreach (var attrib in attributes) {
var attribType = attrib.GetAttributeType ();
if (attribType.Name == "NullableAttribute") {
// https://codeblog.jonskeet.uk/2019/02/10/nullableattribute-and-c-8/
if (attrib.ConstructorArguments.Count == 1) {
var argType = attrib.ConstructorArguments [0].ArgumentType;
if (argType.Namespace == "System" && argType.Name == "Byte")
return ((byte) attrib.ConstructorArguments [0].Value) == 2;
if (argType.IsArray && argType.GetElementType ().Namespace == "System" && argType.GetElementType ().Name == "Byte") {
var valueCollection = (ReadOnlyCollection<CustomAttributeTypedArgument>) attrib.ConstructorArguments [0].Value;
// Getting complex nullability right means we'll have to completely rework how we render types.
// So don't do that for now, just look at the outermost type (the first number in the array),
// and return nullability depending on that value.
return ((byte) valueCollection [0].Value) == 2;
}
}
}
}

return false;
}
}
2 changes: 1 addition & 1 deletion src/bgen/Filters.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,7 @@ void GenerateProperties (Type type, Type? originalType = null, bool fromProtocol
nullable = true;
break;
}
if (AttributeManager.HasAttribute<NullAllowedAttribute> (p))
if (AttributeManager.IsNullable (p))
nullable = true;
print ("public {0}{1} {2} {{", ptype, nullable ? "?" : "", p.Name);
indent++;
Expand Down
58 changes: 29 additions & 29 deletions src/bgen/Generator.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -807,7 +807,7 @@ public string MarshalParameter (MethodInfo mi, ParameterInfo pi, bool null_allow
if (mai.PlainString)
return safe_name;
else {
bool allow_null = null_allowed_override || AttributeManager.HasAttribute<NullAllowedAttribute> (pi);
bool allow_null = null_allowed_override || AttributeManager.IsNullable (pi);

if (mai.ZeroCopyStringMarshal) {
if (allow_null)
Expand All @@ -828,15 +828,15 @@ public string MarshalParameter (MethodInfo mi, ParameterInfo pi, bool null_allow

MarshalType mt;
if (marshalTypes.TryGetMarshalType (pi.ParameterType, out mt)) {
if (null_allowed_override || AttributeManager.HasAttribute<NullAllowedAttribute> (pi))
if (null_allowed_override || AttributeManager.IsNullable (pi))
return safe_name + "__handle__";
return String.Format (mt.ParameterMarshal, safe_name);
}

if (pi.ParameterType.IsArray) {
//Type etype = pi.ParameterType.GetElementType ();

if (null_allowed_override || AttributeManager.HasAttribute<NullAllowedAttribute> (pi))
if (null_allowed_override || AttributeManager.IsNullable (pi))
return String.Format ("nsa_{0}.GetHandle ()", pi.Name);
return "nsa_" + pi.Name + ".Handle";
}
Expand Down Expand Up @@ -875,13 +875,13 @@ public string MarshalParameter (MethodInfo mi, ParameterInfo pi, bool null_allow
}

if (TypeManager.IsDictionaryContainerType (pi.ParameterType)) {
if (null_allowed_override || AttributeManager.HasAttribute<NullAllowedAttribute> (pi))
if (null_allowed_override || AttributeManager.IsNullable (pi))
return String.Format ("{0} is null ? NativeHandle.Zero : {0}.Dictionary.Handle", safe_name);
return safe_name + ".Dictionary.Handle";
}

if (pi.ParameterType.IsGenericParameter) {
if (null_allowed_override || AttributeManager.HasAttribute<NullAllowedAttribute> (pi))
if (null_allowed_override || AttributeManager.IsNullable (pi))
return string.Format ("{0}.GetHandle ()", safe_name);
return safe_name + ".Handle";
}
Expand All @@ -895,14 +895,14 @@ public bool ParameterNeedsNullCheck (ParameterInfo pi, MethodInfo mi, PropertyIn
if (pi.ParameterType.IsByRef)
return false;

if (AttributeManager.HasAttribute<NullAllowedAttribute> (pi))
if (AttributeManager.IsNullable (pi))
return false;

if (IsSetter (mi)) {
if (AttributeManager.HasAttribute<NullAllowedAttribute> (mi)) {
if (AttributeManager.IsNullable (mi)) {
return false;
}
if ((propInfo is not null) && AttributeManager.HasAttribute<NullAllowedAttribute> (propInfo)) {
if ((propInfo is not null) && AttributeManager.IsNullable (propInfo)) {
return false;
}
}
Expand Down Expand Up @@ -2022,7 +2022,7 @@ void GenerateEventArgsFile ()
var is_internal = prop.IsInternal (this);
var export = attrs [0];
var use_export_as_string_constant = export.ArgumentSemantic != ArgumentSemantic.None;
var null_allowed = AttributeManager.HasAttribute<NullAllowedAttribute> (prop);
var null_allowed = AttributeManager.IsNullable (prop);
var nullable_type = prop.PropertyType.IsValueType && null_allowed;
var propertyType = prop.PropertyType;
var propNamespace = prop.DeclaringType.Namespace;
Expand Down Expand Up @@ -2716,12 +2716,12 @@ public string MakeSignature (MemberInformation minfo, bool is_async, ParameterIn

var bindAsAttrib = GetBindAsAttribute (minfo.mi);
sb.Append (prefix + TypeManager.FormatType (bindAsAttrib.Type.DeclaringType, bindAsAttrib.Type));
if (!bindAsAttrib.Type.IsValueType && AttributeManager.HasAttribute<NullAllowedAttribute> (mi.ReturnParameter))
if (!bindAsAttrib.Type.IsValueType && AttributeManager.IsNullable (mi.ReturnParameter))
sb.Append ('?');
} else {
sb.Append (prefix);
sb.Append (TypeManager.FormatType (minfo.type, mi.ReturnType));
if (!mi.ReturnType.IsValueType && AttributeManager.HasAttribute<NullAllowedAttribute> (mi.ReturnParameter))
if (!mi.ReturnType.IsValueType && AttributeManager.IsNullable (mi.ReturnParameter))
sb.Append ('?');
}

Expand Down Expand Up @@ -2815,18 +2815,18 @@ public void MakeSignatureFromParameterInfo (bool comma, StringBuilder sb, Member
PrintBindAsAttribute (pi, sb);
var bt = bindAsAtt.Type;
sb.Append (TypeManager.FormatType (bt.DeclaringType, bt));
if (!bt.IsValueType && AttributeManager.HasAttribute<NullAllowedAttribute> (pi))
if (!bt.IsValueType && AttributeManager.IsNullable (pi))
sb.Append ('?');
} else {
sb.Append (TypeManager.FormatType (declaringType, parType));
// some `IntPtr` are decorated with `[NullAttribute]`
if (!parType.IsValueType) {
if (AttributeManager.HasAttribute<NullAllowedAttribute> (pi)) {
if (AttributeManager.IsNullable (pi)) {
sb.Append ('?');
} else if (pi.Position == 0 && mi is MethodInfo minfo) {
// only need to check for setter, since we wouldn't get here for a getter.
var propertyInfo = GetProperty (minfo, getter: false, setter: true);
if (AttributeManager.HasAttribute<NullAllowedAttribute> (propertyInfo))
if (AttributeManager.IsNullable (propertyInfo))
sb.Append ('?');
}
}
Expand Down Expand Up @@ -3288,7 +3288,7 @@ void GenerateTypeLowering (MethodInfo mi, bool null_allowed_override, out String

// Construct conversions
if (mai.Type == TypeCache.System_String && !mai.PlainString) {
bool probe_null = null_allowed_override || AttributeManager.HasAttribute<NullAllowedAttribute> (pi);
bool probe_null = null_allowed_override || AttributeManager.IsNullable (pi);

convs.AppendFormat (GenerateMarshalString (probe_null, !mai.ZeroCopyStringMarshal), pi.Name, pi.Name.GetSafeParamName ());
disposes.AppendFormat (GenerateDisposeString (probe_null, !mai.ZeroCopyStringMarshal), pi.Name);
Expand All @@ -3300,7 +3300,7 @@ void GenerateTypeLowering (MethodInfo mi, bool null_allowed_override, out String
} else if (HasBindAsAttribute (propInfo)) {
disposes.AppendFormat ("\nnsb_{0}?.Dispose ();", propInfo.Name);
} else if (etype == TypeCache.System_String) {
if (null_allowed_override || AttributeManager.HasAttribute<NullAllowedAttribute> (pi)) {
if (null_allowed_override || AttributeManager.IsNullable (pi)) {
convs.AppendFormat ("var nsa_{0} = {1} is null ? null : NSArray.FromStrings ({1});\n", pi.Name, pi.Name.GetSafeParamName ());
disposes.AppendFormat ("if (nsa_{0} is not null)\n\tnsa_{0}.Dispose ();\n", pi.Name);
} else {
Expand All @@ -3310,7 +3310,7 @@ void GenerateTypeLowering (MethodInfo mi, bool null_allowed_override, out String
} else if (etype == TypeCache.Selector) {
exceptions.Add (ErrorHelper.CreateError (1065, mai.Type.FullName, string.IsNullOrEmpty (pi.Name) ? $"#{pi.Position}" : pi.Name, mi.DeclaringType.FullName, mi.Name));
} else {
if (null_allowed_override || AttributeManager.HasAttribute<NullAllowedAttribute> (pi)) {
if (null_allowed_override || AttributeManager.IsNullable (pi)) {
convs.AppendFormat ("var nsa_{0} = {1} is null ? null : NSArray.FromNSObjects ({1});\n", pi.Name, pi.Name.GetSafeParamName ());
disposes.AppendFormat ("if (nsa_{0} is not null)\n\tnsa_{0}.Dispose ();\n", pi.Name);
} else {
Expand All @@ -3320,11 +3320,11 @@ void GenerateTypeLowering (MethodInfo mi, bool null_allowed_override, out String
}
} else if (mai.Type.IsSubclassOf (TypeCache.System_Delegate)) {
string trampoline_name = MakeTrampoline (pi.ParameterType).StaticName;
bool null_allowed = AttributeManager.HasAttribute<NullAllowedAttribute> (pi);
bool null_allowed = AttributeManager.IsNullable (pi);
if (!null_allowed) {
var property = GetProperty (mi);
if (property is not null)
null_allowed = AttributeManager.HasAttribute<NullAllowedAttribute> (property);
null_allowed = AttributeManager.IsNullable (property);
}
var createBlockMethod = null_allowed ? "CreateNullableBlock" : "CreateBlock";
convs.AppendFormat ("using var block_{0} = Trampolines.{1}.{3} ({2});\n", pi.Name, trampoline_name, safe_name, createBlockMethod);
Expand Down Expand Up @@ -3427,7 +3427,7 @@ void GenerateTypeLowering (MethodInfo mi, bool null_allowed_override, out String

void GenerateArgumentChecks (MethodInfo mi, bool null_allowed_override, PropertyInfo propInfo = null)
{
if (AttributeManager.HasAttribute<NullAllowedAttribute> (mi))
if (AttributeManager.IsNullable (mi))
ErrorHelper.Show (new BindingException (1118, false, mi));

foreach (var pi in mi.GetParameters ()) {
Expand Down Expand Up @@ -3945,7 +3945,7 @@ void GenerateProperty (Type type, PropertyInfo pi, List<string> instance_fields_
Type inlinedType = pi.DeclaringType == type ? null : type;
GetAccessorInfo (pi, out var getter, out var setter, out var generate_getter, out var generate_setter);

var nullable = !pi.PropertyType.IsValueType && AttributeManager.HasAttribute<NullAllowedAttribute> (pi);
var nullable = !pi.PropertyType.IsValueType && AttributeManager.IsNullable (pi);

// So we don't hide the get or set of a parent property with the same name, we need to see if we have a parent declaring the same property
PropertyInfo parentBaseType = TypeManager.GetParentTypeWithSameNamedProperty (ReflectionExtensions.GetBaseTypeAttribute (type, this), pi.Name);
Expand Down Expand Up @@ -4066,7 +4066,7 @@ void GenerateProperty (Type type, PropertyInfo pi, List<string> instance_fields_
var bindAsAttrib = GetBindAsAttribute (minfo.mi);
propertyTypeName = TypeManager.FormatType (bindAsAttrib.Type.DeclaringType, bindAsAttrib.Type);
// it remains nullable only if the BindAs type can be null (i.e. a reference type)
nullable = !bindAsAttrib.Type.IsValueType && AttributeManager.HasAttribute<NullAllowedAttribute> (pi);
nullable = !bindAsAttrib.Type.IsValueType && AttributeManager.IsNullable (pi);
} else {
propertyTypeName = TypeManager.FormatType (minfo.type, pi.PropertyType);
}
Expand Down Expand Up @@ -4178,10 +4178,10 @@ void GenerateProperty (Type type, PropertyInfo pi, List<string> instance_fields_
}
if (generate_setter) {
var ba = GetBindAttribute (setter);
bool null_allowed = AttributeManager.HasAttribute<NullAllowedAttribute> (setter);
bool null_allowed = AttributeManager.IsNullable (setter);
if (null_allowed)
ErrorHelper.Show (new BindingException (1118, false, setter));
null_allowed |= AttributeManager.HasAttribute<NullAllowedAttribute> (pi);
null_allowed |= AttributeManager.IsNullable (pi);
var not_implemented_attr = AttributeManager.GetCustomAttribute<NotImplementedAttribute> (setter);
string sel;

Expand Down Expand Up @@ -4639,7 +4639,7 @@ void GenerateMethod (MemberInformation minfo)
// we do not need the information if it's a getter (it won't change generated code)
pinfo = GetProperty (method, getter: false, setter: true);
if (pinfo is not null)
null_allowed = AttributeManager.HasAttribute<NullAllowedAttribute> (pinfo);
null_allowed = AttributeManager.IsNullable (pinfo);
}
GenerateMethodBody (minfo, minfo.Method, minfo.selector, null_allowed, null, BodyOption.None, pinfo);
if (minfo.is_autorelease) {
Expand Down Expand Up @@ -5130,7 +5130,7 @@ void GenerateProtocolTypes (Type type, string class_visibility, string TypeName,
if (minfo.is_unsafe)
mod = "unsafe ";
// IsValueType check needed for `IntPtr` signatures (which can't become `IntPtr?`)
var nullable = !pi.PropertyType.IsValueType && AttributeManager.HasAttribute<NullAllowedAttribute> (pi) ? "?" : String.Empty;
var nullable = !pi.PropertyType.IsValueType && AttributeManager.IsNullable (pi) ? "?" : String.Empty;
GetAccessorInfo (pi, out var getMethod, out var setMethod, out var generate_getter, out var generate_setter);
print ("{0}{1}{2} {3} {{", mod, TypeManager.FormatType (type, pi.PropertyType), nullable, pi.Name, generate_getter ? "get;" : string.Empty, generate_setter ? "set;" : string.Empty);
indent++;
Expand Down Expand Up @@ -6251,8 +6251,8 @@ public void Generate (Type type)
throw new BindingException (1037, true, pi.Name, type.Name);
}

if (AttributeManager.HasAttribute<NullAllowedAttribute> (pi)) {
var nonNullableProperty = protocolsThatHaveThisProp.SingleOrDefault (v => !AttributeManager.HasAttribute<NullAllowedAttribute> (v));
if (AttributeManager.IsNullable (pi)) {
var nonNullableProperty = protocolsThatHaveThisProp.SingleOrDefault (v => !AttributeManager.IsNullable (v));
if (nonNullableProperty is not null) {
// We're getting the same property from multiple interfaces, and the nullability attributes don't match.
// This results in a warning (which turn into an error because we've turned on warnaserror):
Expand Down Expand Up @@ -7195,7 +7195,7 @@ public void Generate (Type type)
foreach (var p in pars.Skip (minPars).OrderBy (p => p.Name, StringComparer.Ordinal)) {
var pt = p.ParameterType;
var bareType = pt.IsByRef ? pt.GetElementType () : pt;
var nullable = !pt.IsValueType && AttributeManager.HasAttribute<NullAllowedAttribute> (p);
var nullable = !pt.IsValueType && AttributeManager.IsNullable (p);

print ("public {0}{1} {2} {{ get; set; }}", TypeManager.RenderType (bareType), nullable ? "?" : "", GetPublicParameterName (p));
}
Expand Down
2 changes: 1 addition & 1 deletion src/bgen/Models/AsyncMethodInfo.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ public AsyncMethodInfo (Generator generator, IMemberGatherer gather, Type type,
var lastParam = cbParams.LastOrDefault ();
if (lastParam is not null && lastParam.ParameterType.Name == "NSError") {
HasNSError = true;
IsNSErrorNullable = generator.AttributeManager.HasAttribute<NullAllowedAttribute> (lastParam);
IsNSErrorNullable = generator.AttributeManager.IsNullable (lastParam);
cbParams = cbParams.DropLast ();
}

Expand Down
7 changes: 7 additions & 0 deletions tests/generator/BGenTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,13 @@ public void Bug34042 ()
BuildFile (Profile.iOS, "bug34042.cs");
}

[Test]
[TestCase (Profile.iOS)]
public void NSCopyingNullability (Profile profile)
{
var bgen = BuildFile (profile, "tests/nscopying-nullability.cs");
bgen.AssertNoWarnings ();
}

static string RenderArgument (CustomAttributeArgument arg)
{
Expand Down
9 changes: 9 additions & 0 deletions tests/generator/tests/nscopying-nullability.cs
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
using System;
using Foundation;
using ObjCRuntime;

namespace NS {
[BaseType (typeof (NSObject))]
interface MyNSCopyingWidget : INSCopying {
}
}

2 comments on commit 0679876

@vs-mobiletools-engineering-service2

This comment was marked as outdated.

Sign in to view

@vs-mobiletools-engineering-service2

This comment was marked as outdated.

Sign in to view

Please sign in to comment.