Annotate ReferenceManager #42348

jaredpar · 2020-03-11T23:35:33Z

No description provided.

jaredpar · 2020-03-12T16:20:52Z

src/Compilers/CSharp/Portable/Symbols/ReferenceManager.cs

@@ -424,19 +424,20 @@ private bool CreateAndSetSourceAssemblyFullBind(CSharpCompilation compilation)

                    for (int i = 1; i < bindingResult.Length; i++)
                    {
-                        if ((object)bindingResult[i].AssemblySymbol == null)
+                        ref BoundInputAssembly bound = ref bindingResult[i];


The lack of member tracking in arrays was particularly problematic for this change. Then again this code is essentially paassing around large arrays of mutable struct values. It's the worst possible case for nullability. This doesn't make me want to push for array index tracking (that's really expensive) but it does make me think that if we were doing this again we would've chosen a different approach. Possibly wrapping this array in a simple struct wrapper that could've handled many of these assertions.

jaredpar · 2020-03-12T16:22:23Z

src/Compilers/CSharp/Portable/Symbols/ReferenceManager.cs

@@ -567,36 +568,41 @@ private bool CreateAndSetSourceAssemblyFullBind(CSharpCompilation compilation)

                foreach (int i in newSymbols)
                {
+                    ref BoundInputAssembly currentBindingResult = ref bindingResult[i];


This is another theme in this PR: stop passing around an array an index. The lack of array index state tracking makes this hard to manage. Instead just pass around a ref to the array element. This also has the benefit of reducing the amount of times we dereference the array at runtime.

src/Compilers/Core/Portable/ReferenceManager/CommonReferenceManager.Resolution.cs

jaredpar · 2020-03-12T17:40:52Z

@dotnet/roslyn-compiler PTAL

src/Compilers/Core/Portable/MetadataReference/ReferenceDirective.cs

src/Compilers/Core/Portable/ReferenceManager/CommonReferenceManager.Resolution.cs

jcouv · 2020-03-13T16:54:37Z

src/Compilers/Core/Portable/ReferenceManager/AssemblyReferenceBinding.cs

        {
-            private readonly AssemblyIdentity _referenceIdentity;
+            private readonly AssemblyIdentity? _referenceIdentity;


Both constructors use non-nullable referenceIdentity. I assume this field is marked as nullable because one might do default(AssemblyReferenceBinding). Is that the reason?

Yes. The runtime nullable annotation guide favors treating reference fields in structs as nullable since the use case default(T) must be considered. There are some exceptions considered but my reading is that it should be the rare case.

https://github.com/dotnet/runtime/blob/master/docs/coding-guidelines/api-guidelines/nullability.md

jcouv · 2020-03-13T16:55:30Z

src/Compilers/Core/Portable/ReferenceManager/BoundInputAssembly.cs


-            private string GetDebuggerDisplay()
+            private string? GetDebuggerDisplay()


Just curious, how does the debugger behave when the method given to DebuggerDisplay returns null?

For the way it's defined on this type, using the [DebuggerDisplay] attribute, it will display simply null.

jcouv · 2020-03-13T17:00:58Z

#nullable enable

Can we remove these (also below)?

Refers to: src/Compilers/Core/Portable/ReferenceManager/CommonReferenceManager.Binding.cs:1017 in dbb8542. [](commit_id = dbb8542e5c9269794800d7dc0104515009801903, deletion_comment = False)

jcouv

Done with review pass (iteration 4)

jcouv · 2020-03-19T18:46:41Z

@jaredpar This PR probably needs updating (possibly merging) to resolve build issues.

jaredpar · 2020-03-20T03:13:33Z

@jcouv thanks, should be fixed now.

jcouv

LGTM Thanks (iteration 8)

jaredpar · 2020-03-24T16:00:07Z

@dotnet/roslyn-compiler can I please get a second review? PR is two weeks old.

RikkiGibson · 2020-03-24T19:39:36Z

src/Compilers/Core/Portable/ReferenceManager/CommonReferenceManager.Resolution.cs

@@ -200,7 +201,7 @@ public ReferencedAssemblyIdentity(AssemblyIdentity identity, MetadataReference r
            TCompilation compilation,
            [Out] Dictionary<string, List<ReferencedAssemblyIdentity>> assemblyReferencesBySimpleName,
            out ImmutableArray<MetadataReference> references,
-            out IDictionary<(string, string), MetadataReference> boundReferenceDirectiveMap,
+            [NotNull] out IDictionary<(string, string), MetadataReference>? boundReferenceDirectiveMap,


what does it do to put 'NotNull' on an 'out' parameter of a reference type?

It's like [NotNullWhen] except that the "When" portion is always true. Basically this method will unconditionally assign a non-null value to the parameter.

What I was trying to ask is: why can't this parameter be written as out IDictionary<(string, string), MetadataReference> boundReferenceDirectiveMap, instead of the way it is now? i.e. deleting the NotNullAttribute as well as the top-level nullable annotation.

Removing the ? annotation though would mean that I'd have to provide a non-null value at the call site correct? That would break the existing code.

I think this is the scenario you are referring to. This doesn't produce a warning because we know the variable we pass using 'out' will only be written to.

public void M1(string? s1) { M2(out s1); // no warning s1.ToString(); } public void M2(out string s2) { s2 = "a"; }

Ah okay. Yeah then [NonNull] can be removed

jaredpar added Area-Compilers Concept-Null Annotations The issue involves annotating an API for nullable reference types labels Mar 11, 2020

jaredpar commented Mar 12, 2020

View reviewed changes

jaredpar force-pushed the nullable-ref branch from 2764d99 to dbb8542 Compare March 12, 2020 16:46

jaredpar commented Mar 12, 2020

View reviewed changes

src/Compilers/Core/Portable/ReferenceManager/CommonReferenceManager.Resolution.cs Outdated Show resolved Hide resolved

jaredpar marked this pull request as ready for review March 12, 2020 17:40

jaredpar requested a review from a team as a code owner March 12, 2020 17:40

jcouv reviewed Mar 13, 2020

View reviewed changes

src/Compilers/Core/Portable/MetadataReference/ReferenceDirective.cs Show resolved Hide resolved

jcouv reviewed Mar 13, 2020

View reviewed changes

src/Compilers/Core/Portable/ReferenceManager/CommonReferenceManager.Resolution.cs Outdated Show resolved Hide resolved

jcouv reviewed Mar 13, 2020

View reviewed changes

jcouv mentioned this pull request Mar 13, 2020

Enforce [NotNull] in method body only upon exit #42386

Closed

jcouv reviewed Mar 13, 2020

View reviewed changes

jcouv self-assigned this Mar 18, 2020

jaredpar added 7 commits March 19, 2020 20:06

Annotate ReferenceManager

ca21b1d

Annotate C# ReferenceManager

e7708bf

Cleanup

2f5d70a

Fix errors

83b8668

PR feedback

21a2442

Testing toolset change

0986dcf

Fixup build errors

e295351

jaredpar force-pushed the nullable-ref branch from 2527316 to e295351 Compare March 20, 2020 03:13

Fixup build errors

e159727

jcouv approved these changes Mar 20, 2020

View reviewed changes

RikkiGibson reviewed Mar 24, 2020

View reviewed changes

RikkiGibson approved these changes Mar 24, 2020

View reviewed changes

jaredpar merged commit 9e78614 into dotnet:master Mar 24, 2020

ghost added this to the Next milestone Mar 24, 2020

jaredpar deleted the nullable-ref branch March 24, 2020 21:55

sharwell modified the milestones: Next, temp, 16.6.P3 Apr 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Annotate ReferenceManager #42348

Annotate ReferenceManager #42348

jaredpar commented Mar 11, 2020

jaredpar Mar 12, 2020

jaredpar Mar 12, 2020

jaredpar commented Mar 12, 2020

jcouv Mar 13, 2020

jaredpar Mar 14, 2020

jcouv Mar 13, 2020

jaredpar Mar 14, 2020

jcouv commented Mar 13, 2020

jcouv left a comment

jcouv commented Mar 19, 2020

jaredpar commented Mar 20, 2020

jcouv left a comment

jaredpar commented Mar 24, 2020

RikkiGibson Mar 24, 2020

jaredpar Mar 24, 2020

RikkiGibson Mar 24, 2020 •

edited

Loading

jaredpar Mar 24, 2020

RikkiGibson Mar 24, 2020

jaredpar Mar 24, 2020


		private string GetDebuggerDisplay()
		private string? GetDebuggerDisplay()

Annotate ReferenceManager #42348

Annotate ReferenceManager #42348

Conversation

jaredpar commented Mar 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredpar commented Mar 12, 2020

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 Mar 13, 2020

jcouv left a comment

Choose a reason for hiding this comment

jcouv commented Mar 19, 2020

jaredpar commented Mar 20, 2020

jcouv left a comment

Choose a reason for hiding this comment

jaredpar commented Mar 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RikkiGibson Mar 24, 2020 • 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

RikkiGibson Mar 24, 2020 •

edited

Loading