Nullable annotate System.Diagnostics.Process #31671

eiriktsarpalis · 2020-02-03T18:50:09Z

Contributes to #2339

src/libraries/System.Diagnostics.Process/ref/System.Diagnostics.Process.cs

src/libraries/Common/src/Interop/Linux/cgroups/Interop.cgroups.cs

src/libraries/Common/src/Interop/Windows/Kernel32/Interop.ProcessWaitHandle.cs

src/libraries/Common/src/Interop/Linux/cgroups/Interop.cgroups.cs

src/libraries/Common/src/Interop/Linux/procfs/Interop.ProcFsStat.cs

...braries/System.Diagnostics.Process/src/Microsoft/Win32/SafeHandles/SafeProcessHandle.Unix.cs

src/libraries/System.Diagnostics.Process/src/System/Diagnostics/PerformanceCounterLib.cs

src/libraries/System.Diagnostics.Process/ref/System.Diagnostics.Process.cs

src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessStartInfo.cs

src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessThreadCollection.cs

buyaa-n

Few questions, overall LGTM

src/libraries/Common/src/Interop/Linux/procfs/Interop.ProcFsStat.cs

src/libraries/Common/src/Interop/Linux/cgroups/Interop.cgroups.cs

stephentoub · 2020-02-05T20:36:06Z

src/libraries/System.Diagnostics.Process/ref/System.Diagnostics.Process.cs

-        public static System.Diagnostics.Process Start(string fileName, string arguments) { throw null; }
+        public static System.Diagnostics.Process? Start(System.Diagnostics.ProcessStartInfo startInfo) { throw null; }
+        public static System.Diagnostics.Process? Start(string fileName) { throw null; }
+        public static System.Diagnostics.Process? Start(string fileName, string arguments) { throw null; }


This is one of those cases where the annotation is correct but it's probably going to cause more harm than good I think. Still, we should keep what you have.

It looks like the only code path that could result in a null Process value is this one. It doesn't seem possible on the unix implementation.

The particular result stems from this shell32 function. I don't understand enough about that particular API to judge the probability of this happening, but we do seem to throw beforehand if the call is not successful.

That being said, the nullity of the particular call seems to merely reflect the result of the also public bool Process.Start() instance method, so it might make sense to keep for the sake of symmetry?

It is possible for Start to return false, if shell execute is used and returns successfully a zero handle. However, the default for ProcessStartInfo.UseShellExecute in .NET Core is false, which means while to be accurate we do need Start(ProcessStartInfo) to return Process?, the other Start(string) and Start(string, string) overloads should never return null, because they don't use shell execute, so unless I'm missing something, those should return just Process. Reasonable?

I think your analysis makes sense. One counterpoint here is that most real uses of Process.Start() I've seen typically either use ProcessStartInfo or call the Start() instance method directly. So the question here is whether we want to assert non-nullability for a special case of the api based on an invariant which may change in the future or not hold at all in future platforms.

So the question here is whether we want to assert non-nullability for a special case of the api based on an invariant which may change in the future or not hold at all in future platforms.

It'd be a breaking change to start returning null in cases where we don't today, regardless of annotation, and even if documented to say null is possible. So I'm not worried about this. If we wanted to make such a breaking change, we'd break the annotation as part of it.

That's fair enough

src/libraries/System.Diagnostics.Process/src/System/Diagnostics/PerformanceCounterLib.cs

stephentoub

Other than the outstanding comments, LGTM.

eiriktsarpalis added the area-System.Diagnostics.Process label Feb 3, 2020

eiriktsarpalis requested review from GrabYourPitchforks and buyaa-n February 3, 2020 18:50

eiriktsarpalis commented Feb 3, 2020

View reviewed changes

src/libraries/System.Diagnostics.Process/ref/System.Diagnostics.Process.cs Show resolved Hide resolved

eiriktsarpalis commented Feb 3, 2020

View reviewed changes

src/libraries/System.Diagnostics.Process/ref/System.Diagnostics.Process.cs Show resolved Hide resolved

eiriktsarpalis requested a review from stephentoub February 4, 2020 18:15