Fix regression with var pattern nullability #53691
Conversation
jcouv
added
Area-Compilers
New Language Feature - Nullable Reference Types
| @@ -319,7 +316,7 @@ public PossiblyConditionalState Clone() | |||
| tempMap.Add(rootTemp, (originalInputSlot, expressionType.Type)); | |||
|
|
|||
| var nodeStateMap = PooledDictionary<BoundDecisionDagNode, (PossiblyConditionalState state, bool believedReachable)>.GetInstance(); | |||
| nodeStateMap.Add(decisionDag.RootNode, (state: initialState.Clone(), believedReachable: true)); | |||
| nodeStateMap.Add(decisionDag.RootNode, (state: PossiblyConditionalState.Create(this), believedReachable: true)); | |||
There was a problem hiding this comment.
📝 The issue was that makeDagTempSlot above was recording the type of the var pattern local into State, based on correct state (from expressionType), but we'd drop that by using initialState which captured a prior state.
|
@RikkiGibson @cston @dotnet/roslyn-compiler for review. Thanks |
|
I'll take a look to see if this also fixed #46236 (Thanks Chuck) |
| @@ -442,7 +442,7 @@ public Enumerator GetEnumerator() | |||
| ConcurrentOperation: | |||
| ThrowHelper.ThrowInvalidOperationException_ConcurrentOperationsNotSupported(); | |||
| ReturnFound: | |||
| ref var value = ref entry._value; | |||
| ref TValue value = ref entry._value; | |||
There was a problem hiding this comment.
It wasn't clear to me why this needed to change.
There was a problem hiding this comment.
Wouldn't bootstrap without this change
| @@ -87,7 +87,7 @@ public TypeWithAnnotations ToTypeWithAnnotations(CSharpCompilation compilation, | |||
| if (Type?.IsTypeParameterDisallowingAnnotationInCSharp8() == true) | |||
| { | |||
| var type = TypeWithAnnotations.Create(Type, NullableAnnotation.NotAnnotated); | |||
| return State == NullableFlowState.MaybeDefault ? type.SetIsAnnotated(compilation) : type; | |||
| return (State == NullableFlowState.MaybeDefault || asAnnotatedType) ? type.SetIsAnnotated(compilation) : type; | |||
There was a problem hiding this comment.
Was this all that was needed to fix the 'foreach' scenario?
There was a problem hiding this comment.
Yes, the for loop starts with a local declaration. That's handled by VisitLocalDeclaration, which correctly calls ToAnnotatedTypeWithAnnotations when the local has an inferred type, but when we get to ToTypeWithAnnotations the annotation would get dropped.
ghost
modified the milestones:
There was a regression in
LearnFromDecisionDagin 16.10 for top-levelvarpatterns in #51001Also fix #52925 (type of
varshould have an annotation) and #46236 (GetDeclaredSymbolon suchvardeclarations)Note that any change to the inferred type of
varis breaking inref varscenario. We've hit this in bootstrap build and I've added a test to illustrate.