[Group 1] Enable nullable annotations for `Microsoft.Extensions.DependencyModel` by maxkoshevoi · Pull Request #57445 · dotnet/runtime

maxkoshevoi · 2021-08-15T17:08:35Z

Ready for review, but not for merge

Questions (look for ! in the PR):

GetNormalizedCodeBasePath.GetNormalizedCodeBasePath is not used on net5+ due to Assembly.CodeBase being obsolete. Is there some alternative to preserve previous behavior, or is it ok to leave it like this?
Suppressed warning for now.
It is always assumed that reader.GetString() returns non-null value, but it can return null.
Should be resolved in: Remove ! from Microsoft.Extensions.DependencyModel #58139
It is always assumed that DependencyContextJsonReader.Pool(str) returns non-null value, but it only does that for non-null when argument. Suppressed errors with ! for now.
Should be resolved in: Remove ! from Microsoft.Extensions.DependencyModel #58139
In AppBaseCompilationAssemblyResolver.TryResolveAssemblyPaths Path.GetDirectoryName(sharedPath) might return null, but this is not handled.
Added Debug.Assert.

ghost · 2021-08-15T17:08:40Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

ghost · 2021-08-15T17:08:42Z

Tagging subscribers to this area: @eerhardt
See info in area-owners.md if you want to be subscribed.

Issue Details

Related to #43605, #54012

Ready for review, but not for merge

Questions (look for ! in the PR):

It is always assumed that reader.GetString() returns non-null value, but it can return null. Suppressed all errors with ! for now.
It is always assumed that DependencyContextJsonReader.Pool(str) returns non-null value, but it only does that for non-null when argument. Suppressed all errors with ! for now.
In AppBaseCompilationAssemblyResolver.TryResolveAssemblyPaths Path.GetDirectoryName(sharedPath) might return null, but this is not handled. Suppressed error with ! for now.

Author:	maxkoshevoi
Assignees:	-
Labels:	`area-DependencyModel`, `new-api-needs-documentation`, `community-contribution`
Milestone:	-

src/libraries/Microsoft.Extensions.DependencyModel/src/DependencyContextJsonReader.cs

eerhardt · 2021-08-16T22:21:32Z

src/libraries/Microsoft.Extensions.DependencyModel/src/DependencyContextLoader.cs

            string depsJsonFile = Path.ChangeExtension(assemblyLocation, DepsJsonExtension);
            bool depsJsonFileExists = _fileSystem.File.Exists(depsJsonFile);
-
+#if !NET5_0_OR_GREATER


Why are we making this change?

Assembly.CodeBase is deprecated in .net5+: #31127

I don't think removing this code is a good idea for now. You should just suppress the Obsolete warning instead. Ex:

#pragma warning disable SYSLIB0012 // CodeBase is obsolete

Just suppressing the warning breaks CI:

Using member 'System.Reflection.Assembly.CodeBase' which has 'RequiresAssemblyFilesAttribute' can break functionality when embedded in a single-file app. This member throws an exception for assemblies embedded in a single-file app.

src/libraries/Microsoft.Extensions.DependencyModel/ref/Microsoft.Extensions.DependencyModel.cs

eerhardt · 2021-08-17T23:16:28Z

