Nullable annotate for Microsoft.Win32.Registry #2351

manne · 2020-01-29T20:13:30Z

Contributes to #2339

src/libraries/Microsoft.Win32.Registry/src/Microsoft/Win32/RegistryKey.Windows.cs

eiriktsarpalis · 2020-01-29T23:10:15Z

Please hold off from merging, Microsoft.Win32.Registry has a dependency on System.Security.Principal.Windows, whose nullable annotation is still WIP.

src/libraries/Microsoft.Win32.Registry/src/Microsoft/Win32/Registry.cs

src/libraries/Microsoft.Win32.Registry/src/Microsoft/Win32/RegistryKey.Windows.cs

src/libraries/Microsoft.Win32.Registry/ref/Microsoft.Win32.Registry.cs

src/libraries/Microsoft.Win32.Registry/src/Microsoft/Win32/RegistryKey.cs

src/libraries/Microsoft.Win32.Registry/src/System/Security/AccessControl/RegistrySecurity.cs

eiriktsarpalis · 2020-02-03T15:18:37Z

System.Security.Principal.Windows annotations have been merged to master, could you please rebase your branch on top of the latest master?

manne · 2020-02-05T20:07:53Z

@eiriktsarpalis Have I done the correct thing?

buyaa-n · 2020-02-06T00:01:26Z

@manne please update files for Linux / Mac versions too, if you are using VS change your project context to Linux/OSX respectively to see the files in VS

eiriktsarpalis · 2020-02-06T13:14:09Z

There seem to be nullability related build errors on windows too. They might be related to the newly annotated dependencies.

eiriktsarpalis · 2020-02-10T21:33:59Z

Hi @manne, and thank you for your contribution.

What is the status of this PR? As part of our ongoing annotation work we are looking to get this project done asap. Will you able to complete it soon? If not, would you mind if we took this over, either by creating a separate PR or pushing commits to your branch?

manne · 2020-02-11T18:37:46Z

@eiriktsarpalis I am working on it right now

eiriktsarpalis · 2020-02-12T12:30:15Z

src/libraries/Microsoft.Win32.Registry/src/Microsoft/Win32/RegistryKey.Windows.cs

@@ -83,22 +82,20 @@ private void FlushCore()
            }
        }

-        private unsafe RegistryKey CreateSubKeyInternalCore(string subkey, RegistryKeyPermissionCheck permissionCheck, RegistryOptions registryOptions)
+        private unsafe RegistryKey? CreateSubKeyInternalCore(string subkey, RegistryKeyPermissionCheck permissionCheck, RegistryOptions registryOptions)


Looks like the return type is annotated as nullable on account of the code path on line 121. But it seems clear by the assertion above that this is not expected to happen unless there is a bug. I think non-nullability should be asserted.

Good point, if this not return null then all public RegistryKey CreateSubKey(...) overloads not return null

buyaa-n · 2020-02-13T18:45:54Z

src/libraries/Microsoft.Win32.Registry/src/Microsoft/Win32/RegistryKey.Windows.cs

        {
            int type = 0;
            int datasize = 0;
-            int ret = Interop.Advapi32.RegQueryValueEx(_hkey, name, null, ref type, (byte[])null, ref datasize);
+            int ret = Interop.Advapi32.RegQueryValueEx(_hkey, name, null, ref type, (byte[])null!, ref datasize);


Suggested change

int ret = Interop.Advapi32.RegQueryValueEx(_hkey, name, null, ref type, (byte[])null!, ref datasize);

int ret = Interop.Advapi32.RegQueryValueEx(_hkey, name, null, ref type, (byte[]?)null, ref datasize);

buyaa-n · 2020-02-13T19:01:34Z

src/libraries/Microsoft.Win32.Registry/ref/Microsoft.Win32.Registry.cs

+        public Microsoft.Win32.RegistryKey? CreateSubKey(string subkey, Microsoft.Win32.RegistryKeyPermissionCheck permissionCheck, Microsoft.Win32.RegistryOptions registryOptions, System.Security.AccessControl.RegistrySecurity? registrySecurity) { throw null; }
+        public Microsoft.Win32.RegistryKey? CreateSubKey(string subkey, Microsoft.Win32.RegistryKeyPermissionCheck permissionCheck, System.Security.AccessControl.RegistrySecurity? registrySecurity) { throw null; }
+        public Microsoft.Win32.RegistryKey? CreateSubKey(string subkey, bool writable) { throw null; }
+        public Microsoft.Win32.RegistryKey? CreateSubKey(string subkey, bool writable, Microsoft.Win32.RegistryOptions options) { throw null; }


As per @eiriktsarpalis's comment https://github.com/dotnet/runtime/pull/2351/files#r378221011 all this CreateSubKey(...) overloads not return null

buyaa-n

Except @eiriktsarpalis's comment and updates related to that change overall looks great, thank you @manne

eiriktsarpalis · 2020-02-17T14:06:08Z

LGTM. If you could just fix the merge conflicts we should be ready to merge your changes.

Thank you!

manne · 2020-02-18T20:09:37Z

Thx

Dotnet-GitSync-Bot added the area-Microsoft.Win32 label Jan 29, 2020

danmoseley reviewed Jan 29, 2020

View reviewed changes

src/libraries/Microsoft.Win32.Registry/src/Microsoft/Win32/RegistryKey.Windows.cs Outdated Show resolved Hide resolved

danmoseley reviewed Jan 29, 2020

View reviewed changes

src/libraries/Microsoft.Win32.Registry/src/Microsoft/Win32/RegistryKey.Windows.cs Outdated Show resolved Hide resolved

eiriktsarpalis added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Jan 29, 2020