Report passing a maybe-null int? to [DisallowNull] parameter by jcouv · Pull Request #36263 · dotnet/roslyn

jcouv · 2019-06-08T22:17:24Z

We should warn for passing a maybe-null value to a [DisallowNull] parameter that has a nullable value type.
Note: when implementing attributes on fields and properties, we'll have to do the same, since the check was not folded into VisitConversion or ReportNullableAssignmentIfNecessary.

Relates to #35816 (attributes work)

AlekseyTs · 2019-06-11T18:05:28Z

src/Compilers/CSharp/Portable/CSharpResources.resx

    <value>Argument cannot be used as an output for parameter due to differences in the nullability of reference types.</value>
  </data>
+  <data name="WRN_NullDisallowedInAssignment" xml:space="preserve">
+    <value>A possible null value may not be passed to a target marked with the [DisallowNull] attribute</value>


Would it make sense to make the message more descriptive and include information for the parameter and the method as we do in other similar warnings? #Closed

The reason I used general terms is because we'll use the same message for passing a maybe-null value to a property or field marked with [DisallowNull].

In reply to: 292590685 [](ancestors = 292590685)

AlekseyTs · 2019-06-11T18:09:53Z

src/Compilers/CSharp/Portable/CSharpResources.resx

  <data name="WRN_NullabilityMismatchInArgumentForOutput_Title" xml:space="preserve">
    <value>Argument cannot be used as an output for parameter due to differences in the nullability of reference types.</value>
  </data>
+  <data name="WRN_NullDisallowedInAssignment" xml:space="preserve">


WRN_NullDisallowedInAssignment [](start = 14, length = 30)

Consider using for specific name, reflecting that it is about DisallowNullAttribute. #Closed

AlekseyTs · 2019-06-11T18:10:40Z

src/Compilers/CSharp/Portable/Errors/ErrorCode.cs

        WRN_NullReferenceIterationVariable = 8606,
-        // Unused 8607-8608
+        WRN_NullDisallowedInAssignment = 8607,
+        // Unused 8608-8608


8608-8608 [](start = 18, length = 9)

Minor, this isn't really a range any more. #Closed

AlekseyTs · 2019-06-11T18:41:47Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs


