Annotate System.ComponentModel.DataAnnotations for nullability

[roji]

By default, nullability warnings/errors won't appear - use this build hack to temporarily target .NET Core and generate warnings.

[jeffhandley]

I was tempted to suggest trying to make ValidationAttribute not nullable here since I don't think we'd have any uses ourselves where it would be null, but with it being a public constructor and with no references to it other than the property setter, this is the right call.

[roji]

Agreed... I'm generally trying to be as conservative as possible - if there's a public code path which results in something being nullable (and in the type being usable), I tend to mark it nullable, even if it doesn't make a lot of sense.

[krwq]

One thing to consider: for arguments we can always relax the constraint and allow null in vNext (and user have an option to pass it anyway with !), if we mark it as nullable then constraining it will be a breaking change for vNext (for return values relaxation would be making it non-nullable)

[roji]

@krwq agreed about the general logic, but do you feel that going non-nullable is the right choice for this specific case? For these cases where we're annotating legacy APIs with a very long history, there's non-negligible chance of users using these APIs with null, and if the implementation supports that correctly, my tendency is to annotate the actual, supported state of affairs (at least that's the guidance I've received).

Another point is that an overly-relaxed annotation of a parameter as nullable only has the consequence of not flagging a warning in user code which passes null; again, as long as the API handles that correctly that may not be a big concern.

[ghost]

Tagging subscribers to this area: @ajcvickers
Notify danmosemsft if you want to be subscribed.

[krwq]

Can't put a comment there but in here:
public override ICustomTypeDescriptor GetTypeDescriptor(Type objectType, object instance)

should object be nullable (I'm not familiar with the API just shape and name of the API suggest like there should be no issue with providing a type but not providing instance and possibly also reverse)

[roji]

Am also not familiar with this API, so am waiting for it to be annotated by someone who hopefully does :)

[krwq]

should AssociatedMetadataType be nullable? I'm seeing null check in GetAttributes()

[roji]

Definitely seems so - I left AssociatedMetadataTypeTypeDescriptor and AssociatedMetadataTypeTypeDescriptionProvider unannotated (and disabled) because of the dependency on System.ComponentModel.TypeDescriptionProvider. The idea is to return to these soon once that's done.

[krwq]

should this be Func<object?, object?>? (with NotNullWhenNotNull or similar)

[roji]

The conversion function isn't user-provided - it's generated internally in SetupConversion based on user-provided minimum and maximum values, and except for the int/double special cases (where there's no null anyway), it will NRE on null inputs (it calls GetType in inputs to see if it needs to actual perform conversions). It's "protected" against this in IsValid, where value is checked for null explicitly, and invocation is skipped entirely - so the implementation never seems to pass null to the function. So it seems right for the conversion parameter to be non-nullable.

However, it's technically possible (though probably weird) for the user-provided conversion function to return null, am changing that.

Re NotNullWhenNotNull, I don't see anything explicit showing a relationship between the nullability of the input and the output of the conversion function - the user could conceivably decide to convert empty strings to null, for example, for the purpose of this attribute.

Let me know what you think of the above.

[krwq]

cc: @GrabYourPitchforks for GetHashCode below

[roji]

Anything in particular we need to do here?

[krwq]

might make sense to just mark it as non-nullable and = null! to suppress warnings

[roji]

It's possible, but as it's late-initialized in at least some scenarios (via SetupResourceAccessor, where there's an actual null check) it makes a bit more sense to me to annotate it as nullable... Let me know if you feel otherwise.

[jeffhandley]

I left a review comment where you had a code comment suggesting something might be a big, but it was by design. Theoretically a big exists there, but it's intentional and maybe could use a different comment to reflect that.

Other than updating that comment appropriately, approved.

Thanks!!

[roji]

Remaining build failures appear to be unrelated MacOS infrastructure-related, merging.

[pentp]

ValidationAttribute.IsValid should allow null returns because ValidationResult.Success is null (and correctly annotated).

[stephentoub]

@jeffhandley?

[roji]

This is a discrepancy between ref and src that has already been fixed in master (#41731), but apparently not yet in release/5.0.0-rc2... am not sure what the backporting status of this... /cc @buyaa-n @jeffhandley

[jeffhandley]

The RC2 port PR (#41845) was already merged. I'm checking to see if we somehow missed some commits in the port.

[jeffhandley]

The port PR has everything that #41731 had, and I see the ref assembly source contains ValidationResult? IsValid.

https://github.com/dotnet/runtime/blob/release/5.0-rc2/src/libraries/System.ComponentModel.Annotations/ref/System.ComponentModel.Annotations.cs#L295

    public abstract partial class ValidationAttribute : System.Attribute
    {
        protected ValidationAttribute() { }
        protected ValidationAttribute(System.Func<string> errorMessageAccessor) { }
        protected ValidationAttribute(string errorMessage) { }
        public string? ErrorMessage { get { throw null; } set { } }
        public string? ErrorMessageResourceName { get { throw null; } set { } }
        public System.Type? ErrorMessageResourceType { get { throw null; } set { } }
        protected string ErrorMessageString { get { throw null; } }
        public virtual bool RequiresValidationContext { get { throw null; } }
        public virtual string FormatErrorMessage(string name) { throw null; }
        public System.ComponentModel.DataAnnotations.ValidationResult? GetValidationResult(object? value, System.ComponentModel.DataAnnotations.ValidationContext validationContext) { throw null; }
        public virtual bool IsValid(object? value) { throw null; }
        protected virtual System.ComponentModel.DataAnnotations.ValidationResult? IsValid(object? value, System.ComponentModel.DataAnnotations.ValidationContext validationContext) { throw null; }
        public void Validate(object? value, System.ComponentModel.DataAnnotations.ValidationContext validationContext) { }
        public void Validate(object? value, string name) { }
    }

@pentp Can you let me know where you were seeing that ValidationResult.IsValid wasn't allowing null?

[pentp]

I was testing out one of our applications on RC1, so that's why I noticed it. Good to know that it's already fixed in RC2.

[jeffhandley]

@pentp Thank you! And thanks for reporting it. RC2 builds are available here if you're interested in verifying that you see the expected behavior with RC2.