Remove NonNullTypes context and other unnecessary information stored in TypeSymbolWithAnnotations. #30913

AlekseyTs · 2018-11-01T19:09:15Z

…in TypeSymbolWithAnnotations. Fixes dotnet#30845.

AlekseyTs · 2018-11-01T20:20:25Z

Test windows_release_unit64_prtest please

cston · 2018-11-01T20:23:49Z

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

+        NotNullable, // For string, int, T
+        Nullable,    // For string?, T? where T : class; and for int?, T? where T : struct.
+        NotNullableBasedOnAnalysis, // Explicitly set by flow analysis
+        NullableBasedOnAnalysis, // Explicitly set by flow analysis


Could the Analysis part be a separate bit?

[Flags] enum NullableAnnotation { Unknown = 0, NotNullable = 1, Nullable = 2, FromAnalysis = 4, }

Or a separate bool field on TypeSymbolWithAnnotations?

Could the Analysis part be a separate bit?

I thought about that, but for now decided against using flags to avoid the need to deal with "not meaningful"/impossible combinations. Also, I am hoping to get rid of NullableBasedOnAnalysis in the next change, making all values fit in two bits.

Or a separate bool field on TypeSymbolWithAnnotations?

I find it more convenient to deal with single value than with a pair of values. Going forward we should be able to get rid of Create methods that don't take NullableAnnotation.

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

I find it more convenient to deal with single value than with a pair of values.

Which callers are treating the pair as one value?

In reply to: 230193197 [](ancestors = 230193197,230184941)

Which callers are treating the pair as one value?

I think every caller as it stands right now, the TypeSymbolWithAnnotations itself treats it as one value when merging transitioning between annotations. With pair of values it is not clear what combinations the code should be prepared to deal with.

In reply to: 230195433 [](ancestors = 230195433,230193197,230184941)

cston · 2018-11-01T22:02:46Z

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

+        Nullable,    // For string?, T? where T : class; and for int?, T? where T : struct.
+        NotNullableBasedOnAnalysis, // Explicitly set by flow analysis
+        NullableBasedOnAnalysis, // Explicitly set by flow analysis
+    }


If the additional field (NotNullableBasedOnAnalysis) is for marking unconstrained type parameter references that are not nullable, perhaps the Nullable and NotNullable values should be independent bits, with the nullability of unconstrained type parameter references initially set to Nullable | NotNullable? Then flow analysis might infer the reference is NotNullable only.

With this structure, we should be able to accurately reflect what modifiers are in source code. Therefore, always pretending that there is a modifier when there isn't one in source, or always pretending that something is nullable when it isn't marked as nullable in source has a negative impact on checks we do on declarations. Checks on declarations should be performed specifically based on presence, or lack of '?' question mark in source code. Then, flow analysis can take that information coming from declarations and interpret it as it is appropriate for the purpose of flow analysis, however the structure itself shouldn't do this interpretation as it used to do before this change.

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

At some point I was thinking naming NotNullable as None (or something similar) to reflect exactly the fact that simply there was no annotation on the declaration, and then it is up to flow analysis to figure out what that means for a specific type.

In reply to: 230218492 [](ancestors = 230218492,230214729)

jcouv · 2018-11-02T21:50:28Z

src/Compilers/Core/Portable/Symbols/TypeCompareKind.cs

