Annotate Components.Forms \ Components.Web \ Component.Server with nullable attributes #23204

pranavkm · 2020-06-21T21:34:46Z

Contributes to #5680

src/Components/Web/src/Forms/InputSelect.cs

pranavkm · 2020-06-22T15:16:49Z

src/Components/Forms/src/FieldIdentifier.cs

-                    break;
+                if (constantExpression.Value is null)
+                {
+                    throw new ArgumentException("The provided expression must evaluate to a non-null value.");


I changed the implementation a bit here. Creating a FieldIdentifier with a null value will results in an argument null exception. This provides a slightly better error experience.

pranavkm · 2020-06-22T15:20:55Z

src/Components/Server/src/Microsoft.AspNetCore.Components.Server.csproj

@@ -10,6 +10,7 @@
    <DefineConstants>$(DefineConstants);ENABLE_UNSAFE_MSGPACK;SPAN_BUILTIN;MESSAGEPACK_INTERNAL;COMPONENTS_SERVER</DefineConstants>
    <IsPackable>false</IsPackable>
    <EmbeddedFilesManifestFileName>Microsoft.Extensions.FileProviders.Embedded.Manifest.xml</EmbeddedFilesManifestFileName>
+    <Nullable>annotations</Nullable>


The vast majority of the server's code is internal implementation types and I don't want to spend time fixing that up at this point. Consequently it only has annotations on, which means you get nullability hints, but the code is not checked for correctness.

Does this means the compiler doesn't do the check within the library but that we "trust" the annotations?

Yup. The API surface is annotated, but the implementation isn't null-checked. I want to do a follow up on these things, but it'd be useful to get the user API in first.

pranavkm · 2020-06-22T15:21:30Z

src/Components/Server/src/DependencyInjection/ServerSideBlazorBuilderExtensions.cs

            {
-                builder.Services.Configure<HubOptions<ComponentHub>>(configure);
+                throw new ArgumentNullException(nameof(configure));


This matches the behavior of AddCircuitOptions. Seems very strange to call this and not have it do anything by passing in a null.

I think this follows a specific pattern, I believe we do it this way across all asp.net core. I would suggest we keep it as is for consistency.

javiercn · 2020-06-23T14:49:25Z

src/Components/Web/src/BindInputElementAttribute.cs

@@ -25,7 +25,7 @@ public sealed class BindInputElementAttribute : Attribute
        /// <param name="format">
        /// An optional format to use when converting values. 
        /// </param>
-        public BindInputElementAttribute(string type, string suffix, string valueAttribute, string changeAttribute, bool isInvariantCulture, string format)
+        public BindInputElementAttribute(string? type, string? suffix, string? valueAttribute, string? changeAttribute, bool isInvariantCulture, string? format)


What does it mean for the type to be null? Isn't type required? Or does it default to "text"?

In cases like these, we should consider applying defaults to the properties when we receive null-values and avoid having the code that uses these things from having to deal with null values.

aspnetcore/src/Components/Web/src/Web/BindAttributes.cs

Line 16 in 602cb1d

[BindInputElement(null, null, "value", "onchange", isInvariantCulture: false, format: null)]

javiercn · 2020-06-23T14:51:38Z

src/Components/Web/src/Forms/InputBase.cs

+        [AllowNull]
+        [MaybeNull]
+        [Parameter]
+        public TValue Value { get; set; } = default;


Is it because TValue is a generic parameter that we can't do something like TValue? Value?

Yeah, you can only apply nullable to generics if you define them to be either reference or value types. Opaque types, not so much. This says even if you declare an InputText<string> the value could be null, either because it was initialized as such or the value being assigned to it cannot be guaranteed to be non-null.

javiercn · 2020-06-23T15:00:29Z

src/Components/Web/src/Routing/NavLink.cs


        /// <summary>
        /// Gets or sets a collection of additional attributes that will be added to the generated
        /// <c>a</c> element.
        /// </summary>
        [Parameter(CaptureUnmatchedValues = true)]
-        public IReadOnlyDictionary<string, object> AdditionalAttributes { get; set; }
+        public IReadOnlyDictionary<string, object?>? AdditionalAttributes { get; set; }


Do we use this annotation on all AdditionalAttributes parameters? I think I've seen one that is different, but I can't find it now. Just double check that these are consistent across different components.

Does the current annotation mean that the dictionary and its contents can be both null? (So you have a valid entry with a null value) What's the scenario for that last case?

Do we use this annotation on all AdditionalAttributes parameters?

Yup, the compiler should complain if we missed some.

Does the current annotation mean that the dictionary and its contents can be both null? (So you have a valid entry with a null value) What's the scenario for that last case?

I went back and checked, and I don't think there's a scenario for the values inside the dictionary being null. I can change that. The dictionary itself would be null.

Ok, I see what happened here. RenderTreeBuilder allows null-valued attributes here: https://github.com/dotnet/aspnetcore/blob/master/src/Components/Components/src/Rendering/RenderTreeBuilder.cs#L329-L333. Making https://github.com/dotnet/aspnetcore/blob/master/src/Components/Components/src/Rendering/RenderTreeBuilder.cs#L405 allow nullable attributes prevents these properties from having a non-nullable value.

It's a bit of a compromise, but we'll declare AddMultipleAttributes to accept non-null values. ComponentProperties already produces a non-null valued dictionary, so we should be good.

javiercn

LGTM, left a couple of questions, but it's good to go once we clarify things.

Contributes to #5680

ghost · 2020-06-25T17:34:27Z

Hello @pranavkm!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

…llable attributes (#23204) * Annotate Components.Forms \ Components.Web with nullable attributes Contributes to #5680 * Fixup * Fixup rebase * Undo nullable * Fixup

…llable attributes (#23204) (#23355) * Annotate Components.Forms \ Components.Web with nullable attributes Contributes to #5680 * Fixup * Fixup rebase * Undo nullable * Fixup

ghost added the area-blazor Includes: Blazor, Razor Components label Jun 21, 2020

pranavkm changed the title ~~Annotate Components.Forms \ Components.Web with nullable attributes~~ Annotate Components.Forms \ Components.Web \ Component.Server with nullable attributes Jun 21, 2020

pranavkm marked this pull request as ready for review June 22, 2020 15:06

pranavkm requested a review from SteveSandersonMS as a code owner June 22, 2020 15:06

pranavkm force-pushed the prkrishn/nullable-components-forms branch from a28f1a8 to 454ff06 Compare June 22, 2020 15:13

pranavkm requested a review from a team June 22, 2020 15:13

pranavkm commented Jun 22, 2020

View reviewed changes

pranavkm force-pushed the prkrishn/nullable-components-forms branch from 454ff06 to 0763580 Compare June 22, 2020 19:07

javiercn reviewed Jun 23, 2020

View reviewed changes

pranavkm added 5 commits June 25, 2020 08:26

Annotate Components.Forms \ Components.Web with nullable attributes

e1c7a71

Contributes to #5680

Fixup

9f50501

Fixup rebase

bc444a7

Undo nullable

b540b53

Fixup

625ef7c

pranavkm force-pushed the prkrishn/nullable-components-forms branch from 0763580 to 625ef7c Compare June 25, 2020 16:03

pranavkm requested a review from a team June 25, 2020 16:07

pranavkm added the auto-merge label Jun 25, 2020

pranavkm added this to the 5.0.0-preview8 milestone Jun 25, 2020

javiercn approved these changes Jun 25, 2020

View reviewed changes

ghost merged commit c42d1a3 into master Jun 25, 2020

ghost deleted the prkrishn/nullable-components-forms branch June 25, 2020 17:36

pranavkm mentioned this pull request Jun 25, 2020

Annotate Components.Forms \ Components.Web \ Component.Server with nullable attributes #23355

Merged

ScarletKuro mentioned this pull request Jul 15, 2024

MudDialog: Add nullable annotation MudBlazor/MudBlazor#9146

Merged

3 tasks

This pull request was closed.

Annotate Components.Forms \ Components.Web \ Component.Server with nullable attributes #23204

Annotate Components.Forms \ Components.Web \ Component.Server with nullable attributes #23204

Uh oh!

Conversation

pranavkm commented Jun 21, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javiercn left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Jun 25, 2020

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Uh oh!

Uh oh!

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.