Skip to content

Address param-nullchecking public API design review #58987

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Jan 25, 2022
Merged

Address param-nullchecking public API design review #58987

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c7c0825
Address param-nullchecking public API design review
RikkiGibson Jan 20, 2022
701cb1d
Remove CodeGenerationParameterSymbol.IsNullChecked
RikkiGibson Jan 21, 2022
8effb3a
Update src/Workspaces/SharedUtilitiesAndExtensions/Compiler/VisualBas…
RikkiGibson Jan 21, 2022
969a517
Address feedback
RikkiGibson Jan 22, 2022
d97ae93
Merge branch 'main' of github.com:dotnet/roslyn into pnc-api
RikkiGibson Jan 25, 2022
46e369f
Remove IsNullChecked from shipped public API
RikkiGibson Jan 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions src/Compilers/CSharp/Portable/Symbols/PublicModel/ParameterSymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,6 @@ IParameterSymbol IParameterSymbol.OriginalDefinition

bool IParameterSymbol.IsParams => _underlying.IsParams;

bool IParameterSymbol.IsNullChecked => _underlying.IsNullChecked;

bool IParameterSymbol.IsOptional => _underlying.IsOptional;

bool IParameterSymbol.IsThis => _underlying.IsThis;
Expand Down
2,602 changes: 2 additions & 2,600 deletions src/Compilers/CSharp/Test/IOperation/IOperation/IOperationTests_NullCheckedParameters.cs
View file
Open in desktop

Large diffs are not rendered by default.

80 changes: 0 additions & 80 deletions src/Compilers/CSharp/Test/Semantic/Semantics/NullCheckedParameterTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -399,16 +399,6 @@ public void M()
var tree = Parse(source, options: TestOptions.RegularPreview);
var comp = CreateCompilation(tree);
comp.VerifyDiagnostics();

var model = (CSharpSemanticModel)comp.GetSemanticModel(tree);
SimpleLambdaExpressionSyntax node = comp.GlobalNamespace.GetTypeMember("C")
.GetMember<SourceMethodSymbol>("M")
.GetNonNullSyntaxNode()
.DescendantNodes()
.OfType<SimpleLambdaExpressionSyntax>()
.Single();
var methodSymbol = (IMethodSymbol)model.GetSymbolInfo(node).Symbol!;
Assert.True(methodSymbol.Parameters[0].IsNullChecked);
}

[Fact]
Expand All @@ -426,17 +416,6 @@ public void M()
var tree = Parse(source, options: TestOptions.RegularPreview);
var comp = CreateCompilation(tree);
comp.VerifyDiagnostics();

var model = (CSharpSemanticModel)comp.GetSemanticModel(tree);
ParenthesizedLambdaExpressionSyntax node = comp.GlobalNamespace.GetTypeMember("C")
.GetMember<SourceMethodSymbol>("M")
.GetNonNullSyntaxNode()
.DescendantNodes()
.OfType<ParenthesizedLambdaExpressionSyntax>()
.Single();
var methodSymbol = (IMethodSymbol)model.GetSymbolInfo(node).Symbol!;
Assert.True(methodSymbol.Parameters[0].IsNullChecked);
Assert.False(methodSymbol.Parameters[1].IsNullChecked);
}

[Fact]
Expand All @@ -454,16 +433,6 @@ public void M()
var tree = Parse(source, options: TestOptions.RegularPreview);
var comp = CreateCompilation(tree);
comp.VerifyDiagnostics();

CSharpSemanticModel model = (CSharpSemanticModel)comp.GetSemanticModel(tree);
Syntax.ParenthesizedLambdaExpressionSyntax node = comp.GlobalNamespace.GetTypeMember("C")
.GetMember<SourceMethodSymbol>("M")
.GetNonNullSyntaxNode()
.DescendantNodes()
.OfType<Syntax.ParenthesizedLambdaExpressionSyntax>()
.Single();
var methodSymbol = (IMethodSymbol)model.GetSymbolInfo(node).Symbol!;
Assert.True(methodSymbol.Parameters[0].IsNullChecked);
}

[Fact]
Expand Down Expand Up @@ -571,16 +540,6 @@ public void M()
var tree = Parse(source, options: TestOptions.RegularPreview);
var comp = CreateCompilation(tree);
comp.VerifyDiagnostics();

