Add nullable annotations to MSBuildTask project #26622

jcouv · 2018-05-04T02:36:55Z

I encountered a couple of minor issues so far:

Need annotation for nullable property with non-nullable setter #26621 (Need annotation for nullable property with non-nullable setter)
Adding ! fails to suppress warning #26618 (Adding ! fails to suppress warning)
Async method with nullable return type can return null #26614 (Async method with nullable return type can return null)
it would be useful to have tooling to list and review all public APIs, as well as all API this project depends on (to determine whether they need annotations, rather than oblivious)
issue with sourced linked into multiple projects

@cston @dotnet/roslyn-compiler for review. Thanks

sharwell · 2018-05-04T02:42:40Z

src/Compilers/Core/CommandLine/BuildProtocol.cs

@@ -60,12 +60,21 @@ internal class BuildRequest
            }
        }

+#if USES_ANNOTATIONS


❔ If this is in a branch anyway, why have the preprocessor like this? It makes it hard to see the true impact 😦

Edit: Probably a linked file and nullable reference types is only enabled in one of the locations. #Resolved

Correct. I'm setting LangVersion in MSBuildTask first, then other projects will follow. #Resolved

sharwell · 2018-05-04T02:46:53Z

src/Compilers/Core/MSBuildTask/Csc.cs

@@ -724,7 +726,7 @@ protected override bool CallHostObjectToExecute()

            ICscHostObject cscHostObject = HostObject as ICscHostObject;


💡 An InvalidCastException is probably clearer than a NullReferenceException. I believe the following resolves this without requiring the non-null assertion operator.

ICscHostObject cscHostObject = (ICscHostObject)HostObject;

sharwell · 2018-05-04T02:48:54Z

src/Compilers/Core/MSBuildTask/ManagedCompiler.cs

@@ -39,25 +39,25 @@ public ManagedCompiler()
        #region Properties

        // Please keep these alphabetized.
