Treat unconstrained type parameters declared in `#nullable disable` context as having an unknown nullability in case they are substituted with a reference type. #34797

AlekseyTs · 2019-04-05T19:17:18Z

As apposed to possibly nullable or not nullable reference type.
Fixes #29980.
Fixes #34844.

…`` context as having an unknown nullability in case they are substituted with a reference type. As apposed to possibly nullable or not nullable reference type. Fixes dotnet#29980.

gafter · 2019-04-05T20:14:46Z

I do not understand the value of this change. There seems to be no effect on executable code (because those are uses of type parameters, and have their own annotations). And in the declarations, it only seems to introduce warnings that are not valuable to the end user. So why would we make this change? #Closed

gafter · 2019-04-05T20:18:50Z

Please bring this to the LDM for confirmation of the proposed language change before integrating this.

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

gafter · 2019-04-05T20:46:31Z

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

@@ -14037,6 +14037,9 @@ partial class C1
            var compilation = CreateCompilation(new[] { source }, options: WithNonNullTypesTrue());

            compilation.VerifyDiagnostics(
+                // (10,18): error CS0761: Partial method declarations of 'C1.M1<T>(T?, T[]?, Action<T?>, Action<T?[]?>?[]?)' have inconsistent type parameter constraints
+                //     partial void M1<T>(T? x, T[]? y, System.Action<T?> z, System.Action<T?[]?>?[]? u) where T : class
+                Diagnostic(ErrorCode.ERR_PartialMethodInconsistentConstraints, "M1").WithArguments("C1.M1<T>(T?, T[]?, System.Action<T?>, System.Action<T?[]?>?[]?)").WithLocation(10, 18),


Diagnostic [](start = 16, length = 10)

This looks wrong to me #Closed

If they differ by nullable annotations only, this should be a nullability warning, not an error.

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

If they differ by nullable annotations only, this should be a nullability warning, not an error.

At the moment, nullable mismatch causes an error. We do have an issue #30229 to finalize the design and behavior around nullability mismatch in partials. For the purpose of this PR, the error is good enough to verify an expectation that there is a mismatch, even though the error might be converted to a warning, or go away completely in the future.

In reply to: 272742716 [](ancestors = 272742716,272738601)

OK, please add a comment.

In reply to: 272766054 [](ancestors = 272766054,272742716,272738601)

gafter · 2019-04-05T20:49:01Z

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

-                // (23,17): warning CS8633: Nullability in constraints for type parameter 'TF2A' of method 'A.F2<TF2A>()' doesn't match the constraints for type parameter 'TF2' of interface method 'I1.F2<TF2>()'. Consider using an explicit interface implementation instead.
-                //     public void F2<TF2A>()
-                Diagnostic(ErrorCode.WRN_NullabilityMismatchInConstraintsOnImplicitImplementation, "F2").WithArguments("TF2A", "A.F2<TF2A>()", "TF2", "I1.F2<TF2>()").WithLocation(23, 17)
-                );


The LDM specified that a disabled region suppresses nullability warnings. The bug here was that we were producing a nullability warning in a disabled region. It doesn't appear you fixed it by adjusting where we suppress or produce nullability diagnostics. #Resolved

The LDM specified that a disabled region suppresses nullability warnings. The bug here was that we were producing a nullability warning in a disabled region. It doesn't appear you fixed it by adjusting where we suppress or produce nullability diagnostics.

The warnings can be enabled even when declarations are disabled. I'll adjust the test to reflect that. So, this isn't an issue about suppression isn't working. We do have an issue #32742 tracking the work about finalizing what warning should be suppressed when nullable warnings are disabled.

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

Please add a comment.

In reply to: 272763113 [](ancestors = 272763113,272739261)

Please add a comment.

I do not think any comment is warranted here. As I mentioned in the previous reply to this thread, I am planning to adjust the test to enable nullability warnings explicitly

In reply to: 272766744 [](ancestors = 272766744,272763113,272739261)

OK. Reopening thread to track that a change (enable nullability warnings explicitly) is pending for this test.

In reply to: 272768576 [](ancestors = 272768576,272766744,272763113,272739261)

Reopening thread to track that a change (enable nullability warnings explicitly) is pending for this test.

This not pending, it is done

In reply to: 273195151 [](ancestors = 273195151,272768576,272766744,272763113,272739261)

gafter · 2019-04-05T20:53:01Z

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