CSharpSemanticModel model = (CSharpSemanticModel)comp.GetSemanticModel(tree);
ParenthesizedLambdaExpressionSyntax node = comp.GlobalNamespace.GetTypeMember("C")
.GetMember<SourceMethodSymbol>("M")
.GetNonNullSyntaxNode()
.DescendantNodes()
.OfType<ParenthesizedLambdaExpressionSyntax>()
.Single();
var methodSymbol = (IMethodSymbol)model.GetSymbolInfo(node).Symbol!;
Assert.True(methodSymbol.Parameters[0].IsNullChecked);
}

[Fact]
Expand All @@ -598,17 +557,6 @@ public void M()
var tree = Parse(source, options: TestOptions.RegularPreview);
var comp = CreateCompilation(tree);
comp.VerifyDiagnostics();

CSharpSemanticModel model = (CSharpSemanticModel)comp.GetSemanticModel(tree);
ParenthesizedLambdaExpressionSyntax node = comp.GlobalNamespace.GetTypeMember("C")
.GetMember<SourceMethodSymbol>("M")
.GetNonNullSyntaxNode()
.DescendantNodes()
.OfType<ParenthesizedLambdaExpressionSyntax>()
.Single();
var methodSymbol = (IMethodSymbol)model.GetSymbolInfo(node).Symbol!;
Assert.True(methodSymbol.Parameters[0].IsNullChecked);
Assert.False(methodSymbol.Parameters[1].IsNullChecked);
}

[Fact]
Expand All @@ -626,17 +574,6 @@ public void M()
var tree = Parse(source, options: TestOptions.RegularPreview);
var comp = CreateCompilation(tree);
comp.VerifyDiagnostics();

CSharpSemanticModel model = (CSharpSemanticModel)comp.GetSemanticModel(tree);
ParenthesizedLambdaExpressionSyntax node = comp.GlobalNamespace.GetTypeMember("C")
.GetMember<SourceMethodSymbol>("M")
.GetNonNullSyntaxNode()
.DescendantNodes()
.OfType<ParenthesizedLambdaExpressionSyntax>()
.Single();
var methodSymbol = (IMethodSymbol)model.GetSymbolInfo(node).Symbol!;
Assert.True(methodSymbol.Parameters[0].IsNullChecked);
Assert.True(methodSymbol.Parameters[1].IsNullChecked);
}

[Fact]
Expand All @@ -655,23 +592,6 @@ public void M()
var tree = Parse(source, options: TestOptions.RegularPreview);
var comp = CreateCompilation(tree);
comp.VerifyDiagnostics();

CSharpSemanticModel model = (CSharpSemanticModel)comp.GetSemanticModel(tree);
ParenthesizedLambdaExpressionSyntax[] nodes = comp.GlobalNamespace.GetTypeMember("C")
.GetMember<SourceMethodSymbol>("M")
.GetNonNullSyntaxNode()
.DescendantNodes()
.OfType<ParenthesizedLambdaExpressionSyntax>()
.ToArray();

Assert.Equal(2, nodes.Length);

var methodSymbol = (IMethodSymbol)model.GetSymbolInfo(nodes[0]).Symbol!;
Assert.True(methodSymbol.Parameters[0].IsNullChecked);

methodSymbol = (IMethodSymbol)model.GetSymbolInfo(nodes[1]).Symbol!;
Assert.True(methodSymbol.Parameters[0].IsNullChecked);
Assert.False(methodSymbol.Parameters[1].IsNullChecked);
}