+        private void CheckDisallowedNullAssignment(TypeWithState state, TypeWithAnnotations type, FlowAnalysisAnnotations annotations, Location location)
+        {
+            if (type.IsNullableType() && state.MayBeNull && ((annotations & FlowAnalysisAnnotations.DisallowNull) != 0))


((annotations & FlowAnalysisAnnotations.DisallowNull) != 0) [](start = 60, length = 59)

Would it make sense to check this first? #Closed

AlekseyTs · 2019-06-11T18:43:04Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs


+        private void CheckDisallowedNullAssignment(TypeWithState state, TypeWithAnnotations type, FlowAnalysisAnnotations annotations, Location location)
+        {
+            if (type.IsNullableType() && state.MayBeNull && ((annotations & FlowAnalysisAnnotations.DisallowNull) != 0))


type.IsNullableType() [](start = 16, length = 21)

Do we want to cover type parameters constrained to a nullable value type? This check will not cover them. Please add a test regardless. #Closed

I didn't understand.
I don't think we can constrain a type parameter to a nullable value type: where T : U? where U : struct is an error. (error CS0701: 'U?' is not a valid constraint. A type used as a constraint must be an interface, a non-sealed class or a type parameter.)

In reply to: 292606714 [](ancestors = 292606714)

Chuck gave me some hint how to do that.

In reply to: 292632259 [](ancestors = 292632259,292606714)

I don't see a unit-test for that scenario.

In reply to: 292634369 [](ancestors = 292634369,292632259,292606714)

AlekseyTs · 2019-06-11T18:58:07Z

src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs

+@"using System.Runtime.CompilerServices;
+class Program
+{
+    static void F5<T>([DisallowNull]ref T? t) where T : struct { }


ref [](start = 36, length = 3)

Consider covering in as well. #Closed

AlekseyTs · 2019-06-11T19:01:06Z

Done with review pass (iteration 2) #Closed

333fred · 2019-06-11T19:41:15Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

            Debug.Assert(!this.IsConditionalState);
        }

+        private void CheckDisallowedNullAssignment(TypeWithState state, TypeWithAnnotations type, FlowAnalysisAnnotations annotations, Location location)


Does it make sense to make this a local function? #ByDesign

I expect to use the method for checking on scenario when assigning to fields and properties too.

In reply to: 292627658 [](ancestors = 292627658)

AlekseyTs · 2019-06-11T22:13:30Z

src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs

+        }
+
+        [Fact, WorkItem(36009, "https://github.com/dotnet/roslyn/issues/36009")]
+        public void DisallowNull_ByValParameter_NullableValueTypeViaConstraint()


DisallowNull_ByValParameter_NullableValueTypeViaConstraint [](start = 20, length = 58)

I am not sure what are we trying to test here, struct constraint doesn't allow nullable value types and U is a nullable value type from constraints. #Closed

AlekseyTs · 2019-06-11T22:15:16Z

Done with review pass (iteration 3) #Closed

AlekseyTs

LGTM (iteration 4)

jcouv · 2019-06-12T17:00:15Z

Thanks @AlekseyTs

@333fred Do you have any other feedback?

jcouv added Area-Compilers Feature - Nullable Reference Types Nullable Reference Types labels Jun 8, 2019

jcouv added this to the 16.2.P3 milestone Jun 8, 2019

jcouv self-assigned this Jun 8, 2019

jinujoseph modified the milestones: 16.2.P3, 16.2 Jun 9, 2019

Report passing a maybe-null int? to [DisallowNull] parameter

68dd9f9

jcouv force-pushed the disallow-nullable branch from 18fcc0a to 68dd9f9 Compare June 10, 2019 19:16

jcouv marked this pull request as ready for review June 10, 2019 19:34

jcouv requested a review from a team as a code owner June 10, 2019 19:34

Update docs

c7725d6

jcouv force-pushed the disallow-nullable branch from 7147f1b to c7725d6 Compare June 10, 2019 22:53

jaredpar mentioned this pull request Jun 10, 2019

Nullable Reference Type Changes #35816

Closed

23 tasks

AlekseyTs reviewed Jun 11, 2019

View reviewed changes

333fred reviewed Jun 11, 2019

View reviewed changes

Address PR feedback

14ea0cb

AlekseyTs reviewed Jun 11, 2019

View reviewed changes

Fix scenario with Nullable'1 constrained type parameter

b49632e

jcouv force-pushed the disallow-nullable branch from 9b99ed7 to b49632e Compare June 12, 2019 00:28

AlekseyTs approved these changes Jun 12, 2019

View reviewed changes

333fred approved these changes Jun 12, 2019

View reviewed changes

jcouv merged commit 56dde62 into dotnet:master Jun 12, 2019

jcouv deleted the disallow-nullable branch June 12, 2019 19:53

Conversation

jcouv commented Jun 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs Jun 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Jun 11, 2019

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jun 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jun 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jun 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jun 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Jun 11, 2019

Choose a reason for hiding this comment

Uh oh!

jcouv Jun 11, 2019

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jun 11, 2019

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jun 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Jun 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

333fred Jun 11, 2019 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Jun 11, 2019

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jun 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Jun 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlekseyTs left a comment

Choose a reason for hiding this comment

Uh oh!

jcouv commented Jun 12, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jcouv commented Jun 8, 2019 •

edited

Loading

AlekseyTs Jun 11, 2019 •

edited

Loading

AlekseyTs Jun 11, 2019 •

edited

Loading

AlekseyTs Jun 11, 2019 •

edited

Loading

AlekseyTs Jun 11, 2019 •

edited

Loading

AlekseyTs Jun 11, 2019 •

edited

Loading

AlekseyTs Jun 11, 2019 •

edited

Loading

AlekseyTs commented Jun 11, 2019 •

edited

Loading

333fred Jun 11, 2019 •

edited by jcouv

Loading

AlekseyTs Jun 11, 2019 •

edited

Loading

AlekseyTs commented Jun 11, 2019 •

edited

Loading