src/libraries/Microsoft.Extensions.DependencyModel/src/DependencyContextJsonReader.cs

            while (reader.Read() && reader.IsTokenTypeProperty())
            {
-                targets.Add(ReadTarget(ref reader, reader.GetString()));
+                targets.Add(ReadTarget(ref reader, reader.GetString()!));


I think this ! is lying that this can't be null. We should make the ReadTarget method accept string? targetName.

Similar comment for ReadTargetLibrary

Experimented with it and looks like targetName shouldn't be null, or code will NRE elsewhere:

https://github.com/dotnet/runtime/blob/main/src/libraries/Microsoft.Extensions.DependencyModel/src/DependencyContextJsonReader.cs#L195 (IsRuntimeTarget would crash if Name is null)

https://github.com/dotnet/runtime/blob/main/src/libraries/Microsoft.Extensions.DependencyModel/src/DependencyContextJsonReader.cs#L202 (new TargetInfo(framework...) a bit later would crash if framework is null)

https://github.com/dotnet/runtime/blob/main/src/libraries/Microsoft.Extensions.DependencyModel/src/DependencyContextJsonReader.cs#L238 (IsRuntimeTarget would crash if Name is null)

I've added Debug.Assert here just in case

Returned ! and created an issue #58139

src/libraries/Microsoft.Extensions.DependencyModel/src/DependencyContextWriter.cs

src/libraries/Microsoft.Extensions.DependencyModel/src/Utf8JsonReaderExtensions.cs

# Conflicts: # src/libraries/Microsoft.Extensions.DependencyModel/src/Dependency.cs

...braries/Microsoft.Extensions.DependencyModel/src/Microsoft.Extensions.DependencyModel.csproj

...braries/Microsoft.Extensions.DependencyModel/ref/Microsoft.Extensions.DependencyModel.csproj

src/libraries/Microsoft.Extensions.DependencyModel/ref/Microsoft.Extensions.DependencyModel.cs

src/libraries/Microsoft.Extensions.DependencyModel/src/RuntimeAssetGroup.cs

src/libraries/Microsoft.Extensions.DependencyModel/ref/Microsoft.Extensions.DependencyModel.cs

src/libraries/Microsoft.Extensions.DependencyModel/src/DependencyContextWriter.cs

eerhardt · 2021-08-25T13:43:07Z

src/libraries/Microsoft.Extensions.DependencyModel/src/DependencyContextLoader.cs

            string depsJsonFile = Path.ChangeExtension(assemblyLocation, DepsJsonExtension);
            bool depsJsonFileExists = _fileSystem.File.Exists(depsJsonFile);
-
+#if !NET5_0_OR_GREATER


I don't think removing this code is a good idea for now. You should just suppress the Obsolete warning instead. Ex:

#pragma warning disable SYSLIB0012 // CodeBase is obsolete

src/libraries/Microsoft.Extensions.DependencyModel/src/DependencyContextJsonReader.cs

TODO: Create an issue to remove !

* Don't throw for null assemblies in TryResolveAssemblyPaths * Fix up ref source * Fix RuntimeLibrary constructor overloads

eerhardt

Thank you again, @maxkoshevoi, for this work. This really helps!

I pushed a couple commits to this branch to get the build and unit tests passing. I also fixed a few nits and inconsistencies while I was working here.

I'll merge this once CI is green.

dotnet/runtime#57445

maxkoshevoi added 3 commits August 15, 2021 19:47

Annotate src

5b217b6

Annotate ref

fddbdaf

Update ref csproj

d5f37dd

ghost added community-contribution Indicates that the PR has been added by a community member area-DependencyModel new-api-needs-documentation labels Aug 15, 2021

Dont use Assembly.CodeBase

7fc679c

eerhardt reviewed Aug 16, 2021

View reviewed changes

src/libraries/Microsoft.Extensions.DependencyModel/src/DependencyContextJsonReader.cs Show resolved Hide resolved

eerhardt reviewed Aug 16, 2021

View reviewed changes

maxkoshevoi added 2 commits August 17, 2021 10:56

Add some Debug.Asserts

da8ec27

One more assert

0ccc382

maryamariyan added this to the 7.0.0 milestone Aug 17, 2021

eerhardt reviewed Aug 17, 2021

View reviewed changes

src/libraries/Microsoft.Extensions.DependencyModel/ref/Microsoft.Extensions.DependencyModel.cs Outdated Show resolved Hide resolved

eerhardt reviewed Aug 17, 2021

View reviewed changes

src/libraries/Microsoft.Extensions.DependencyModel/src/DependencyContextWriter.cs Outdated Show resolved Hide resolved

eerhardt reviewed Aug 17, 2021

View reviewed changes

src/libraries/Microsoft.Extensions.DependencyModel/src/Utf8JsonReaderExtensions.cs Outdated Show resolved Hide resolved

eerhardt reviewed Aug 17, 2021

View reviewed changes

src/libraries/Microsoft.Extensions.DependencyModel/src/Utf8JsonReaderExtensions.cs Outdated Show resolved Hide resolved

maxkoshevoi added 5 commits August 18, 2021 09:48

runtimeSignature can be null

ac62b42

reader.GetString() can be null

6ad9a51

Throw NRE when library is null

9ed265b

Update ref

dde76da

Merge branch 'main' into mk/43605-DependencyModel

73b839a

# Conflicts: # src/libraries/Microsoft.Extensions.DependencyModel/src/Dependency.cs

eerhardt reviewed Aug 25, 2021

View reviewed changes

maxkoshevoi added 5 commits August 25, 2021 22:58

CodeBase is obsolete

b076ad3

Update ref for HashCodeCombiner

cc9dfca

Annotate RuntimeAssetGroup

dd75fb0

Null check resolvers

ffe7d77

Update ReferenceAssemblyPathResolver's constructor

6d205a8

maxkoshevoi added 2 commits August 25, 2021 23:21

Don't throw NRE explicitly

e7ac46c

Remove Debug.Asserts

57dee24

TODO: Create an issue to remove !

maxkoshevoi mentioned this pull request Aug 25, 2021

Remove ! from Microsoft.Extensions.DependencyModel #58139

Closed

maxkoshevoi and others added 5 commits August 26, 2021 10:27

DisableImplicitAssemblyReferences

ee49e1f

Use ProjectReference in ref

75f22be

Merge branch 'main' into mk/43605-DependencyModel

618d285

PR feedback and fix build

ae167eb

Fix unit tests -

9a9a90f

* Don't throw for null assemblies in TryResolveAssemblyPaths * Fix up ref source * Fix RuntimeLibrary constructor overloads

eerhardt approved these changes Sep 1, 2021

View reviewed changes

eerhardt merged commit f41b259 into dotnet:main Sep 2, 2021

maxkoshevoi deleted the mk/43605-DependencyModel branch September 2, 2021 14:43

roji mentioned this pull request Sep 7, 2021

[main] Update dependencies from dotnet/runtime dotnet/efcore#25883

Merged

AnudeepGunukula added a commit to AnudeepGunukula/runtime that referenced this pull request Sep 8, 2021

remove ! operations added in dotnet#57445 (mainly in 57dee24)

a8d16a8

AnudeepGunukula mentioned this pull request Sep 8, 2021

Removed ! operations from Microsoft.Extensions.DependencyModel (DependencyContextJsonReader.cs) #58789

Closed

ericstj mentioned this pull request Sep 13, 2021

[main] Update dependencies from dotnet/runtime dotnet/sdk#20859

Merged

ericstj added a commit to dotnet/sdk that referenced this pull request Sep 13, 2021

React to nullable changes in Microsoft.Extensions.DependencyModel

e4f19da

dotnet/runtime#57445

maxkoshevoi mentioned this pull request Sep 16, 2021

[Group 1] Enable nullable annotations for Microsoft.Extensions.FileSystemGlobbing #57398

Merged

javiercn pushed a commit to dotnet-maestro-bot/sdk that referenced this pull request Sep 30, 2021

React to nullable changes in Microsoft.Extensions.DependencyModel

8657ce2

dotnet/runtime#57445

ghost locked as resolved and limited conversation to collaborators Oct 2, 2021

Conversation

maxkoshevoi commented Aug 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Aug 15, 2021

Uh oh!

ghost commented Aug 15, 2021

Uh oh!

Uh oh!

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!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

eerhardt left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

maxkoshevoi commented Aug 15, 2021 •

edited

Loading