[Fact]
Expand Down
154 changes: 0 additions & 154 deletions src/Compilers/Core/Portable/Operations/ControlFlowGraphBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,6 @@ public static ControlFlowGraph Create(IOperation body, ControlFlowGraph? parent
case OperationKind.AnonymousFunction:
Debug.Assert(captureIdDispenser != null);
var anonymousFunction = (IAnonymousFunctionOperation)body;
builder.VisitNullChecks(anonymousFunction, anonymousFunction.Symbol.Parameters);
builder.VisitStatement(anonymousFunction.Body);
break;
default:
Expand Down Expand Up @@ -1481,12 +1480,6 @@ bool visitPossibleUsingDeclarationInLabel(ILabeledOperation labelOperation)

EnterRegion(new RegionBuilder(ControlFlowRegionKind.LocalLifetime, locals: operation.Locals));

// https://github.com/dotnet/roslyn/issues/58335: this implementation doesn't handle record primary constructors
if (operation.SemanticModel!.GetDeclaredSymbol(operation.Syntax) is IMethodSymbol method)
{
VisitNullChecks(operation, method.Parameters);
}

if (operation.Initializer != null)
{
VisitStatement(operation.Initializer);
Expand All @@ -1501,157 +1494,11 @@ bool visitPossibleUsingDeclarationInLabel(ILabeledOperation labelOperation)
public override IOperation? VisitMethodBodyOperation(IMethodBodyOperation operation, int? captureIdForResult)
{
StartVisitingStatement(operation);

// https://github.com/dotnet/roslyn/issues/58335: do we need to use SemanticModel here?
var member = operation.SemanticModel!.GetDeclaredSymbol(operation.Syntax);
Debug.Assert(captureIdForResult is null);
VisitNullChecks(operation, ((IMethodSymbol)member!).Parameters);

VisitMethodBodyBaseOperation(operation);
return FinishVisitingStatement(operation);
}

private void VisitNullChecks(IOperation operation, ImmutableArray<IParameterSymbol> parameters)
{
var temp = _currentStatement;
foreach (var param in parameters)
{
if (param.IsNullChecked)
{
// https://github.com/dotnet/roslyn/issues/58335: do we need to use SemanticModel here?
var check = GenerateNullCheckForParameter(param, operation.Syntax, ((Operation)operation).OwningSemanticModel!);
_currentStatement = check;
VisitConditional(check, captureIdForResult: null);
}
}
_currentStatement = temp;
}

private ConditionalOperation GenerateNullCheckForParameter(IParameterSymbol parameter, SyntaxNode syntax, SemanticModel semanticModel)
{
Debug.Assert(parameter.Language == LanguageNames.CSharp);
var paramReference = new ParameterReferenceOperation(parameter, semanticModel, syntax, parameter.Type, isImplicit: true);
var boolType = _compilation.GetSpecialType(SpecialType.System_Boolean);

IOperation conditionOp;
if (ITypeSymbolHelpers.IsNullableType(parameter.Type))
{
// https://github.com/dotnet/roslyn/issues/58335: is there a better way to get the HasValue symbol here?
// This way doesn't work with compilation.MakeMemberMissing for testing
var nullableHasValueProperty = parameter.Type.GetMembers(nameof(Nullable<int>.HasValue)).FirstOrDefault() as IPropertySymbol;
var nullableHasValueGet = nullableHasValueProperty?.GetMethod;
if (nullableHasValueGet is null)
{
conditionOp = new UnaryOperation(
UnaryOperatorKind.Not,
new InvalidOperation(
ImmutableArray.Create<IOperation>(paramReference),
semanticModel,
syntax,
boolType,
constantValue: null,
isImplicit: true),
isLifted: false,
isChecked: false,
operatorMethod: null,
semanticModel,
syntax,
boolType,
constantValue: null,
isImplicit: true);
}
else
{
conditionOp = new UnaryOperation(
UnaryOperatorKind.Not,
new InvocationOperation(
targetMethod: nullableHasValueGet,
instance: paramReference,
isVirtual: false,
arguments: ImmutableArray<IArgumentOperation>.Empty,
semanticModel,
syntax,
boolType,
isImplicit: true),
isLifted: false,
isChecked: false,
operatorMethod: null,
semanticModel,
syntax,
boolType,
constantValue: null,
isImplicit: true);
}
}
else
{
conditionOp = new BinaryOperation(
BinaryOperatorKind.Equals,
paramReference,
new LiteralOperation(semanticModel, syntax, parameter.Type, ConstantValue.Null, isImplicit: true),
isLifted: false,
isChecked: false,
isCompareText: false,
operatorMethod: null,
unaryOperatorMethod: null,
semanticModel,
syntax,
boolType,
constantValue: null,
isImplicit: true);
}

var paramNameLiteral = new LiteralOperation(semanticModel, syntax, _compilation.GetSpecialType(SpecialType.System_String), ConstantValue.Create(parameter.Name), isImplicit: true);
var argumentNullExceptionMethod = (IMethodSymbol?)_compilation.CommonGetWellKnownTypeMember(WellKnownMember.System_ArgumentNullException__ctorString)?.GetISymbol();
var argumentNullExceptionType = argumentNullExceptionMethod?.ContainingType;

// Occurs when a member is missing.
IOperation argumentNullExceptionObject;
if (argumentNullExceptionMethod is null)
{
argumentNullExceptionObject = new InvalidOperation(
children: ImmutableArray.Create((IOperation)paramNameLiteral),
semanticModel,
syntax,
argumentNullExceptionType,
constantValue: null,
isImplicit: true);
}
else
{
argumentNullExceptionObject = new ObjectCreationOperation(
argumentNullExceptionMethod,
initializer: null,
ImmutableArray.Create<IArgumentOperation>(
new ArgumentOperation(
ArgumentKind.Explicit,
parameter: argumentNullExceptionMethod.Parameters[0],
value: paramNameLiteral,
OperationFactory.IdentityConversion,
OperationFactory.IdentityConversion,
semanticModel,
syntax,
isImplicit: true)),
semanticModel,
syntax,
argumentNullExceptionType,
constantValue: null,
isImplicit: true);
}

IOperation whenTrue = new ExpressionStatementOperation(
new ThrowOperation(
argumentNullExceptionObject,
semanticModel,
syntax,
argumentNullExceptionType,
isImplicit: true),
semanticModel,
syntax,
isImplicit: true);
return new ConditionalOperation(conditionOp, whenTrue, whenFalse: null, isRef: false, semanticModel, syntax, boolType, constantValue: null, isImplicit: true);
}

private void VisitMethodBodyBaseOperation(IMethodBodyBaseOperation operation)
{
Debug.Assert(_currentStatement == operation);
Expand Down Expand Up @@ -6305,7 +6152,6 @@ IOperation visitAndCaptureInitializer(IPropertySymbol initializedProperty, IOper
private IOperation? VisitLocalFunctionAsRoot(ILocalFunctionOperation operation)
{
Debug.Assert(_currentStatement == null);
VisitNullChecks(operation, operation.Symbol.Parameters);
VisitMethodBodies(operation.Body, operation.IgnoredBody);
return null;
}
Expand Down
1 change: 0 additions & 1 deletion src/Compilers/Core/Portable/PublicAPI.Shipped.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1380,7 +1380,6 @@ Microsoft.CodeAnalysis.IParameterSymbol.CustomModifiers.get -> System.Collection
Microsoft.CodeAnalysis.IParameterSymbol.ExplicitDefaultValue.get -> object?
Microsoft.CodeAnalysis.IParameterSymbol.HasExplicitDefaultValue.get -> bool
Microsoft.CodeAnalysis.IParameterSymbol.IsDiscard.get -> bool
Microsoft.CodeAnalysis.IParameterSymbol.IsNullChecked.get -> bool
Microsoft.CodeAnalysis.IParameterSymbol.IsOptional.get -> bool
Microsoft.CodeAnalysis.IParameterSymbol.IsParams.get -> bool
Microsoft.CodeAnalysis.IParameterSymbol.IsThis.get -> bool
Expand Down
5 changes: 0 additions & 5 deletions src/Compilers/Core/Portable/Symbols/IParameterSymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,10 +91,5 @@ public interface IParameterSymbol : ISymbol
/// source or metadata.
/// </summary>
new IParameterSymbol OriginalDefinition { get; }

/// <summary>
/// True if the compiler will synthesize a null check for this parameter (the parameter is declared in source with a <c>!!</c> following the parameter name).
/// </summary>
bool IsNullChecked { get; }
}
}
6 changes: 0 additions & 6 deletions src/Compilers/VisualBasic/Portable/Symbols/ParameterSymbol.vb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -359,12 +359,6 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Symbols
End Get
End Property

Private ReadOnly Property IsNullChecked As Boolean Implements IParameterSymbol.IsNullChecked
Get
Return False
End Get
End Property

Public Overrides Sub Accept(visitor As SymbolVisitor)
visitor.VisitParameter(Me)
End Sub
Expand Down
4 changes: 2 additions & 2 deletions ...res/Core/Portable/InitializeParameter/AbstractAddParameterCheckCodeRefactoringProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,12 +264,12 @@ protected bool ParameterValidForNullCheck(Document document, IParameterSymbol pa
return false;
}

if (parameter.IsNullChecked)
if (parameter.IsDiscard)
{
return false;
}

if (parameter.IsDiscard)
if (document.GetRequiredLanguageService<ISemanticFactsService>().IsNullChecked(parameter, cancellationToken))
{
return false;
}
Expand Down
1 change: 0 additions & 1 deletion src/Workspaces/Core/Portable/CodeGeneration/Symbols/CodeGenerationParameterSymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,6 @@ public override TResult Accept<TResult>(SymbolVisitor<TResult> visitor)

public ImmutableArray<CustomModifier> CustomModifiers => ImmutableArray.Create<CustomModifier>();

public bool IsNullChecked => false;
public bool IsDiscard => false;
}
}
1 change: 1 addition & 0 deletions src/Workspaces/Core/Portable/Shared/Extensions/IParameterSymbolExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

using System.Collections.Generic;
using System.Collections.Immutable;
using System.Linq;
using Microsoft.CodeAnalysis.CodeGeneration;
using Microsoft.CodeAnalysis.PooledObjects;

Expand Down
Loading