+
+            comp2.VerifyDiagnostics(
+                // (3,19): error CS0265: Partial declarations of 'I1<TF1>' have inconsistent constraints for type parameter 'TF1'
+                // partial interface I1<TF1> where TF1 : new()


This is correct, but they differ only in nullability, and so this should be a nullability warning that is suppressed in this disabled context. Please add a comment if this is a known issue. #Closed

This is correct, but they differ only in nullability, and so this should be a nullability warning that is suppressed in this disabled context. Please add a comment if this is a known issue.

This is existing behavior and is not changed in this PR. The object constraint is the source of the error. Partial declarations cannot add any constraints.

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

gafter · 2019-04-05T20:59:00Z

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

+
+            comp1.VerifyDiagnostics(
+                // (8,14): error CS0761: Partial method declarations of 'A.M1<TF1>()' have inconsistent type parameter constraints
+                // partial void M1<TF1>() where TF1 : object


Because they differ by nullability only, this should be a nullable warning and suppressed in this disabled region. Please add a comment if this is a known issue. #Closed

gafter · 2019-04-05T23:00:24Z

I've created dotnet/csharplang#2398 to describe the spec change for LDM review. #Pending

gafter · 2019-04-05T23:09:11Z

Will resume reviewing implementation Monday. #Closed

gafter · 2019-04-08T18:12:07Z

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

+        /// where nullable annotations are disabled.
+        /// Cannot be combined with <see cref="ReferenceType"/>, <see cref="ValueType"/> and <see cref="Unmanaged"/>.
+        /// </summary>
+        UnknownNullabilityIfReferenceType = 0x40,


UnknownNullabilityIfReferenceType [](start = 8, length = 33)

"Unknown" is now called "Oblivious" in NullableAnnotation. Please rename this to match. #Resolved

gafter · 2019-04-08T18:12:18Z

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

+        /// <summary>
+        /// Type parameter has no type constraints, including `struct`, `class`, `unmanaged` and is declared in a context 
+        /// where nullable annotations are disabled.
+        /// Cannot be combined with <see cref="ReferenceType"/>, <see cref="ValueType"/> and <see cref="Unmanaged"/>.


and [](start = 89, length = 3)

or #Resolved

gafter · 2019-04-08T18:20:50Z

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

+            {
+                ImmutableArray<TypeParameterConstraintClause> defaultClauses = binder.GetDefaultTypeParameterConstraintClauses(typeParameterList);
+
+                return defaultClauses.IsEmpty() ? ImmutableArray<TypeParameterConstraintClause>.Empty : defaultClauses;


defaultClauses.IsEmpty() ? ImmutableArray.Empty : defaultClauses [](start = 23, length = 95)

I don't understand why this isn't just defaultClauses. Are you trying to implement an optimization that you assume ArrayBuilder doesn't perform? #Resolved

I don't understand why this isn't just defaultClauses. Are you trying to implement an optimization that you assume ArrayBuilder doesn't perform?

defaultClauses here contains at least one item. If all items do not contain any significant data, we replace it with an empty array so that later phases don't have to worry about doing more analysis. This mimics what we would get prior to this PR.

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

How can "defaultClauses here contains at least one item" when defaultClauses.IsEmpty()? #Resolved

How can "defaultClauses here contains at least one item" when defaultClauses.IsEmpty()?

This is an extension method in TypeParameterConstraintClauseExtensions type, not a property on an ImmutableArray that checks it's length.

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

gafter · 2019-04-08T18:48:31Z

src/Compilers/CSharp/Portable/Binder/Binder_Constraints.cs

@@ -207,9 +214,26 @@ internal partial class Binder
                }
            }

+            if (!hasTypeLikeConstraint && !IsNullableEnabled(typeParameterSyntax.Identifier))
+            {


It would be useful to have some comments somewhere about the strategy here: we add constraints to represent the default oblivious constraint where T: object~, which causes us to suppress the IL emission of the default nullable constraint to imitate the behavior of pre-nullable compilers. When reading an assembly, the absence of a constraint causes us to create the default oblivious constraint. where T: object~, whereas reading the default nullable constraint where T: object? causes us to drop it on the floor, as that is the new default. Adjusted to correct any misunderstanding I may have. #Resolved

gafter

Done reviewing through Iteration 3.

gafter · 2019-04-08T20:12:26Z

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

@@ -103,5 +119,10 @@ internal static bool IsEarly(this ImmutableArray<TypeParameterConstraintClause>
        {
            return constraintClauses.Any(clause => clause.IsEarly);
        }
+
+        internal static bool IsEmpty(this ImmutableArray<TypeParameterConstraintClause> constraintClauses)


IsEmpty [](start = 29, length = 7)

Can you please rename something like "ContainsOnlyEmptyConstraintClauses"? #Resolved

gafter

AlekseyTs · 2019-04-08T21:01:27Z

@dotnet/roslyn-compiler Please review, need a second sign-off.

AlekseyTs · 2019-04-09T17:29:34Z

@dotnet/roslyn-compiler Please review, need a second sign-off.

cston · 2019-04-09T17:50:38Z

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

+            {
+                ImmutableArray<TypeParameterConstraintClause> defaultClauses = binder.GetDefaultTypeParameterConstraintClauses(typeParameterList);
+
+                return defaultClauses.ContainsOnlyEmptyConstraintClauses() ? ImmutableArray<TypeParameterConstraintClause>.Empty : defaultClauses;


defaultClauses.ContainsOnlyEmptyConstraintClauses() [](start = 23, length = 51)

Should Binder.GetDefaultTypeParameterConstraintClauses() simply return ImmutableArray<TypeParameterConstraintClause>.Empty if all are empty? #Resolved

Should Binder.GetDefaultTypeParameterConstraintClauses() simply return ImmutableArray.Empty if all are empty?

Some consumers do not want this optimization.

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

cston · 2019-04-09T18:04:11Z

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PETypeParameterSymbol.cs

+                    {
+                        constraints = default(GenericParameterConstraintHandleCollection);
+                        Interlocked.CompareExchange(ref _lazyConstraintsUseSiteErrorInfo, new CSDiagnosticInfo(ErrorCode.ERR_BindToBogus, this), CSDiagnosticInfo.EmptyErrorInfo);
+                    }


Please extract this to a helper method, shared with GetDeclaredConstraintTypes(). #Resolved

jaredpar · 2019-04-10T16:45:30Z

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

@@ -568,7 +568,7 @@ internal bool GetIsReferenceType(ConsList<TypeParameterSymbol> inProgress)
        }

        // https://github.com/dotnet/roslyn/issues/26198 Should this API be exposed through ITypeParameterSymbol?
-        internal bool? IsNotNullableIfReferenceType => GetIsNotNullableIfReferenceType();
+        internal abstract bool? IsNotNullableIfReferenceType { get; }


Should this be IsNonNullableIfReferenceType? Generally we use Non in the APIs over Not. Want to make sure that's not significant here. #Resolved

Should this be IsNonNullableIfReferenceType? Generally we use Non in the APIs over Not. Want to make sure that's not significant here.

This makes no difference to me. At the moment this is not a public API, we can revisit the name if we decide to make it public.

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

jaredpar

AlekseyTs · 2019-04-10T22:57:47Z

@gafter, @cston Could you please review the last commit, it fixes a scenario I missed before?

* dotnet/master: (495 commits) Roslyn Installer: Stop processes that block VSIX installation. (dotnet#34886) Remove unused helper BeginInvokeOnUIThread Apply a hang mitigating timeout to InvokeOnUIThread Apply a hang mitigating timeout in RestoreNuGetPackages Apply a hang mitigating timeout to WaitForApplicationIdle Fix Value/Ref checks for pattern Index/Range (dotnet#35004) Fix assert in remove unused member analyzer Treat unconstrained type parameters declared in `#nullable disable` context as having an oblivious nullability in case they are substituted with a reference type. (dotnet#34797) Add symbol name to tests. Fix to be the correct name emitted PR feedback Improve IDE0052 diagnostic message for properties with used setter but unused getter Use original definition symbol for fetching control flow graph of generic local functions. Properly treat ambiguous explicit interface implementations (dotnet#34584) Remove the dependence between the order in NullableAnnotation and metadata attribute values (dotnet#34909) Fix warning level test. Fix bootstrap on Linux/Mac (dotnet#34978) disable completion for immediate window commands (dotnet#32631) Updated PreviewWarning color Implement design changes for "pattern" Index/Range indexers (dotnet#34897) Add IVTs to 16.1 version of RemoteLS ...

Treat unconstrained type parameters declared in ``#nullable disable…

080d432

…`` context as having an unknown nullability in case they are substituted with a reference type. As apposed to possibly nullable or not nullable reference type. Fixes dotnet#29980.

AlekseyTs added the Area-Compilers label Apr 5, 2019

AlekseyTs requested a review from a team as a code owner April 5, 2019 19:17

AlekseyTs added the Feature - Nullable Reference Types Nullable Reference Types label Apr 5, 2019

gafter reviewed Apr 5, 2019

View reviewed changes

AlekseyTs mentioned this pull request Apr 5, 2019

Use of generic type parameters declared in [NonNullTypes(false)] context should not cause any "nullable" diagnostics on use #30214

Closed

gafter self-assigned this Apr 5, 2019

Strengthen test scenarios

baca72a

More test scenarios

b84946f

gafter reviewed Apr 8, 2019

View reviewed changes

PR feedback

3d4d0c5

gafter approved these changes Apr 8, 2019

View reviewed changes

cston reviewed Apr 9, 2019

View reviewed changes

cston approved these changes Apr 9, 2019

View reviewed changes

PR feedback

4fc7699

AlekseyTs mentioned this pull request Apr 9, 2019

Properly treat ambiguous explicit interface implementations #34584

Merged

AlekseyTs added 2 commits April 9, 2019 16:50

Merge remote-tracking branch 'dotnet/master' into Issue29980

822f080

Follow up on merge from master

5c62a66

jaredpar reviewed Apr 10, 2019

View reviewed changes

jaredpar approved these changes Apr 10, 2019

View reviewed changes

AlekseyTs mentioned this pull request Apr 10, 2019

Unexpected different between inferred type argument nullability and explicit #34844

Closed

AlekseyTs added 2 commits April 10, 2019 14:38

Merge remote-tracking branch 'dotnet/master' into Issue29980

0e3794d

Fixes dotnet#34844.

cda3fd2

Add some test scenarios

8d2048e

This was referenced Apr 11, 2019

Use of unannotated unconstrained type parameter in nullable diabled code #34889

Merged

Remove the dependence between the order in NullableAnnotation and metadata attribute values #34909

Merged

AlekseyTs added 2 commits April 15, 2019 13:55

Merge remote-tracking branch 'dotnet/master' into Issue29980

c30c856

Follow up on merge from master

159f7cc

AlekseyTs merged commit 651d3f0 into dotnet:master Apr 16, 2019

Treat unconstrained type parameters declared in #nullable disable context as having an unknown nullability in case they are substituted with a reference type. #34797

Treat unconstrained type parameters declared in #nullable disable context as having an unknown nullability in case they are substituted with a reference type. #34797

Conversation

AlekseyTs commented Apr 5, 2019 • edited Loading

gafter commented Apr 5, 2019 • edited Loading

gafter commented Apr 5, 2019

gafter Apr 5, 2019 • 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

gafter Apr 5, 2019 • edited by AlekseyTs 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gafter Apr 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gafter Apr 5, 2019 • edited Loading

Choose a reason for hiding this comment

gafter commented Apr 5, 2019 • edited Loading

gafter commented Apr 5, 2019 • edited Loading

gafter Apr 8, 2019 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

gafter Apr 8, 2019 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

gafter Apr 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gafter Apr 8, 2019 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gafter Apr 8, 2019 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

gafter left a comment

Choose a reason for hiding this comment

gafter Apr 8, 2019 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

gafter left a comment

Choose a reason for hiding this comment

AlekseyTs commented Apr 8, 2019

AlekseyTs commented Apr 9, 2019

cston Apr 9, 2019 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Apr 9, 2019 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

jaredpar Apr 10, 2019 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredpar left a comment

Choose a reason for hiding this comment

AlekseyTs commented Apr 10, 2019 • edited Loading

Treat unconstrained type parameters declared in `#nullable disable` context as having an unknown nullability in case they are substituted with a reference type. #34797

Treat unconstrained type parameters declared in `#nullable disable` context as having an unknown nullability in case they are substituted with a reference type. #34797

AlekseyTs commented Apr 5, 2019 •

edited

Loading

gafter commented Apr 5, 2019 •

edited

Loading

gafter Apr 5, 2019 •

edited

Loading

gafter Apr 5, 2019 •

edited by AlekseyTs

Loading

gafter Apr 5, 2019 •

edited

Loading

gafter Apr 5, 2019 •

edited

Loading

gafter commented Apr 5, 2019 •

edited

Loading

gafter commented Apr 5, 2019 •

edited

Loading

gafter Apr 8, 2019 •

edited by AlekseyTs

Loading

gafter Apr 8, 2019 •

edited by AlekseyTs

Loading

gafter Apr 8, 2019 •

edited

Loading

gafter Apr 8, 2019 •

edited by AlekseyTs

Loading

gafter Apr 8, 2019 •

edited by AlekseyTs

Loading

gafter Apr 8, 2019 •

edited by AlekseyTs

Loading

cston Apr 9, 2019 •

edited by AlekseyTs

Loading

cston Apr 9, 2019 •

edited by AlekseyTs

Loading

jaredpar Apr 10, 2019 •

edited by AlekseyTs

Loading

AlekseyTs commented Apr 10, 2019 •

edited

Loading