Add nullability to Localization #22323

pranavkm · 2020-05-28T18:10:03Z

Changes to support nullable for ns2.0
Change ref assembly project to use nullable context

* Changes to support nullable for ns2.0 * Change ref assembly project to use nullable context

ghost · 2020-05-28T23:08:11Z

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

pranavkm · 2020-05-28T23:12:24Z

eng/targets/CSharp.Common.targets

    </ItemGroup>
  </Target>

+  <Choose>


FYI @dotnet/aspnet-build

dougbu

Pending API review of course…

dougbu · 2020-05-28T23:15:22Z

eng/targets/CSharp.Common.targets

  </Target>

+  <Choose>
+    <When Condition="'$(Nullable)' != '' AND ('$(TargetFramework)' == 'netstandard2.0' OR '$(TargetFrameworkIdentifier)' == '.NETFramework')">


Could you add a comment explaining why .NET Core TFMs are excluded?

dougbu · 2020-05-28T23:17:37Z

src/Localization/Abstractions/ref/Microsoft.Extensions.Localization.Abstractions.csproj

  <PropertyGroup>
    <TargetFrameworks>netstandard2.0;$(DefaultNetCoreTargetFramework)</TargetFrameworks>
    <TargetFrameworks Condition="'$(DotNetBuildFromSource)' == 'true'">$(DefaultNetCoreTargetFramework)</TargetFrameworks>
+    <Nullable>annotations</Nullable>


Ick, this gets ugly duplicating the src/ project's properties. Is it possible to add the (conditional) property in eng/targets/CSharp.ReferenceAssembly.props. I'm betting that would require a target in the src/ project that returns $(Nullable) and that's a major hack as well as an <MSBuild/> explosion. I hope there's something we could do but don't have a good suggestion.

Another bad suggestion 😈 would be to load the src/ project into a string and see if it contains <Nullable>annotations</Nullable> or <Nullable>enable</Nullable>

Using <Nullable>enable</Nullable> in the ref assembly causes build errors because of all the of throw null; for instance, from the ref assembly below:

public string Value { [System.Runtime.CompilerServices.CompilerGeneratedAttribute] get { throw null; } }

Always using annotations seems like the right way unless we change the codegen to be null-safe.

dougbu · 2020-05-28T23:20:35Z

src/Localization/Localization/src/ResourceManagerStringLocalizerFactory.cs

+                return rootNamespaceAttribute.RootNamespace;
+            }
+
+            return assembly.GetName().Name;


Just curious: Did the compiler complain about the previoius expression? If yes, what was it's concern?

Both FullName and Name are nullable and AssemblyName's ctor does not like nulls. This had fewer null checks

dougbu · 2020-05-28T23:29:03Z

...on/test/Microsoft.Extensions.Localization.Tests/ResourceManagerStringLocalizerFactoryTest.cs

+// Copyright (c) .NET Foundation. All rights reserved.
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

+#nullable disable


You're using nullable types. Why is this needed?

Some of these tests have dozens of null errors. I would like for new tests to be authored "correctly", but fixing these tests to either be correct or just litter it with null-forgiveness seems like a massive chore with low payout.

dougbu · 2020-05-28T23:31:01Z

src/Shared/Nullable/NullableAttributes.cs

