Annotate System.ComponentModel.Primitives for nullable #41185
Conversation
| { | ||
| ISite s = _site; | ||
| ISite? s = _site; |
There was a problem hiding this comment.
I don't think ToString could be returning null here. You should be able to change return type back to string
There was a problem hiding this comment.
I agree, just because of Type.FullName return nullable, not sure if i should suppress that and make it non null
There was a problem hiding this comment.
GetType().FullName can definitely return null if the type has GenericTypeParameters: https://source.dot.net/#System.Private.CoreLib/src/System/RuntimeType.CoreCLR.cs,1491 -- however since this type is not generic it will never be null, so I think we should just use ! on the return.
| @@ -39,7 +39,7 @@ public partial class CategoryAttribute : System.Attribute | |||
| public static System.ComponentModel.CategoryAttribute Layout { get { throw null; } } | |||
| public static System.ComponentModel.CategoryAttribute Mouse { get { throw null; } } | |||
| public static System.ComponentModel.CategoryAttribute WindowStyle { get { throw null; } } | |||
| public override bool Equals(object obj) { throw null; } | |||
| public override bool Equals(object? obj) { throw null; } | |||
| public override int GetHashCode() { throw null; } | |||
| protected virtual string GetLocalizedString(string value) { throw null; } | |||
There was a problem hiding this comment.
GetLocalizedString should return a nullable string.
There was a problem hiding this comment.
I was thinking same at first, but then noticed the only method called here returning not null even suppresed possible null return, maybe that one annotated incorrectly? From SR.sc:
internal static string GetResourceString(string resourceKey, string? defaultString = null)
{
if (UsingResourceKeys())
{
return defaultString ?? resourceKey;
}
string? resourceString = null;
try
{
resourceString = ResourceManager.GetString(resourceKey);
}
catch (MissingManifestResourceException) { }
if (defaultString != null && resourceKey.Equals(resourceString))
{
return defaultString;
}
return resourceString!; // only null if missing resources
}There was a problem hiding this comment.
That's the base implementation, but it's virtual, so derived types could be overriding it and returning null. Since it's protected, really the only code that might call this method is this class (technically a derived one could as well, but that's unlikely), and it's already checking to see if the method returns null, which both suggests that was part of the design and means that there's little harm in allowing null.
| protected virtual object GetService(System.Type service) { throw null; } | ||
| public override string ToString() { throw null; } | ||
| protected virtual object? GetService(System.Type service) { throw null; } | ||
| public override string? ToString() { throw null; } |
There was a problem hiding this comment.
I'm curious why you made this nullable. Do you know of a case where a Component-derived type returns null from ToString?
There was a problem hiding this comment.
Just because of Type.FullName, got feedback from Maryam and Santi about that #41185 (comment) and made it not nullable, but seems I didn't pushed that change yet
| void Add(System.ComponentModel.IComponent component, string name); | ||
| void Remove(System.ComponentModel.IComponent component); | ||
| void Add(System.ComponentModel.IComponent? component); | ||
| void Add(System.ComponentModel.IComponent? component, string name); |
There was a problem hiding this comment.
name should be a nullable string
| { | ||
| if (obj == this) | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| BrowsableAttribute other = obj as BrowsableAttribute; | ||
| BrowsableAttribute? other = obj as BrowsableAttribute; | ||
| return other?.Browsable == Browsable; |
There was a problem hiding this comment.
Nit: I suggest we simplify this to:
public override bool Equals(object? obj) =>
obj is BrowsableAttribute other && other.Browsable == Browsable;
| { | ||
| if (obj == this) | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| CategoryAttribute other = obj as CategoryAttribute; | ||
| CategoryAttribute? other = obj as CategoryAttribute; | ||
| return other != null && Category == other.Category; |
| { | ||
| if (obj == this) | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| DescriptionAttribute other = obj as DescriptionAttribute; | ||
| DescriptionAttribute? other = obj as DescriptionAttribute; | ||
| return other != null && other.Description == Description; |
| { | ||
| if (obj == this) | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| DesignOnlyAttribute other = obj as DesignOnlyAttribute; | ||
| DesignOnlyAttribute? other = obj as DesignOnlyAttribute; | ||
| return other?.IsDesignOnly == IsDesignOnly; |
There was a problem hiding this comment.
Ditto; I'll stop commenting on these.
|
@stephentoub addressed feedback please re review |
|
As all comments addressed and not receiving more am gonna merge this PR |
…#41185) * Annotate System.ComponentModel.Primitives for nullable Commit migrated from dotnet/corefx@da147ec
Contributes to #40623
cc: @dotnet/nullablefc