diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs index dcd5c3aee1f7f..1cf58e3364a98 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs @@ -462,6 +462,48 @@ private void AssertNoPlaceholderReplacements() } } + private void AddPlaceholderReplacement(BoundValuePlaceholderBase placeholder, BoundExpression expression, VisitResult result) + { +#if DEBUG + Debug.Assert(AreCloseEnough(placeholder.Type, result.RValueType.Type)); +#endif + + _resultForPlaceholdersOpt ??= PooledDictionary.GetInstance(); + _resultForPlaceholdersOpt.Add(placeholder, (expression, result)); + } + + private void RemovePlaceholderReplacement(BoundValuePlaceholderBase placeholder) + { + Debug.Assert(_resultForPlaceholdersOpt is { }); + bool removed = _resultForPlaceholdersOpt.Remove(placeholder); + Debug.Assert(removed); + } + + [Conditional("DEBUG")] + private static void AssertPlaceholderAllowedWithoutRegistration(BoundValuePlaceholderBase placeholder) + { + Debug.Assert(placeholder is { }); + + switch (placeholder.Kind) + { + case BoundKind.DeconstructValuePlaceholder: + case BoundKind.InterpolatedStringHandlerPlaceholder: + case BoundKind.InterpolatedStringArgumentPlaceholder: + case BoundKind.ObjectOrCollectionValuePlaceholder: + case BoundKind.AwaitableValuePlaceholder: + return; + + case BoundKind.ImplicitIndexerValuePlaceholder: + // Since such placeholders are always not-null, we skip adding them to the map. + return; + + default: + // Newer placeholders are expected to follow placeholder discipline, namely that + // they must be added to the map before they are visited, then removed. + throw ExceptionUtilities.UnexpectedValue(placeholder.Kind); + } + } + protected override ImmutableArray Scan(ref bool badRegion) { if (_returnTypesOpt != null) @@ -4177,12 +4219,14 @@ private void LearnFromNonNullTest(BoundExpression expression, ref LocalState sta { if (expression is BoundValuePlaceholderBase placeholder) { - if (_resultForPlaceholdersOpt != null && _resultForPlaceholdersOpt.TryGetValue(placeholder, out var value)) + if (_resultForPlaceholdersOpt != null && + _resultForPlaceholdersOpt.TryGetValue(placeholder, out var value)) { expression = value.Replacement; } else { + AssertPlaceholderAllowedWithoutRegistration(placeholder); return; } } @@ -8710,10 +8754,9 @@ private TypeWithAnnotations GetDeclaredParameterResult(ParameterSymbol parameter var receiverResult = _visitResult; VisitRvalue(node.Argument); - EnsurePlaceholdersToResultMap(); - _resultForPlaceholdersOpt.Add(node.ReceiverPlaceholder, (node.Receiver, receiverResult)); + AddPlaceholderReplacement(node.ReceiverPlaceholder, node.Receiver, receiverResult); VisitRvalue(node.IndexerOrSliceAccess); - _resultForPlaceholdersOpt.Remove(node.ReceiverPlaceholder); + RemovePlaceholderReplacement(node.ReceiverPlaceholder); SetResult(node, ResultType, LvalueResultType); return null; @@ -8721,6 +8764,8 @@ private TypeWithAnnotations GetDeclaredParameterResult(ParameterSymbol parameter public override BoundNode? VisitImplicitIndexerValuePlaceholder(BoundImplicitIndexerValuePlaceholder node) { + // These placeholders don't need to be replaced because we know they are always not-null + AssertPlaceholderAllowedWithoutRegistration(node); SetNotNullResult(node); return null; } @@ -8980,11 +9025,10 @@ protected override void VisitForEachExpression(BoundForEachStatement node) { var moveNextAsyncMethod = (MethodSymbol)AsMemberOfType(reinferredGetEnumeratorMethod.ReturnType, node.EnumeratorInfoOpt.MoveNextInfo.Method); - EnsurePlaceholdersToResultMap(); var result = new VisitResult(GetReturnTypeWithState(moveNextAsyncMethod), moveNextAsyncMethod.ReturnTypeWithAnnotations); - _resultForPlaceholdersOpt.Add(moveNextPlaceholder, (moveNextPlaceholder, result)); + AddPlaceholderReplacement(moveNextPlaceholder, moveNextPlaceholder, result); Visit(awaitMoveNextInfo); - _resultForPlaceholdersOpt.Remove(moveNextPlaceholder); + RemovePlaceholderReplacement(moveNextPlaceholder); } // Analyze `await DisposeAsync()` @@ -8996,9 +9040,8 @@ protected override void VisitForEachExpression(BoundForEachStatement node) { Debug.Assert(disposalPlaceholder is not null); var disposeAsyncMethod = (MethodSymbol)AsMemberOfType(reinferredGetEnumeratorMethod.ReturnType, originalDisposeMethod); - EnsurePlaceholdersToResultMap(); var result = new VisitResult(GetReturnTypeWithState(disposeAsyncMethod), disposeAsyncMethod.ReturnTypeWithAnnotations); - _resultForPlaceholdersOpt.Add(disposalPlaceholder, (disposalPlaceholder, result)); + AddPlaceholderReplacement(disposalPlaceholder, disposalPlaceholder, result); addedPlaceholder = true; } @@ -9006,7 +9049,7 @@ protected override void VisitForEachExpression(BoundForEachStatement node) if (addedPlaceholder) { - _resultForPlaceholdersOpt!.Remove(disposalPlaceholder!); + RemovePlaceholderReplacement(disposalPlaceholder!); } } } @@ -9376,10 +9419,9 @@ private TypeWithState InferResultNullabilityOfBinaryLogicalOperator(BoundExpress var placeholder = awaitableInfo.AwaitableInstancePlaceholder; Debug.Assert(placeholder is object); - EnsurePlaceholdersToResultMap(); - _resultForPlaceholdersOpt.Add(placeholder, (node.Expression, _visitResult)); + AddPlaceholderReplacement(placeholder, node.Expression, _visitResult); Visit(awaitableInfo); - _resultForPlaceholdersOpt.Remove(placeholder); + RemovePlaceholderReplacement(placeholder); if (node.Type.IsValueType || node.HasErrors || awaitableInfo.GetResult is null) { @@ -9864,12 +9906,16 @@ private static bool IsNullabilityMismatch(TypeSymbol type1, TypeSymbol type2) public override BoundNode? VisitInterpolatedStringHandlerPlaceholder(BoundInterpolatedStringHandlerPlaceholder node) { + // These placeholders don't yet follow proper placeholder discipline + AssertPlaceholderAllowedWithoutRegistration(node); SetNotNullResult(node); return null; } public override BoundNode? VisitInterpolatedStringArgumentPlaceholder(BoundInterpolatedStringArgumentPlaceholder node) { + // These placeholders don't yet follow proper placeholder discipline + AssertPlaceholderAllowedWithoutRegistration(node); SetNotNullResult(node); return null; } @@ -9985,37 +10031,39 @@ protected override void VisitCatchBlock(BoundCatchBlock node, ref LocalState fin public override BoundNode? VisitDeconstructValuePlaceholder(BoundDeconstructValuePlaceholder node) { + // These placeholders don't yet follow proper placeholder discipline + AssertPlaceholderAllowedWithoutRegistration(node); SetNotNullResult(node); return null; } public override BoundNode? VisitObjectOrCollectionValuePlaceholder(BoundObjectOrCollectionValuePlaceholder node) { + // These placeholders don't yet follow proper placeholder discipline + AssertPlaceholderAllowedWithoutRegistration(node); SetNotNullResult(node); return null; } - [MemberNotNull(nameof(_resultForPlaceholdersOpt))] - private void EnsurePlaceholdersToResultMap() - { - _resultForPlaceholdersOpt ??= PooledDictionary.GetInstance(); - } - public override BoundNode? VisitAwaitableValuePlaceholder(BoundAwaitableValuePlaceholder node) { + // These placeholders don't always follow proper placeholder discipline yet + AssertPlaceholderAllowedWithoutRegistration(node); VisitPlaceholderWithReplacement(node); return null; } private void VisitPlaceholderWithReplacement(BoundValuePlaceholderBase node) { - if (_resultForPlaceholdersOpt != null && _resultForPlaceholdersOpt.TryGetValue(node, out var value)) + if (_resultForPlaceholdersOpt != null && + _resultForPlaceholdersOpt.TryGetValue(node, out var value)) { var result = value.Result; SetResult(node, result.RValueType, result.LValueType); } else { + AssertPlaceholderAllowedWithoutRegistration(node); SetNotNullResult(node); } }