From 5277d325e6743a4230a664134d6e6bb98d21f19d Mon Sep 17 00:00:00 2001 From: Chris Sienkiewicz Date: Fri, 15 Mar 2019 14:23:48 -0700 Subject: [PATCH] Warn on __refvalue null dereference: (#34135) * Warn on __refvalue null dereference: - Store the nullable annotation of the type passed to __refvalue - Use that information in nullable walker to report nullability warnings - Add test --- .../Portable/Binder/Binder_Expressions.cs | 4 ++-- .../CSharp/Portable/BoundTree/BoundNodes.xml | 1 + .../Portable/FlowAnalysis/NullableWalker.cs | 2 +- .../Generated/BoundNodes.xml.Generated.cs | 16 ++++++++----- .../Semantics/NullableReferenceTypesTests.cs | 23 +++++++++++++++++++ 5 files changed, 37 insertions(+), 9 deletions(-) diff --git a/src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs b/src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs index 624f7eaa2d0..28095859578 100644 --- a/src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs +++ b/src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs @@ -981,9 +981,9 @@ private BoundExpression BindRefValue(RefValueExpressionSyntax node, DiagnosticBa argument = CreateConversion(argument, conversion, typedReferenceType, diagnostics); - TypeSymbol type = BindType(node.Type, diagnostics).Type; + TypeWithAnnotations typeWithAnnotations = BindType(node.Type, diagnostics); - return new BoundRefValueOperator(node, argument, type, hasErrors); + return new BoundRefValueOperator(node, typeWithAnnotations.NullableAnnotation, argument, typeWithAnnotations.Type, hasErrors); } private BoundExpression BindMakeRef(MakeRefExpressionSyntax node, DiagnosticBag diagnostics) diff --git a/src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml b/src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml index 0b9cb5d54b3..931bf35481f 100644 --- a/src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml +++ b/src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml @@ -329,6 +329,7 @@ + diff --git a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs index 14043538940..e79c637f400 100644 --- a/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs +++ b/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs @@ -5157,7 +5157,7 @@ public override BoundNode VisitMakeRefOperator(BoundMakeRefOperator node) public override BoundNode VisitRefValueOperator(BoundRefValueOperator node) { var result = base.VisitRefValueOperator(node); - var type = TypeWithAnnotations.Create(node.Type); + var type = TypeWithAnnotations.Create(node.Type, node.NullableAnnotation); LvalueResultType = type; return result; } diff --git a/src/Compilers/CSharp/Portable/Generated/BoundNodes.xml.Generated.cs b/src/Compilers/CSharp/Portable/Generated/BoundNodes.xml.Generated.cs index 69f8447c2de..a38952999e6 100644 --- a/src/Compilers/CSharp/Portable/Generated/BoundNodes.xml.Generated.cs +++ b/src/Compilers/CSharp/Portable/Generated/BoundNodes.xml.Generated.cs @@ -1237,17 +1237,20 @@ protected override BoundExpression ShallowClone() internal sealed partial class BoundRefValueOperator : BoundExpression { - public BoundRefValueOperator(SyntaxNode syntax, BoundExpression operand, TypeSymbol type, bool hasErrors = false) + public BoundRefValueOperator(SyntaxNode syntax, NullableAnnotation nullableAnnotation, BoundExpression operand, TypeSymbol type, bool hasErrors = false) : base(BoundKind.RefValueOperator, syntax, type, hasErrors || operand.HasErrors()) { Debug.Assert((object)operand != null, "Field 'operand' cannot be null (use Null=\"allow\" in BoundNodes.xml to remove this check)"); Debug.Assert((object)type != null, "Field 'type' cannot be null (use Null=\"allow\" in BoundNodes.xml to remove this check)"); + this.NullableAnnotation = nullableAnnotation; this.Operand = operand; } + public NullableAnnotation NullableAnnotation { get; } + public BoundExpression Operand { get; } public override BoundNode Accept(BoundTreeVisitor visitor) @@ -1255,11 +1258,11 @@ public override BoundNode Accept(BoundTreeVisitor visitor) return visitor.VisitRefValueOperator(this); } - public BoundRefValueOperator Update(BoundExpression operand, TypeSymbol type) + public BoundRefValueOperator Update(NullableAnnotation nullableAnnotation, BoundExpression operand, TypeSymbol type) { - if (operand != this.Operand || !TypeSymbol.Equals(type, this.Type, TypeCompareKind.ConsiderEverything)) + if (nullableAnnotation != this.NullableAnnotation || operand != this.Operand || !TypeSymbol.Equals(type, this.Type, TypeCompareKind.ConsiderEverything)) { - var result = new BoundRefValueOperator(this.Syntax, operand, type, this.HasErrors); + var result = new BoundRefValueOperator(this.Syntax, nullableAnnotation, operand, type, this.HasErrors); result.CopyAttributes(this); return result; } @@ -1268,7 +1271,7 @@ public BoundRefValueOperator Update(BoundExpression operand, TypeSymbol type) protected override BoundExpression ShallowClone() { - var result = new BoundRefValueOperator(this.Syntax, this.Operand, this.Type, this.HasErrors); + var result = new BoundRefValueOperator(this.Syntax, this.NullableAnnotation, this.Operand, this.Type, this.HasErrors); result.CopyAttributes(this); return result; } @@ -11164,7 +11167,7 @@ public override BoundNode VisitRefValueOperator(BoundRefValueOperator node) { BoundExpression operand = (BoundExpression)this.Visit(node.Operand); TypeSymbol type = this.VisitType(node.Type); - return node.Update(operand, type); + return node.Update(node.NullableAnnotation, operand, type); } public override BoundNode VisitFromEndIndexExpression(BoundFromEndIndexExpression node) { @@ -12366,6 +12369,7 @@ public override TreeDumperNode VisitRefValueOperator(BoundRefValueOperator node, { return new TreeDumperNode("refValueOperator", null, new TreeDumperNode[] { + new TreeDumperNode("nullableAnnotation", node.NullableAnnotation, null), new TreeDumperNode("operand", null, new TreeDumperNode[] { Visit(node.Operand, null) }), new TreeDumperNode("type", node.Type, null), new TreeDumperNode("isSuppressed", node.IsSuppressed, null) diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs index be307b87d47..ae4c6552a3a 100644 --- a/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs +++ b/src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs @@ -85835,5 +85835,28 @@ public struct Foo var comp = CreateCompilation(new[] { source }, options: WithNonNullTypesTrue()); comp.VerifyDiagnostics(); } + + [Fact, WorkItem(32446, "https://github.com/dotnet/roslyn/issues/32446")] + public void RefValueOfNullableType() + { + var source = +@" +using System; +public class C +{ + public void M(TypedReference r) + { + _ = __refvalue(r, string?).Length; // 1 + _ = __refvalue(r, string).Length; + } +} +"; + var comp = CreateCompilation(new[] { source }, options: WithNonNullTypesTrue()); + comp.VerifyDiagnostics( + // (7,13): warning CS8602: Possible dereference of a null reference. + // _ = __refvalue(r, string?).Length; // 1 + Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "__refvalue(r, string?)").WithLocation(7, 13) + ); + } } } -- GitLab