+{
+    /// <summary>Specifies that null is allowed as an input even if the corresponding type disallows it.</summary>
+    [AttributeUsage(AttributeTargets.Field | AttributeTargets.Parameter | AttributeTargets.Property, Inherited = false)]
+#if INTERNAL_NULLABLE_ATTRIBUTES


Am I correct these attributes will be public when targeting .NET 5 and internal otherwise?

This file doesn't get used in .NET Core since these types are already part of the runtime and we get them for free. They should be available as internal types in .NET Core. I copied this file as-is from dotnet/runtime with the hope that it would be easier to keep it updated as they add new annotations.

ryanbrandenburg

Overall looks good.

ryanbrandenburg · 2020-05-29T17:47:55Z

src/Localization/Localization/src/Internal/AssemblyWrapper.cs

        public Assembly Assembly { get; }

-        public virtual string FullName => Assembly.FullName;
+        public virtual string FullName => Assembly.FullName!;


I've not dug deep on the null-forgiveness operator so correct me if I'm wrong. Is this quite right though? It seems like we're saying Assembly.FullName might be null, but that's fine and we'll assign it to a string that should be non-null. Or does this actually mean "Assembly.FullName CLAIMS that it will return string?, but I know better and I'm telling you that that value will always be a non-null string"?

Or does this actually mean "Assembly.FullName CLAIMS that it will return string?, but I know better and I'm telling you that that value will always be a non-null string"?

This

ryanbrandenburg · 2020-05-29T17:57:58Z

src/Localization/Localization/src/ResourceManagerStringLocalizer.cs

                : _resourceStringProvider.GetAllResourceStrings(culture, true);

-            foreach (var name in resourceNames)
+            foreach (var name in resourceNames ?? Enumerable.Empty<string>())


Hmm, I'm not a huge fan of this pattern but I can't come up with a suggestion that I prefer. Maybe the better solution in this case is to not return null here. ResourceManagerStringProvider is an internal (technically pubternal) class so we shouldn't have to worry about the breaking change. Actually looking at it as-is this might be a bug which would allow a null-ref in the current state.

IMO, if (resourceNames == null) { return; }. Avoids allocating an IEnumerable<string>

Although this isn't a hot path so it doesn't really matter.

AFAIK, these things get cached, so it's not saving much.

ryanbrandenburg · 2020-05-29T18:00:08Z

src/Localization/Localization/src/ResourceManagerStringLocalizerFactory.cs


-            return rootNamespaceAttribute?.RootNamespace ??
-                new AssemblyName(assembly.FullName).Name;
+            if (rootNamespaceAttribute != null)


I like it, much clearer.

JamesNK · 2020-05-29T21:04:00Z

src/Localization/Localization/src/ResourceManagerStringLocalizer.cs

                : _resourceStringProvider.GetAllResourceStrings(culture, true);

-            foreach (var name in resourceNames)
+            foreach (var name in resourceNames ?? Enumerable.Empty<string>())


IMO, if (resourceNames == null) { return; }. Avoids allocating an IEnumerable<string>

Although this isn't a hot path so it doesn't really matter.

Add nullability to Localization

eabf599

* Changes to support nullable for ns2.0 * Change ref assembly project to use nullable context

pranavkm marked this pull request as ready for review May 28, 2020 23:06

pranavkm requested review from JamesNK and ryanbrandenburg May 28, 2020 23:07

pranavkm added the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label May 28, 2020

pranavkm requested a review from a team May 28, 2020 23:11

pranavkm commented May 28, 2020

View reviewed changes

eng/targets/CSharp.Common.targets

</ItemGroup>

</Target>

<Choose>

Copy link

Contributor Author

pranavkm May 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI @dotnet/aspnet-build

dougbu approved these changes May 28, 2020

View reviewed changes

pranavkm mentioned this pull request May 29, 2020

Add JSON extension methods to request and response #21731

Merged

6 tasks

ryanbrandenburg approved these changes May 29, 2020

View reviewed changes

Pilchie added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label May 29, 2020

JamesNK approved these changes May 29, 2020

View reviewed changes

pranavkm merged commit c87d14a into master Jun 1, 2020

pranavkm deleted the prkrishn/localization branch June 1, 2020 18:32

pranavkm removed the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Mar 22, 2021

Add nullability to Localization #22323

Add nullability to Localization #22323

Uh oh!

Conversation

pranavkm commented May 28, 2020

Uh oh!

ghost commented May 28, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dougbu left a comment

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!

ryanbrandenburg left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants