Report error for T? when T is unconstrained by cston · Pull Request #28804 · dotnet/roslyn

cston · 2018-07-24T05:16:10Z

No description provided.

jcouv · 2018-07-24T15:21:50Z

src/Compilers/CSharp/Portable/CSharpResources.resx

    <value>Explicit application of 'System.Runtime.CompilerServices.NullableAttribute' is not allowed.</value>
  </data>
+  <data name="ERR_NullableUnconstrainedTypeParameter" xml:space="preserve">
+    <value>A nullable type parameter must have a value type or reference type constraint.</value>


Consider something more specific to guide users:
"... must be known to be a value or reference type. Consider adding a class, struct or type constraint." #Closed

jcouv · 2018-07-24T15:23:50Z

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

+        public static TypeSymbol VisitType<T>(
+            this TypeSymbolWithAnnotations type,
+            Func<TypeSymbolWithAnnotations, T, bool, bool> predicate1,
+            Func<TypeSymbol, T, bool, bool> predicate2,


Can we find better named than predicate1 and predicate2? #Closed

jcouv · 2018-07-24T15:24:14Z

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

+            // The first unconstrained type parameter in the type.
+            var typeParameter = this.VisitType(
+                (t, a, b) => t.IsAnnotated && t.TypeSymbol.IsUnconstrainedTypeParameter(),
+                (t, a, b) => false,


Once the parameters have more meaningful names. Please use named arguments here. #Closed

jaredpar · 2018-07-24T15:46:44Z

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

+        }
+
+        private static TypeSymbol VisitType<T>(
+            TypeSymbolWithAnnotations typeWithAnnotationsOpt,


Prototype comment to address once TWA is a struct? #Closed

cston · 2018-07-24T15:54:19Z

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

        public void CheckAllConstraints(ConversionsBase conversions, Location location, DiagnosticBag diagnostics)
        {
+            // The first unconstrained type parameter in the type.
+            var typeParameter = this.VisitType(


Note: This code path only handles a fraction of the cases where T? can appear. (See failing tests.) The diagnostic probably needs to be reported elsewhere. #Resolved

333fred · 2018-07-24T23:12:43Z

@cston test failures appear legitimate. #Closed

…s' into nullable-t

cston · 2018-07-26T22:27:51Z

@dotnet/roslyn-compiler please review. #Closed

333fred · 2018-07-26T23:41:22Z

src/Compilers/CSharp/Portable/Symbols/Source/FieldSymbolWithAttributesAndModifiers.cs

                    DeclaringCompilation.SynthesizeTupleNamesAttribute(type.TypeSymbol));
            }

+            // PROTOTYPE(NullableReferenceTypes): type.ReportAnnotatedUnconstrainedTypeParameterIfAny()


Why can't we do this today? #Resolved

We should be able to do these today, but these were cases where we may need to pass through a DiagnosticBag. It seemed more important to get the general approach for now rather than covering all the corner cases.

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

333fred

LGTM other than the question on the prototype comment additions.

cston · 2018-07-27T14:49:41Z

@jcouv, please review, thanks. #Closed

jcouv · 2018-07-27T16:47:43Z

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

+        /// traversal stops and that type is returned from this method. Otherwise if traversal
+        /// completes without the predicate returning true for any type, this method returns null.
+        /// </summary>
+        public static TypeSymbol VisitType<T>(


This signature, with two exclusive parameters, seems strange. Could we have one VisitType for TypeSymbol and one for TypeSymbolWithAnnotations?
Both would receive both predicates, and they would be mutually recursive (the overload for TSWA is a trivial wrapper for the other overload).

Alternatively, would it help to bundle the TypeSymbol and TSWA for this API? #Closed

One predicate is not sufficient in two cases: when the top-level type is a TypeSymbol rather than TypeSymbolWithAnnotations, and when walking up the containing types which are also TypeSymbols.

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

I didn't mean bundle the predicates (I understand we need to keep two). I meant the TypeSymbol typeOpt and TSWA ... parameters. #Closed

I agree, it's strange to have two mutually exclusive parameters. But it does allow writing specific helper methods that encapsulate the call to VisitType but that also take either a TypeSymbol or a TypeSymbolWithAnnotations (see TypeSymbolWithAnnotations.ContainsNullableReferenceTypes for instance).

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

jcouv · 2018-07-27T17:04:41Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter.cs

+            if (returnType.ContainsAnnotatedUnconstrainedTypeParameter() ||
+                parameters.Any(p => p.Type.ContainsAnnotatedUnconstrainedTypeParameter()))
+            {
+                TypeSymbolWithAnnotations.ReportAnnotatedUnconstrainedTypeParameter(location, _diagnostics);


ReportAnnotatedUnconstrainedTypeParameter [](start = 42, length = 41)

I'm confused. If I read this code correctly, it seems like we're not reporting on the specific location that caused the problem.
Yet, I think this is covered by NullableT_LocalFunction which seems to report a specific diagnostic (either on return type or parameter type).
I don't understand how that works.

Also, it feels strange to report this kind of diagnostic (for lambdas and local functions) during lowering. Could the check be moved into the relevant symbols? #Closed

Looking at LocalFunctionSymbol, I see that the check is performed there. That's why the NullableT_LocalFunction test works.
Then I wonder which scenarios/tests hit this code path?

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

Good catch. Removed.

In reply to: 205839676 [](ancestors = 205839676,205838678)

jcouv · 2018-07-27T17:12:50Z

src/Compilers/CSharp/Portable/Symbols/Source/ParameterHelpers.cs

-                if (parameter.Type.ContainsNullableReferenceTypes())
+                var location = parameter.GetNonNullSyntaxNode().Location;
+                var parameterType = parameter.Type;
+                parameterType.ReportAnnotatedUnconstrainedTypeParameterIfAny(location, diagnostics);


ReportAnnotatedUnconstrainedTypeParameterIfAny [](start = 30, length = 46)

I understand the appeal of bundling this check here (paired with check for existence of Nullable attribute, as in other places). Maybe the method should be renamed to reflect that.
Maybe CheckParametersForNullability? (or some better name)

Or maybe create a new method ParameterHelpersReportAnnotatedUnconstrainedTypeParameters? #Resolved

jcouv · 2018-07-27T17:18:22Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceDelegateMethodSymbol.cs


            internal override void AfterAddingTypeMembersChecks(ConversionsBase conversions, DiagnosticBag diagnostics)
            {
-                Location getReturnTypeLocation()


getReturnTypeLocation() [](start = 25, length = 23)

Not related to this PR: Any idea why we were doing those gymnastics before? Seems really strange. #Closed

I added the local functions in #22820 to allow the locations to be calculated late without duplicating code in each place that needed a location.

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

jcouv · 2018-07-27T17:19:55Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs

+                interfaces.Any(t => t.ContainsAnnotatedUnconstrainedTypeParameter()))
            {
-                this.DeclaringCompilation.EnsureNullableAttributeExists(diagnostics, Locations[0], modifyCompilation: true);
+                TypeSymbolWithAnnotations.ReportAnnotatedUnconstrainedTypeParameter(location, diagnostics);


Should we report more specific locations here too? #Closed

Added comment.

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

jcouv · 2018-07-27T17:29:43Z

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

+            Func<TypeSymbol, T, bool, bool> typePredicateOpt,
+            T arg,
+            bool canDigThroughNullable = false)
        {


{ [](start = 8, length = 1)

Consider asserting that we must have a Type or a TSWA, but not both. #Closed

jcouv

Done with review pass (iteration 10).

jcouv · 2018-07-27T20:34:48Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceUserDefinedOperatorSymbolBase.cs

-                this.DeclaringCompilation.EnsureNullableAttributeExists(diagnostics, ReturnTypeSyntax.Location, modifyCompilation: true);
+                this.DeclaringCompilation.EnsureNullableAttributeExists(diagnostics, location, modifyCompilation: true);
            }
+            ParameterHelpers.ReportAnnotatedUnconstrainedTypeParameters(Parameters, diagnostics);


ReportAnnotatedUnconstrainedTypeParameters [](start = 29, length = 42)

Is the operator case covered by test? #Resolved

Maybe add a PROTOTYPE comment to cover

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

The operator case is covered by NullableT_Members. I've added a constructor case to that test.

In reply to: 205909782 [](ancestors = 205909782,205892348)

jcouv · 2018-07-27T21:58:17Z

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

+            T arg,
+            bool canDigThroughNullable = false)
+        {
+            Debug.Assert(((object)typeWithAnnotationsOpt == null) != ((object)typeOpt == null));


== [](start = 57, length = 2)

nit: could be simplified with is null #Resolved

jcouv

LGTM Thanks (iteration 12).

Report error for T? when T is unconstrained

8685c1e

cston requested a review from a team as a code owner July 24, 2018 05:16

cston added Area-Compilers Feature - Nullable Reference Types Nullable Reference Types labels Jul 24, 2018

jcouv reviewed Jul 24, 2018

View reviewed changes

jaredpar reviewed Jul 24, 2018

View reviewed changes

cston mentioned this pull request Jul 24, 2018

[Work items] Nullable reference types #22152

Closed

cston commented Jul 24, 2018

View reviewed changes

cston added 3 commits July 24, 2018 08:55

More tests

d9e20e4

PR feedback

0ebc02a

More tests

80db37a

jcouv self-assigned this Jul 24, 2018

cston added 5 commits July 24, 2018 23:13

Misc.

76b38f6

Fix tests

9dee857

Misc.

ec336b7

Fix build

2c89f4b

Merge remote-tracking branch 'upstream/features/NullableReferenceType…

89874ee

…s' into nullable-t

Misc.

8b39edd

333fred reviewed Jul 26, 2018

View reviewed changes

333fred approved these changes Jul 26, 2018

View reviewed changes

jcouv reviewed Jul 27, 2018

View reviewed changes

cston added 2 commits July 27, 2018 11:06

PR feedback

9307486

PR feedback

bb6d6d5

jcouv reviewed Jul 27, 2018

View reviewed changes

jcouv approved these changes Jul 27, 2018

View reviewed changes

PR feedback

72fe668

cston merged commit 4945499 into dotnet:features/NullableReferenceTypes Jul 27, 2018

cston deleted the nullable-t branch July 27, 2018 22:09

Conversation

cston commented Jul 24, 2018

Uh oh!

jcouv Jul 24, 2018 • 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 Jul 24, 2018 • 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 Jul 24, 2018 • 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!

jaredpar Jul 24, 2018 • 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!

cston Jul 24, 2018 • 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!

333fred commented Jul 24, 2018 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cston commented Jul 26, 2018 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

333fred Jul 26, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

333fred left a comment

Choose a reason for hiding this comment

Uh oh!

cston commented Jul 27, 2018 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv Jul 27, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

jcouv Jul 27, 2018 • 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!

cston Jul 27, 2018 • 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 Jul 27, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Jul 27, 2018 • edited by cston 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 Jul 27, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

jcouv Jul 27, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

jcouv Jul 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

jcouv Jul 24, 2018 •

edited

Loading

jcouv Jul 24, 2018 •

edited

Loading

jcouv Jul 24, 2018 •

edited

Loading

jaredpar Jul 24, 2018 •

edited by jcouv

Loading

cston Jul 24, 2018 •

edited by jcouv

Loading

333fred commented Jul 24, 2018 •

edited by jcouv

Loading

cston commented Jul 26, 2018 •

edited by jcouv

Loading

333fred Jul 26, 2018 •

edited by jcouv

Loading

cston commented Jul 27, 2018 •

edited by jcouv

Loading

jcouv Jul 27, 2018 •

edited

Loading

jcouv Jul 27, 2018 •

edited

Loading

cston Jul 27, 2018 •

edited

Loading

jcouv Jul 27, 2018 •

edited

Loading

jcouv Jul 27, 2018 •

edited by cston

Loading

jcouv Jul 27, 2018 •

edited

Loading

jcouv Jul 27, 2018 •

edited

Loading

jcouv Jul 27, 2018 •

edited

Loading

jcouv Jul 27, 2018 •

edited by cston

Loading

jcouv Jul 27, 2018 •

edited by cston

Loading