@@ -10,7 +10,7 @@ namespace Microsoft.CodeAnalysis
    [Flags]
    internal enum TypeCompareKind
    {
-        ConsiderEverything = 0, // Perhaps rename since this does not consider nullable modifiers.
+        ConsiderEverything = 0,


ConsiderEverything [](start = 8, length = 18)

Consider leaving a comment or xml doc to indicate this option does not compare nullability. #Closed

Consider leaving a comment or xml doc to indicate this option does not compare nullability.

But it does, the comment was obsolete.

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

jcouv · 2018-11-02T22:09:52Z

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

+                        return true;
+
+                    case NullableAnnotation.NotNullableBasedOnAnalysis:
+                        return false;


return false; [](start = 24, length = 13)

nit: consider collapsing the two "not nullable" cases, as you did for the two "nullable" cases.
We could probably also assert in "not nullable" cases, as you did for the "unknown" case (Debug.Assert(!TypeSymbol.IsNullableType());). #Closed

jcouv · 2018-11-02T22:20:48Z

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

-                result.AsNotNullableReferenceType();
+            if (isAnnotated)
+            {
+                result = result.AsNullableReferenceType(fromDeclaration: true);


fromDeclaration: true [](start = 56, length = 21)

This is "from declaration" because we only use this method when loading from PE, correct?
If so, it may be good to comment.

Update: if you think "from declaration" will disappear in next iteration, then don't bother with comment. #Closed

This is "from declaration" because we only use this method when loading from PE, correct?

No this is not correct. It is also used to manipulate symbols while binding declarations. It is never used by flow analysis. BTW, we are going to get rid of nonNullTypesContext and change the ImmutableArray<bool> transforms to be an array of NullableAnnotation. So we will not need to guess or assume what value to use.

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

jcouv · 2018-11-02T22:32:11Z

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

            {
-                return CreateNonLazyType(type._defaultType, type.NonNullTypesContext, isAnnotated: true,
-                    type._treatPossiblyNullableReferenceTypeTypeParameterAsNullable, _customModifiers);
+                return CreateNonLazyType(type._defaultType, fromDeclaration ? NullableAnnotation.Nullable : NullableAnnotation.NullableBasedOnAnalysis, _customModifiers);
            }

            internal override TypeSymbolWithAnnotations AsNotNullableReferenceType(TypeSymbolWithAnnotations type)


AsNotNullableReferenceType [](start = 56, length = 26)

Should we have a "from declaration" flag here too? It looks like AsNotNullableReferenceType is used in method type inference, which is run during both binding and flow analysis. #Closed

Should we have a "from declaration" flag here too? It looks like AsNotNullableReferenceType is used in method type inference, which is run during both binding and flow analysis.

At the moment there is no client that would use it. We'll add on as needed basis.

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

The client would be method type inference. Since it can be run during flow analysis, it feels like it should be passing a fromDeclaration: false in that scenario.

In reply to: 230951976 [](ancestors = 230951976,230525559)

The client would be method type inference. Since it can be run during flow analysis, it feels like it should be passing a fromDeclaration: false in that scenario.

Feel free to come up with a scenario that would indicate the need, so far we don't have any tests indicating that.

In reply to: 230958183 [](ancestors = 230958183,230951976,230525559)

It seems that the distinction between NotNullable and NotNullableBasedOnAnalysis currently does nothing. So if we're incorrectly producing one rather than the other, there would be no way to observe that.

This makes me think one of those two flags should be removed. Then the question of whether we're producing the correct flag here becomes mute ;-)

In reply to: 230958960 [](ancestors = 230958960,230958183,230951976,230525559)

This makes me think one of those two flags should be removed. Then the question of whether we're producing the correct flag here becomes mute ;-)

At the moment, NotNullableBasedOnAnalysis affects behavior of SetPossiblyNullableReferenceTypeTypeParametersAsNullable in the way that we don't overwrite types that flow from earlier analysis. More real usage is coming in the next commit.

In reply to: 230959880 [](ancestors = 230959880,230958960,230958183,230951976,230525559)

I'm sorry, I didn't follow. I looked at all the references to NotNullableBasedOnAnalysis in this PR, and none of them behaves differently when encountering a NotNullableBasedOnAnalysis or a NotNullable.

In reply to: 230961770 [](ancestors = 230961770,230959880,230958960,230958183,230951976,230525559)

I looked at all the references to NotNullableBasedOnAnalysis in this PR, and none of them behaves differently when encountering a NotNullableBasedOnAnalysis or a NotNullable.

The following method behaves differently:

internal bool IsPossiblyNullableReferenceTypeTypeParameter() { return NullableAnnotation == NullableAnnotation.NotNullable && TypeSymbol.IsPossiblyNullableReferenceTypeTypeParameter(); }

In reply to: 230962617 [](ancestors = 230962617,230961770,230959880,230958960,230958183,230951976,230525559)

jcouv · 2018-11-02T23:01:40Z

src/Compilers/CSharp/Portable/BoundTree/UnboundLambda.cs

@@ -181,7 +181,7 @@ internal static InferredLambdaReturnType InferReturnType(ArrayBuilder<(RefKind,
                    // to the caller, and up through MethodTypeInferrer.Infer.
                    bestResultType = hadNullabilityMismatch ?
                        default :
-                        TypeSymbolWithAnnotations.Create(bestType, isNullableIfReferenceType: BestTypeInferrer.GetIsNullable(resultTypes));
+                        TypeSymbolWithAnnotations.Create(bestType, isNullableIfReferenceType: BestTypeInferrer.GetIsNullable(resultTypes), fromDeclaration: true);


fromDeclaration: true [](start = 139, length = 21)

The calculation of lambda return type is re-done during flow analysis, so it feels like we shouldn't always use "fromDeclaration: true".

public TypeSymbolWithAnnotations GetInferredReturnType(ConversionsBase conversions, NullableWalker.VariableState nullableState, ref HashSet<DiagnosticInfo> useSiteDiagnostics) { ... if (nullableState == null) { return InferredReturnType.Type; } else { ... NullableWalker.Analyze(compilation, lambda: this, diagnostics, delegateInvokeMethod: delegateType?.DelegateInvokeMethod, returnTypes: returnTypes, initialState: nullableState); var inferredReturnType = InferReturnType(returnTypes, compilation, conversions, delegateType, Symbol.IsAsync); ... } } ``` #Closed

The calculation of lambda return type is re-done during flow analysis, so it feels like we shouldn't always use "fromDeclaration: true".

Some of the changes are simply preserving existing behavior, often relying on existing tests to verify that. The current plan is to remove all APIs that use "bool?" to create TypeSymbolWithAnnotations. Annotations from flow analysis will naturally flow through here after the change.

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

jcouv · 2018-11-02T23:04:29Z

src/Compilers/CSharp/Portable/FlowAnalysis/DataFlowPass.VariableIdentifier.cs

            }
            public bool Equals(VariableIdentifier other)
            {
-                return ((object)Symbol == null ? (object)other.Symbol == null : Symbol.Equals(other.Symbol)) && ContainingSlot == other.ContainingSlot;
+                return ((object)Symbol == null ? (object)other.Symbol == null : Symbol.OriginalDefinition.Equals(other.Symbol.OriginalDefinition)) && ContainingSlot == other.ContainingSlot;


Symbol.OriginalDefinition.Equals(other.Symbol.OriginalDefinition)) [](start = 80, length = 66)

I didn't understand this part of the change. What was the impact? #Closed

I didn't understand this part of the change. What was the impact?

The impact is that slight change in nested nullability of containing type (caused by SetPossiblyNullableReferenceTypeTypeParametersAsNullable, for example) won't have negative effect on allocating/finding slots. Container is already uniquely and accurately identified by ContainingSlot, so we can safely rely on definition to match members

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

Thanks for the explanation

In reply to: 230954470 [](ancestors = 230954470,230530406)

jcouv · 2018-11-02T23:09:22Z

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

-                returnType;
+            if (method.IsGenericTaskReturningAsync(compilation))
+            {
+                returnType = ((NamedTypeSymbol)returnType.TypeSymbol).TypeArgumentsNoUseSiteDiagnostics.Single().SetPossiblyNullableReferenceTypeTypeParametersAsNullable();


SetPossiblyNullableReferenceTypeTypeParametersAsNullable [](start = 113, length = 56)

I'm not sure if we need this second call. returnType should already have been "cleansed", including its type arguments. #Closed

I'm not sure if we need this second call. returnType should already have been "cleansed", including its type arguments.

That is correct. I am planning to get rid of all the SetPossiblyNullableReferenceTypeTypeParametersAsNullable cleansing everywhere in my next commit regardless.

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

jcouv · 2018-11-03T00:26:36Z

    /// null if an unannotated reference type and [NonNullTypes(false)].

The comment is out-of-date.
Also, it would be good to clarify expected behavior for unconstrained T. My understanding is that IsNullable will be false (NotNullable during binding) and the caller should check for unconstrained type parameter case, but during flow analysis it would be true. #Closed

Refers to: src/Compilers/CSharp/Portable/Symbols/TypeSymbolWithAnnotations.cs:314 in a2ffdaa. [](commit_id = a2ffdaa, deletion_comment = False)

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

jcouv · 2018-11-03T00:39:48Z

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

@@ -24461,7 +24501,7 @@ static void F<T>()
            var declarators = tree.GetRoot().DescendantNodes().OfType<VariableDeclaratorSyntax>().ToArray();
            var symbol = (LocalSymbol)model.GetDeclaredSymbol(declarators[0]);
            Assert.Equal("T", symbol.Type.ToTestDisplayString(true));
-            Assert.Equal(true, symbol.Type.IsNullable);
+            Assert.False(symbol.Type.IsNullable);


Assert.False(symbol.Type.IsNullable); [](start = 12, length = 37)

Is this the behavior we expect for public API as well, when asking about unconstrained type parameter?
It feels like the public and internal APIs should probably align for this question. #Closed

Is this the behavior we expect for public API as well, when asking about unconstrained type parameter?
It feels like the public and internal APIs should probably align for this question.

We are still figuring out whet the shape of public API should be.
I believe this internal API is misleading and I am planning to get rid of it internally.

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

jcouv · 2018-11-03T00:42:38Z

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

@@ -16,21 +16,22 @@ internal static class NullableTypeDecoder
        internal static TypeSymbolWithAnnotations TransformType(
            TypeSymbolWithAnnotations metadataType,
            EntityHandle targetSymbolToken,
-            PEModuleSymbol containingModule)
+            PEModuleSymbol containingModule,
+            INonNullTypesContext nonNullTypesContext)


INonNullTypesContext nonNullTypesContext) [](start = 12, length = 41)

This seems a significant change/fix, but I didn't see test changes corresponding to it. Do we have adequate coverage? #Closed

This seems a significant change/fix, but I didn't see test changes corresponding to it. Do we have adequate coverage?

Why do you see this as a significant change? Context, which was "inferred" from TypeSymbolWithAnnotations before is now passed explicitly. The final goal is to get rid of INonNullTypesContext completely, but we have to do it in steps to keep changes to a reasonable size.

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

Thanks for the clarification. Makes sense

In reply to: 230957799 [](ancestors = 230957799,230539140)

jcouv · 2018-11-03T00:47:22Z

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

@@ -100,7 +100,7 @@ internal override ImmutableArray<TypeSymbolWithAnnotations> GetConstraintTypes(C
        {
            var constraintTypes = ArrayBuilder<TypeSymbolWithAnnotations>.GetInstance();
            _map.SubstituteTypesDistinctWithoutModifiers(_underlyingTypeParameter.GetConstraintTypes(inProgress, early), constraintTypes, null);
-            return constraintTypes.ToImmutableAndFree().WhereAsArray(type => type.SpecialType != SpecialType.System_Object || !type.IsAnnotated);
+            return constraintTypes.ToImmutableAndFree().WhereAsArray(type => type.SpecialType != SpecialType.System_Object || !type.NullableAnnotation.IsAnyNullable());


type.SpecialType != SpecialType.System_Object || !type.NullableAnnotation.IsAnyNullable() [](start = 77, length = 89)

Would it make sense to extract this logic out to a helper function (maybe type => !type.IsNullableObject())? It also appears in PETypeParameterSymbol.GetDeclaredConstraintTypes as part of this PR. #Closed

Would it make sense to extract this logic out to a helper function (maybe type => !type.IsNullableObject())? It also appears in PETypeParameterSymbol.GetDeclaredConstraintTypes as part of this PR.

I personally do not see much value doing this at this point.

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

jcouv · 2018-11-03T00:50:52Z

            return inferredReturnType.Type;

Should we be using SetPossiblyNullableReferenceTypeTypeParametersAsNullable here too? #Closed

Refers to: src/Compilers/CSharp/Portable/BoundTree/UnboundLambda.cs:115 in a2ffdaa. [](commit_id = a2ffdaa, deletion_comment = False)

jcouv

Done with review pass (iteration 1)

AlekseyTs · 2018-11-05T23:44:29Z

    /// null if an unannotated reference type and [NonNullTypes(false)].

Also, it would be good to clarify expected behavior for unconstrained T. My understanding is that IsNullable will be false (NotNullable during binding) and the caller should check for unconstrained type parameter case, but during flow analysis it would be true.

My plan is to get rid of this property completely. Whether a type should be considered nullable or not depends on the context. This type should be data only, no analysis.

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

Refers to: src/Compilers/CSharp/Portable/Symbols/TypeSymbolWithAnnotations.cs:314 in a2ffdaa. [](commit_id = a2ffdaa, deletion_comment = False)

AlekseyTs · 2018-11-06T00:03:40Z

            return inferredReturnType.Type;

Should we be using SetPossiblyNullableReferenceTypeTypeParametersAsNullable here too?

I do not think so. The SetPossiblyNullableReferenceTypeTypeParametersAsNullable helper is a temporary solution to simply have existing tests passing. My next commit eliminates it everywhere.

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

Refers to: src/Compilers/CSharp/Portable/BoundTree/UnboundLambda.cs:115 in a2ffdaa. [](commit_id = a2ffdaa, deletion_comment = False)

jcouv · 2018-11-06T00:16:10Z

            return inferredReturnType.Type;

Looking at this more, I think we're ok because there is no way to use a lambda directly.
For instance, (() => true ? new T() : default)().ToString(); is not possible (should warn if it were). Instead, you must assign the lambda to a delegate type first, which removes the problem.

In reply to: 436082310 [](ancestors = 436082310,435547738)

Refers to: src/Compilers/CSharp/Portable/BoundTree/UnboundLambda.cs:115 in a2ffdaa. [](commit_id = a2ffdaa, deletion_comment = False)

jcouv

First iteration looks good to me. I'll wait for second commit to sign-off. Thanks

Related to dotnet#29981, dotnet#29993

jcouv · 2018-11-06T18:22:53Z

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

+            return NullableAnnotation == NullableAnnotation.NotNullable && TypeSymbol.IsPossiblyNullableReferenceTypeTypeParameter();
+        }
+
+        internal NullableAnnotation GetValueNullableAnnotation()


GetValueNullableAnnotation [](start = 36, length = 26)

nit: I found this method name confusing. What does "Value" refer to?
Perhaps GetNullableAnnotationForAnalysis would be clearer? #Closed

I found this method name confusing. What does "Value" refer to?

It refers to the fact that we are talking about nullability of a value of this type. There is a distinction between a value and a target of an assignment. Both are involved into analysis, but handled differently even if they have the same type.

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

jcouv · 2018-11-08T21:52:08Z

    private static bool IsNullabilityMismatch(TypeSymbolWithAnnotations type1, TypeSymbolWithAnnotations type2)

Would it make sense to xml comment this method (and overload) to point out that it cares about insignificant nullability differences?

I also wonder if we'll need the new comparison option in the long-term. As you pointed out, refactoring ReportAssignmentWarnings and related methods (which depend on IsNullabilityMismatch) could remove the need for callers of ReportAssignmentWarnings to make adjustments and for IsNullabilityMismatch to do an exact comparison?
If you think that is likely possible, consider filing an issue. #Resolved

Refers to: src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs:4433 in f20e071. [](commit_id = f20e071, deletion_comment = False)

AlekseyTs · 2018-11-08T22:39:51Z

    private static bool IsNullabilityMismatch(TypeSymbolWithAnnotations type1, TypeSymbolWithAnnotations type2)

Would it make sense to xml comment this method (and overload) to point out that it cares about insignificant nullability differences?

I don't think it is necessary, they do what the flow analysis needs to do.

I also wonder if we'll need the new comparison option in the long-term. As you pointed out, refactoring ReportAssignmentWarnings and related methods (which depend on IsNullabilityMismatch) could remove the need for callers of ReportAssignmentWarnings to make adjustments and for IsNullabilityMismatch to do an exact comparison?

Refactoring ReportAssignmentWarnings would only help us with top level nullability, but we need to handle it as deep as it can go. So, I don't think this is a short term option.

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

Refers to: src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs:4433 in f20e071. [](commit_id = f20e071, deletion_comment = False)

cston · 2018-11-09T00:44:59Z

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

+
+    internal static class NullableAnnotationExtensions
+    {
+        public static bool IsAnyNullable(this NullableAnnotation annotation)


IsAny [](start = 27, length = 5)

I trip over IsAny when I see these methods called since Any is usually used for a collection. Consider naming these methods IsNullable() and IsNotNullable(). #Resolved

I trip over IsAny when I see these methods called since Any is usually used for a collection. Consider naming these methods IsNullable() and IsNotNullable().

This is exactly the effect I want, I want people to think and understand what question these methods are answering. I think that the proposed names are going to be misleading, they would almost match specific enum values.

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

cston · 2018-11-09T01:08:21Z

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

-            _resultType = TypeSymbolWithAnnotations.Create(resultType, resultIsNullable);
+            NullableAnnotation resultNullableAnnotation;
+
+            if (getValueNullableAnnotation(leftOperand, leftResult).IsAnyNotNullable())


getValueNullableAnnotation(leftOperand, leftResult) [](start = 16, length = 51)

I don't understand why we use getValueNulalbleAnnotation(left) for the if but then set the result type to getNullableAnnotation(left). Please consider adding a comment for which cases that distinction is important. #Resolved

I don't understand why we use getValueNulalbleAnnotation(left) for the if but then set the result type to getNullableAnnotation(left). Please consider adding a comment for which cases that distinction is important.

We want to check if the value can be nullable based on annotations and getValueNullableAnnotation might adjust value that it returns to emphasize the fact. However, we want the original annotation to flow through the system. I will add a comment.

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

cston · 2018-11-09T01:12:59Z

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

+                                // This code path is likely not reachable due to https://github.com/dotnet/roslyn/issues/30946.
+                                // See unit test NullabilityOfTypeParameters_080 for an attempt to get here.
+                                adjustedResultType = TypeSymbolWithAnnotations.Create(adjustedResultType.TypeSymbol, NullableAnnotation.NullableBasedOnAnalysis);
+                            }


This code snippet is used three times in this method. Consider extracting a local function. #Pending

This code snippet is used three times in this method. Consider extracting a local function.

Will do

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

cston · 2018-11-09T01:21:48Z

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

+                        (b == NullableAnnotation.NotNullable && a == NullableAnnotation.NotNullableBasedOnAnalysis));
+                    return NullableAnnotation.NotNullable; // It is reasonable to settle on this value because the difference in annotations is either
+                                                           // not significant for the type, or candidate corresponding to this value is possibly a 
+                                                           // nullable reference type type parameter and nullable should win. 


nullable [](start = 62, length = 8)

not nullable? #Resolved

not nullable?

No, candidate with NullableAnnotation.NotNullable could be a reference type parameter that can be substituted with a nullable reference type

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

cston · 2018-11-09T01:29:04Z

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

+                    {
+                        Debug.Assert(thisAnnotation.IsAnyNotNullable());
+                        Debug.Assert(otherAnnotation.IsAnyNotNullable());
+                        if (TypeSymbol.IsPossiblyNullableReferenceTypeTypeParameter())


if (TypeSymbol.IsPossiblyNullableReferenceTypeTypeParameter()) [](start = 24, length = 62)

What if other.TypeSymbol.IsPossiblyNullableReferenceTypeTypeParameter() instead? Do we return false in that case? #Resolved

What if other.TypeSymbol.IsPossiblyNullableReferenceTypeTypeParameter() instead? Do we return false in that case?

If we reach here types are the same, they are compared at the beginning of this function.

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

cston · 2018-11-09T01:31:58Z

...Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol_ImplementationChecks.cs

@@ -836,7 +836,7 @@ private void CheckNewModifier(Symbol symbol, bool isNew, DiagnosticBag diagnosti
                            {
                                var overridenParameterType = overriddenParameters[i].Type;
                                if (!overridingParameters[i].Type.Equals(overridenParameterType,
-                                                                         TypeCompareKind.AllIgnoreOptions & ~TypeCompareKind.IgnoreNullableModifiersForReferenceTypes) &&
+                                                                         TypeCompareKind.AllIgnoreOptions & ~(TypeCompareKind.IgnoreNullableModifiersForReferenceTypes | TypeCompareKind.IgnoreInsignificantNullableModifiersDifference)) &&


TypeCompareKind.AllIgnoreOptions & ~(TypeCompareKind.IgnoreNullableModifiersForReferenceTypes | TypeCompareKind.IgnoreInsignificantNullableModifiersDifference) [](start = 73, length = 159)

Consider adding a TypeCompareKind.AllNonNullableIgnoreOptions. #WontFix

Consider adding a TypeCompareKind.AllNonNullableIgnoreOptions.

This and other similar places are still using UnknownNullableModifierMatchesAny, which is one of the nullable ignore options.

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

…lWithAnnotations_merge

…lWithAnnotations

jcouv · 2018-11-09T23:33:35Z

@jaredpar for approval for preview2. Thanks

jaredpar · 2018-11-09T23:34:54Z

approved

…out-if-error-message * origin/master: (174 commits) Add Compilers filter for Roslyn (dotnet#30880) Remove NonNullTypes context and other unnecessary information stored in TypeSymbolWithAnnotations. (dotnet#30913) Update BoundCall method based on receiver nullability (dotnet#31000) Make nullabiulity inference associative and commutative (dotnet#30990) Async-streams: minimal test for IOperation and CFG. Improve diagnostics (dotnet#30363) Add src. Add test. Remove dead code Update the build status table Script for generating our build status tables Add parsing tests to compiler benchmarks (dotnet#31033) Fix Edit and Continue in CPS projects Add comment Sorting. Save work Address PR feedback only produce optprof data on an official build Add bunch of exclusions to dead code analysis for special methods Disable WinRT tests on Linux (dotnet#31026) Change prerelease version to beta2 (dotnet#31017) ... # Conflicts: # src/Compilers/CSharp/Portable/CSharpResources.resx # src/Compilers/CSharp/Portable/Errors/ErrorCode.cs

Remove NonNullTypes context and other unnecessary information stored …

a2ffdaa

…in TypeSymbolWithAnnotations. Fixes dotnet#30845.

AlekseyTs added Area-Compilers New Language Feature - Nullable Reference Types Nullable Reference Types labels Nov 1, 2018

AlekseyTs requested review from 333fred, cston and a team November 1, 2018 19:09

cston reviewed Nov 1, 2018

View reviewed changes

jcouv reviewed Nov 2, 2018

View reviewed changes

jcouv reviewed Nov 3, 2018

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/NamedTypeSymbol.cs Outdated Show resolved Hide resolved

jcouv reviewed Nov 3, 2018

View reviewed changes

jcouv self-assigned this Nov 3, 2018

jcouv reviewed Nov 6, 2018

View reviewed changes

Explicitly handle nullability analysis for unconstraint type parameters.

63cc0a1

Related to dotnet#29981, dotnet#29993

jcouv reviewed Nov 6, 2018

View reviewed changes

cston reviewed Nov 9, 2018

View reviewed changes

cston approved these changes Nov 9, 2018

View reviewed changes

AlekseyTs requested review from a team as code owners November 9, 2018 19:14

AlekseyTs changed the base branch from features/NullableReferenceTypes to master November 9, 2018 19:16

AlekseyTs added 3 commits November 9, 2018 11:45

PR feedback

8aa31a1

Merge remote-tracking branch 'dotnet/dev16.0-preview2' into TypeSymbo…

e3440c3

…lWithAnnotations_merge

Follow-up on merge from dev16.0-preview2

77a0527

AlekseyTs force-pushed the TypeSymbolWithAnnotations branch from 4b7908f to 77a0527 Compare November 9, 2018 20:23

AlekseyTs changed the base branch from master to dev16.0-preview2 November 9, 2018 20:23

AlekseyTs added 2 commits November 9, 2018 12:59

Merge remote-tracking branch 'dotnet/dev16.0-preview2' into TypeSymbo…

d3d7cd4

…lWithAnnotations

Merge remote-tracking branch 'dotnet/dev16.0-preview2' into TypeSymbo…

ff96c46

…lWithAnnotations

jaredpar added the Approved to merge label Nov 9, 2018

jcouv mentioned this pull request Nov 9, 2018

Possible Null Reference on t?.ToString() for unconstrained T #28792

Closed

AlekseyTs merged commit e63f327 into dotnet:dev16.0-preview2 Nov 9, 2018

cston mentioned this pull request Dec 17, 2018

Nullability: Failed pattern match makes compiler think variable is nullable #31803

Closed

Remove NonNullTypes context and other unnecessary information stored in TypeSymbolWithAnnotations. #30913

Remove NonNullTypes context and other unnecessary information stored in TypeSymbolWithAnnotations. #30913

Conversation

AlekseyTs commented Nov 1, 2018

AlekseyTs commented Nov 1, 2018

cston Nov 1, 2018 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Nov 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Nov 2, 2018 • edited Loading

Choose a reason for hiding this comment

jcouv Nov 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Nov 2, 2018 • 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

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 Nov 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Nov 2, 2018 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Nov 2, 2018 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 5, 2018 • edited Loading

Choose a reason for hiding this comment

jcouv commented Nov 3, 2018 • edited Loading

jcouv Nov 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Nov 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Nov 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv commented Nov 3, 2018 • edited Loading

jcouv left a comment

Choose a reason for hiding this comment

AlekseyTs commented Nov 5, 2018

AlekseyTs commented Nov 6, 2018

jcouv commented Nov 6, 2018

jcouv left a comment

Choose a reason for hiding this comment

jcouv Nov 6, 2018 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 6, 2018 • edited Loading

Choose a reason for hiding this comment

jcouv commented Nov 8, 2018 • edited by AlekseyTs Loading

AlekseyTs commented Nov 8, 2018

cston Nov 9, 2018 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Nov 9, 2018 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Nov 9, 2018 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Nov 9, 2018 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Nov 9, 2018 • edited by AlekseyTs Loading

cston Nov 1, 2018 •

edited

Loading

jcouv Nov 2, 2018 •

edited

Loading

jcouv Nov 2, 2018 •

edited

Loading

jcouv Nov 2, 2018 •

edited

Loading

jcouv Nov 2, 2018 •

edited

Loading

jcouv Nov 2, 2018 •

edited

Loading

jcouv Nov 2, 2018 •

edited

Loading

AlekseyTs Nov 5, 2018 •

edited

Loading

jcouv Nov 2, 2018 •

edited

Loading

AlekseyTs Nov 5, 2018 •

edited

Loading

jcouv commented Nov 3, 2018 •

edited

Loading

jcouv Nov 3, 2018 •

edited

Loading

jcouv Nov 3, 2018 •

edited

Loading

jcouv Nov 3, 2018 •

edited

Loading

jcouv commented Nov 3, 2018 •

edited

Loading

jcouv Nov 6, 2018 •

edited

Loading

AlekseyTs Nov 6, 2018 •

edited

Loading

jcouv commented Nov 8, 2018 •

edited by AlekseyTs

Loading

cston Nov 9, 2018 •

edited by AlekseyTs

Loading

cston Nov 9, 2018 •

edited by AlekseyTs

Loading

cston Nov 9, 2018 •

edited by AlekseyTs

Loading

cston Nov 9, 2018 •

edited by AlekseyTs

Loading

cston Nov 9, 2018 •

edited by AlekseyTs

Loading

cston Nov 9, 2018 •

edited by AlekseyTs

Loading

AlekseyTs Nov 9, 2018 •

edited

Loading