-        public string[] AdditionalLibPaths
+        public string[]? AdditionalLibPaths
        {
            set { _store[nameof(AdditionalLibPaths)] = value; }
            get { return (string[])_store[nameof(AdditionalLibPaths)]; }


📝 This appears to a bug/limitation somewhere. Casting to array types is not catching the issue the same way casting to string below was.

For some reason, I remember this is by-design. The cast to (string) doesn't change anything. On the other hand, the cast to (string?) affects the declared nullability of the expression (so var x = (string?)y; gives var a string? type).

@cston to confirm.

Why does it allow casting a nullable object to a non-nullable type without !?

A warning should be reported. With the current features/NullableReferenceTypes, the following generates warning CS8600: Converting null literal or possible null value to non-nullable type:

Dictionary<string, object?> _store = new Dictionary<string, object?>(); string[]? AdditionalLibPaths { get { return (string[])_store[nameof(AdditionalLibPaths)]; } }

sharwell · 2018-05-04T02:50:06Z

src/Compilers/Core/MSBuildTask/ManagedCompiler.cs

        {
            set
            {
                if (UsedCommandLineTool)
                {
-                    NormalizePaths(value);
+                    NormalizePaths(value!); // PROTOTYPE(NullableDogfood): value set should be non-null


💭 I believe this is going to be a common request. Technically the situation occurs many times in this file but this is the only one that catches it.

Tracked by #26621

sharwell · 2018-05-04T02:52:55Z

src/Compilers/Core/MSBuildTask/ManagedCompiler.cs

@@ -939,7 +942,7 @@ private static bool ListHasNoDuplicateItems(ITaskItem[] itemList, string paramet
            foreach (ITaskItem item in itemList)
            {
                string key;
-                string disambiguatingMetadataValue = null;
+                string? disambiguatingMetadataValue = null;


📝 Related to #23281

sharwell · 2018-05-04T02:53:38Z

src/Compilers/Core/MSBuildTask/MapSourceRoots.cs

@@ -24,6 +24,8 @@ public sealed class MapSourceRoots : Task
    {
        public MapSourceRoots()
        {
+            SourceRoots = null!; // PROTOTYPE(NullableDogfood): Required by MSBuild


Required by MSBuild

📝 Saw this a few times; wasn't quite sure what it means

The SourceRoots property has an attribute [Required] (from MSBuild). It means that MSBuild will check this for us and we can assume that it is already initialized.
The constructor however doesn't know that.

sharwell · 2018-05-04T02:56:13Z

src/Compilers/Core/MSBuildTask/RCWForCurrentContext.cs

@@ -56,7 +56,7 @@ public RCWForCurrentContext(T rcw)
            else
            {
                _shouldReleaseRCW = true;
-                _rcwForCurrentCtx = objInCurrentCtx as T;
+                _rcwForCurrentCtx = (objInCurrentCtx as T)!; // PROTOTYPE(NullableDogfood): class? constraint not yet implemented


❔ Why not (T)objInCurrentCtx?

sharwell · 2018-05-04T02:57:14Z

src/Compilers/Core/MSBuildTask/ValidateBootstrap.cs

            if (string.IsNullOrEmpty(path))
            {
                return path;
            }

-            var c = path[path.Length - 1];
+            var c = path![path!.Length - 1];


💭 Isn't the following sufficient?

path[path!.Length - 1] ``` #Resolved

Current semantics of ! is to suppress conversion and assignment warnings, but it does not affect the state of the expression it is applied to.

Update: Looking back at LDM notes, ! also changes the top-level nullability of the value returned by the expression. #Resolved

sharwell · 2018-05-04T02:58:22Z

src/Compilers/Core/MSBuildTask/ValidateBootstrap.cs

            if (c == Path.DirectorySeparatorChar || c == Path.AltDirectorySeparatorChar)
            {
-                path = path.Substring(0, path.Length - 1);
+                path = path!.Substring(0, path!.Length - 1);


❔ Is there a reason ! doesn't propagate in data flow analysis? #Resolved

I don't remember the rationale. #Resolved

sharwell · 2018-05-04T03:02:05Z

src/Compilers/Core/Portable/InternalUtilities/ReflectionUtilities.cs

        {
            if (methodInfo == null)
            {
+#if USES_ANNOTATIONS
+                // PROTOTYPE(NullableDogfood): We will fix with System.Delegate constraint


💭 Isn't this constraint already enabled in this branch due to the use of C# 8.0?

The compiler with nullable implementation is based off 15.6 at the moment. System.Delegate was implemented in 15.7.

If this is the latest build of features/NullableReferenceTypes, it should include dev15.7.x.

cston · 2018-05-04T04:35:15Z

src/Compilers/Core/Portable/InternalUtilities/ReflectionUtilities.cs

@@ -88,7 +104,16 @@ public static T FindItem<T>(IEnumerable<T> collection, params Type[] paramTypes)
                }
            }

+            // PROTOTYPE(NullableDogfood): T is constrained to a specific class
+            // PROTOTYPE(NullableDogfood): ! did not suppress the warning 
+            // https://github.com/dotnet/roslyn/issues/26618


FindItem<T> should be declared to return T?.

cston · 2018-05-04T04:39:44Z

src/Compilers/Core/CommandLine/CompilerServerLogger.cs

@@ -113,11 +117,18 @@ public static void Log(string message)
                string output = prefix + message + "\r\n";
                byte[] bytes = Encoding.UTF8.GetBytes(output);

+                // PROTOTYPE(NullableDogfood): There should be no warning on de-referencing s_loggingStream since checked above


Agreed, there should be no warning dereferencing the field. We should track static fields and properties. Please log a bug. #Resolved

Filed #26651 #Resolved

cston · 2018-05-05T05:13:41Z

src/Compilers/Core/MSBuildTask/MvidReader.cs

                if (!ReadBytes(reader, 8, out byte[] name))
+#endif


Consider using byte[] name for ReadBytes and move the annotation comment there. #Resolved

cston · 2018-05-05T05:18:55Z

src/Compilers/Core/MSBuildTask/RCWForCurrentContext.cs

-                _rcwForCurrentCtx = null;
+                // PROTOTYPE(NullableDogfood): T should be constrained to class? 
+                // 1>RCWForCurrentContext.cs(119,37,119,41): warning CS8625: Cannot convert null literal to non-nullable reference or unconstrained type parameter.
+                _rcwForCurrentCtx = null!;


_rcwForCurrentCtx should be T?.

cston · 2018-05-05T05:24:23Z

src/Compilers/Core/Portable/InternalUtilities/ReflectionUtilities.cs

                return default;
+#endif


Can we use T? and class constraint?

static T? InvokeConstructor<T>(this ConstructorInfo? constructorInfo, params object[]? args) where T : class

cston · 2018-05-05T05:33:33Z

src/Compilers/Shared/BuildServerConnection.cs

@@ -118,8 +149,13 @@ internal sealed class BuildServerConnection
            var clientDir = buildPaths.ClientDirectory;
            var timeoutNewProcess = timeoutOverride ?? TimeOutMsNewProcess;
            var timeoutExistingProcess = timeoutOverride ?? TimeOutMsExistingProcess;
+#if USES_ANNOTATIONS
+            Task<NamedPipeClientStream>? pipeTask = null;


Task<NamedPipeClientStream?>? pipeTask = null; #Resolved

cston · 2018-05-05T05:34:07Z

src/Compilers/Shared/BuildServerConnection.cs

@@ -162,7 +198,12 @@ internal sealed class BuildServerConnection

                if (wasServerRunning || tryCreateServerFunc(clientDir, pipeName))
                {
+#if USES_ANNOTATIONS
+                    // PROTOTYPE(NullableDogfood): Workaround async issue
+                    pipeTask = TryConnectToServerAsync(pipeName, timeout, cancellationToken)!;


Is ! needed here? #Pending

cston · 2018-05-05T05:34:44Z

src/Compilers/Shared/BuildServerConnection.cs

@@ -350,7 +398,12 @@ internal sealed class BuildServerConnection
            catch (Exception e) when (!(e is TaskCanceledException || e is OperationCanceledException))
            {
                LogException(e, "Exception while connecting to process");
+#if USES_ANNOTATIONS
+                // PROTOTYPE(NullableDogfood): https://github.com/dotnet/roslyn/issues/26614 There should be no need for !
+                return null!;


#pragma warning disable 8625 return null; #pragma warning restore 8625

jcouv · 2018-05-09T17:40:58Z

Getting some strange error on Ubuntu (details). @jaredpar @agocke Do you have any clue?

20:32:59 /mnt/j/workspace/dotnet_roslyn/features_NullableDogfood/ubuntu_16_debug_prtest/Binaries/Tools/dotnet/sdk/2.1.300-preview2-008324/Sdks/NuGet.Build.Tasks.Pack/build/NuGet.Build.Tasks.Pack.targets(198,5): error : Could not find a part of the path '/mnt/j/workspace/dotnet_roslyn/features_NullableDogfood/ubuntu_16_debug_prtest/Binaries/Debug/Dlls/MSBuildTask/netcoreapp2.0/publish/runtimes'. ~~[/mnt/j/workspace/dotnet_roslyn/features_NullableDogfood/ubuntu_16_debug_prtest/src/NuGet/NuGetProjectPackUtil.csproj]

20:32:46   csi -> /mnt/j/workspace/dotnet_roslyn/features_NullableDogfood/ubuntu_16_debug_prtest/Binaries/Debug/Exes/csi/net46/csi.exe
20:32:47   Toolset -> /mnt/j/workspace/dotnet_roslyn/features_NullableDogfood/ubuntu_16_debug_prtest/Binaries/Debug/Exes/Toolset/Toolset_DoNotUse.exe
20:32:47   VBCSCompilerTests -> /mnt/j/workspace/dotnet_roslyn/features_NullableDogfood/ubuntu_16_debug_prtest/Binaries/Debug/UnitTests/VBCSCompilerTests/netcoreapp2.0/Roslyn.Compilers.CompilerServer.UnitTests.dll
20:32:47 
20:32:47 Build succeeded.
20:32:47     0 Warning(s)
20:32:47     0 Error(s)
20:32:47 
20:32:47 Time Elapsed 00:02:38.85
20:32:48 /mnt/j/workspace/dotnet_roslyn/features_NullableDogfood/ubuntu_16_debug_prtest/src/NuGet /mnt/j/workspace/dotnet_roslyn/features_NullableDogfood/ubuntu_16_debug_prtest
20:32:48 Packing PreRelease
20:32:50   Successfully created package '/mnt/j/workspace/dotnet_roslyn/features_NullableDogfood/ubuntu_16_debug_prtest/Binaries/Debug/NuGet/PreRelease/Microsoft.CodeAnalysis.CSharp.2.9.0-dev.nupkg'.
20:32:52   Successfully created package '/mnt/j/workspace/dotnet_roslyn/features_NullableDogfood/ubuntu_16_debug_prtest/Binaries/Debug/NuGet/PreRelease/Microsoft.CodeAnalysis.Compilers.2.9.0-dev.nupkg'.
20:32:55   Successfully created package '/mnt/j/workspace/dotnet_roslyn/features_NullableDogfood/ubuntu_16_debug_prtest/Binaries/Debug/NuGet/PreRelease/Microsoft.CodeAnalysis.VisualBasic.2.9.0-dev.nupkg'.
20:32:57   Successfully created package '/mnt/j/workspace/dotnet_roslyn/features_NullableDogfood/ubuntu_16_debug_prtest/Binaries/Debug/NuGet/PreRelease/Microsoft.CodeAnalysis.Common.2.9.0-dev.nupkg'.
20:32:59 
/mnt/j/workspace/dotnet_roslyn/features_NullableDogfood/ubuntu_16_debug_prtest/Binaries/Tools/dotnet/sdk/2.1.300-preview2-008324/Sdks/NuGet.Build.Tasks.Pack/build/NuGet.Build.Tasks.Pack.targets(198,5): error : Could not find a part of the path '/mnt/j/workspace/dotnet_roslyn/features_NullableDogfood/ubuntu_16_debug_prtest/Binaries/Debug/Dlls/MSBuildTask/netcoreapp2.0/publish/runtimes'. ~~[/mnt/j/workspace/dotnet_roslyn/features_NullableDogfood/ubuntu_16_debug_prtest/src/NuGet/NuGetProjectPackUtil.csproj]

# The first commit's message is: Add annotations to MSBuildTask project # The 2nd commit message will be skipped: # Fix more annotations after updating the toolset package # The 3rd commit message will be skipped: # Address PR feedback from Chuck and Sam # The 4th commit message will be skipped: # Address PR feedback from Chuck

jcouv · 2018-08-15T03:03:07Z

Refreshed this PR with latest nullable toolset.

cston · 2018-08-16T20:42:24Z

src/Compilers/Core/Portable/NonNullTypesAttribute.cs

@@ -9,4 +9,10 @@ public NonNullTypesAttribute(bool b = true)
        {
        }
    }
+
+    [AttributeUsage(AttributeTargets.Parameter, AllowMultiple = false)]
+    internal class NotNullWhenTrueAttribute : Attribute


NotNullWhenTrueAttribute [](start = 19, length = 24)

Consider moving to a separate file.

cston · 2018-08-16T21:13:36Z

src/Compilers/Core/MSBuildTask/Csc.cs

@@ -556,8 +557,9 @@ private bool InitializeHostCompiler(ICscHostObject cscHostObject)
                    }
                }

+                // PROTOTYPE(NullableDogfood): Why does flow analysis think cscHostObject might be null here?!


Why does flow analysis think cscHostObject might be null here?! [](start = 47, length = 63)

Please reference bug #.

cston · 2018-08-16T21:16:32Z

src/Compilers/Core/MSBuildTask/InteractiveCompiler.cs

@@ -187,7 +187,8 @@ public ITaskItem Source

        protected sealed override bool IsManagedTool => !HasToolBeenOverridden;

-        protected sealed override string PathToManagedTool => Utilities.GenerateFullPathToTool(ToolName);
+        // PROTOTYPE(NullableDogfood): Need to follow-up, possible compiler bug


Need to follow-up, possible compiler bug [](start = 39, length = 40)

GenerateFullPathToTool() returns string?. Same comment for instances in other files.

cston · 2018-08-16T21:22:22Z

src/Compilers/Core/MSBuildTask/ManagedCompiler.cs

        {
            if (string.IsNullOrEmpty(features))
            {
                return;
            }

-            foreach (var feature in CompilerOptionParseUtilities.ParseFeatureFromMSBuild(features))
+            foreach (var feature in CompilerOptionParseUtilities.ParseFeatureFromMSBuild(features!))


! [](start = 97, length = 1)

Is this still needed?

cston · 2018-08-16T21:27:19Z

src/Compilers/Core/MSBuildTask/MvidReader.cs

            {
                return s_empty;
            }

+            // PROTOTYPE(NullableDogfood): Need annotation on Guid constructor (throws on null)


throws on null [](start = 80, length = 14)

Just [NonNullTypes] Guid(string) correct?

cston · 2018-08-16T21:30:43Z

src/Compilers/Core/MSBuildTask/Vbc.cs

@@ -1094,10 +1096,10 @@ protected override bool CallHostObjectToExecute()
        {
            Debug.Assert(this.HostObject != null, "We should not be here if the host object has not been set.");

-            IVbcHostObject vbcHostObject = this.HostObject as IVbcHostObject;
+            IVbcHostObject vbcHostObject = (IVbcHostObject)this.HostObject;


(IVbcHostObject)this.HostObject [](start = 43, length = 31)

Is there a reason to use explicit cast rather than as? If so, the asserts below no longer apply.

cston · 2018-08-16T21:31:46Z

src/Compilers/Core/Portable/InternalUtilities/ReflectionUtilities.cs

@@ -61,7 +77,11 @@ public static Type GetTypeFromEither(ref Type lazyType, string contractName, str
            return (lazyType == Missing) ? null : lazyType;
        }

+#if USES_ANNOTATIONS
+        public static T? FindItem<T>(IEnumerable<T> collection, params Type[] paramTypes)


T? [](start = 22, length = 2)

T

cston · 2018-08-16T21:32:12Z

src/Compilers/Core/Portable/InternalUtilities/ReflectionUtilities.cs

@@ -111,26 +144,37 @@ public static T CreateDelegate<T>(this MethodInfo methodInfo)
            return (T)(object)methodInfo.CreateDelegate(typeof(T));
        }

+#if USES_ANNOTATIONS
+        public static T? InvokeConstructor<T>(this ConstructorInfo? constructorInfo, params object[]? args)


T? [](start = 22, length = 2)

T

cston · 2018-08-16T21:33:19Z

src/Compilers/Shared/BuildServerConnection.cs

        {
-            ClientDirectory = clientDir;
+            ClientDirectory = clientDir ?? string.Empty;


string.Empty [](start = 43, length = 12)

Is this a change in behavior?

jcouv added Area-Compilers New Language Feature - Nullable Reference Types Nullable Reference Types labels May 4, 2018

jcouv added this to the 16.0 milestone May 4, 2018

jcouv self-assigned this May 4, 2018

jcouv requested review from a team as code owners May 4, 2018 02:36

jcouv force-pushed the nullable-dogfood branch from cfd93f2 to adecdb8 Compare May 4, 2018 02:55

sharwell reviewed May 4, 2018

View reviewed changes

cston reviewed May 4, 2018

View reviewed changes

jcouv force-pushed the nullable-dogfood branch 2 times, most recently from 5a49644 to 47d6fa3 Compare May 5, 2018 02:40

cston reviewed May 5, 2018

View reviewed changes

jcouv force-pushed the nullable-dogfood branch from f7b7570 to ea9e3cc Compare May 9, 2018 03:18

jcouv added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label May 9, 2018

jcouv mentioned this pull request May 11, 2018

Branch with boostrap removed fails on ubuntu #26800

Closed

jcouv force-pushed the nullable-dogfood branch 2 times, most recently from 637f0ac to 564515e Compare August 14, 2018 22:17

Integrating with newest preview toolset

13e999b

jcouv force-pushed the nullable-dogfood branch from 564515e to 13e999b Compare August 14, 2018 22:24

jcouv removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Aug 15, 2018

cston reviewed Aug 16, 2018

View reviewed changes

jcouv added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Sep 11, 2018

jinujoseph modified the milestones: 16.0, 16.3 Jun 9, 2019

jcouv modified the milestones: 16.3, 16.4 Sep 6, 2019

jinujoseph removed this from the 16.4 milestone Dec 11, 2019

jcouv closed this Mar 20, 2020

jcouv deleted the nullable-dogfood branch March 20, 2020 02:04

		@@ -724,7 +726,7 @@ protected override bool CallHostObjectToExecute()

		ICscHostObject cscHostObject = HostObject as ICscHostObject;

Add nullable annotations to MSBuildTask project #26622

Add nullable annotations to MSBuildTask project #26622

Conversation

jcouv commented May 4, 2018 • edited Loading

sharwell May 4, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv May 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharwell May 4, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv May 4, 2018 • edited Loading

Choose a reason for hiding this comment

sharwell May 4, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv May 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston May 4, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv May 5, 2018 • edited Loading

Choose a reason for hiding this comment

cston May 5, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston May 5, 2018 • edited Loading

Choose a reason for hiding this comment

cston May 5, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

cston May 5, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv commented May 9, 2018 • edited Loading

jcouv commented Aug 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Aug 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv commented May 4, 2018 •

edited

Loading

sharwell May 4, 2018 •

edited by jcouv

Loading

jcouv May 4, 2018 •

edited

Loading

sharwell May 4, 2018 •

edited by jcouv

Loading

jcouv May 4, 2018 •

edited

Loading

sharwell May 4, 2018 •

edited by jcouv

Loading

jcouv May 5, 2018 •

edited

Loading

cston May 4, 2018 •

edited by jcouv

Loading

jcouv May 5, 2018 •

edited

Loading

cston May 5, 2018 •

edited by jcouv

Loading

cston May 5, 2018 •

edited

Loading

cston May 5, 2018 •

edited by jcouv

Loading

cston May 5, 2018 •

edited by jcouv

Loading

jcouv commented May 9, 2018 •

edited

Loading

cston Aug 16, 2018 •

edited

Loading