Skip to content

Commit

Permalink
Add support for nullable analysis in interpolated string handler cons…
Browse files Browse the repository at this point in the history 
…tructors (#57780)

Fixes #54583. We do not flow attribute post conditions back to the original slot of passed arguments or receivers, as without a proper in-order visit of the method parameters ensuring that the correct nullabilities are observed at the correct times isn't possible.
  • Loading branch information
@333fred
333fred authored Dec 15, 2021
1 parent b246a00 commit 9cbcb29
Show file tree
Hide file tree
Showing 16 changed files with 1,444 additions and 158 deletions.
14 changes: 2 additions & 12 deletions src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2798,12 +2798,7 @@ internal static uint GetValEscape(BoundExpression expr, uint scopeOfTheContainin

if (conversion.ConversionKind == ConversionKind.InterpolatedStringHandler)
{
var data = conversion.Operand switch
{
BoundInterpolatedString { InterpolationData: { } d } => d,
BoundBinaryOperator { InterpolatedStringHandlerData: { } d } => d,
_ => throw ExceptionUtilities.UnexpectedValue(conversion.Operand.Kind)
};
var data = conversion.Operand.GetInterpolatedStringHandlerData();
return GetInterpolatedStringHandlerConversionEscapeScope(data, scopeOfTheContainingExpression);
}

Expand Down Expand Up @@ -3592,12 +3587,7 @@ private static bool CheckValEscape(ImmutableArray<BoundExpression> expressions,
private static bool CheckInterpolatedStringHandlerConversionEscape(BoundExpression expression, uint escapeFrom, uint escapeTo, BindingDiagnosticBag diagnostics)
{

var data = expression switch
{
BoundInterpolatedString { InterpolationData: { } d } => d,
BoundBinaryOperator { InterpolatedStringHandlerData: { } d } => d,
_ => throw ExceptionUtilities.UnexpectedValue(expression.Kind)
};
var data = expression.GetInterpolatedStringHandlerData();

// We need to check to see if any values could potentially escape outside the max depth via the handler type.
// Consider the case where a ref-struct handler saves off the result of one call to AppendFormatted,
Expand Down
22 changes: 6 additions & 16 deletions src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2993,8 +2993,7 @@ private void CoerceArguments<TMember>(
MemberResolutionResult<TMember> methodResult,
ArrayBuilder<BoundExpression> arguments,
BindingDiagnosticBag diagnostics,
TypeSymbol? receiverType,
uint receiverEscapeScope)
BoundExpression? receiver)
where TMember : Symbol
{
var result = methodResult.Result;
Expand All @@ -3012,7 +3011,7 @@ private void CoerceArguments<TMember>(
Debug.Assert(argument is BoundUnconvertedInterpolatedString or BoundBinaryOperator { IsUnconvertedInterpolatedStringAddition: true });
TypeWithAnnotations parameterTypeWithAnnotations = GetCorrespondingParameterTypeWithAnnotations(ref result, parameters, arg);
reportUnsafeIfNeeded(methodResult, diagnostics, argument, parameterTypeWithAnnotations);
arguments[arg] = BindInterpolatedStringHandlerInMemberCall(argument, arguments, parameters, ref result, arg, receiverType, receiverEscapeScope, diagnostics);
arguments[arg] = BindInterpolatedStringHandlerInMemberCall(argument, arguments, parameters, ref result, arg, receiver, methodResult.LeastOverriddenMember.RequiresInstanceReceiver(), diagnostics);
}
// https://github.com/dotnet/roslyn/issues/37119 : should we create an (Identity) conversion when the kind is Identity but the types differ?
else if (!kind.IsIdentity)
Expand Down Expand Up @@ -4809,13 +4808,7 @@ private BoundExpression BindObjectInitializerMember(
{
if (argument is BoundConversion { Conversion.IsInterpolatedStringHandler: true, Operand: var operand })
{
var handlerPlaceholders = operand switch
{
BoundBinaryOperator { InterpolatedStringHandlerData: { } data } => data.ArgumentPlaceholders,
BoundInterpolatedString { InterpolationData: { } data } => data.ArgumentPlaceholders,
_ => throw ExceptionUtilities.UnexpectedValue(operand.Kind)
};

var handlerPlaceholders = operand.GetInterpolatedStringHandlerData().ArgumentPlaceholders;
if (handlerPlaceholders.Any(placeholder => placeholder.ArgumentIndex == BoundInterpolatedStringArgumentPlaceholder.InstanceParameter))
{
diagnostics.Add(ErrorCode.ERR_InterpolatedStringsReferencingInstanceCannotBeInObjectInitializers, argument.Syntax.Location);
Expand Down Expand Up @@ -5745,7 +5738,7 @@ internal bool TryPerformConstructorOverloadResolution(

if (succeededIgnoringAccessibility)
{
this.CoerceArguments<MethodSymbol>(result.ValidResult, analyzedArguments.Arguments, diagnostics, receiverType: null, receiverEscapeScope: Binder.ExternalScope);
this.CoerceArguments<MethodSymbol>(result.ValidResult, analyzedArguments.Arguments, diagnostics, receiver: null);
}

// Fill in the out parameter with the result, if there was one; it might be inaccessible.
Expand Down Expand Up @@ -8012,11 +8005,6 @@ private BoundExpression BindIndexerOrIndexedPropertyAccess(
{
MemberResolutionResult<PropertySymbol> resolutionResult = overloadResolutionResult.ValidResult;
PropertySymbol property = resolutionResult.Member;
RefKind? receiverRefKind = receiver.GetRefKind();
uint receiverEscapeScope = property.RequiresInstanceReceiver && receiver != null
? receiverRefKind?.IsWritableReference() == true ? GetRefEscape(receiver, LocalScopeDepth) : GetValEscape(receiver, LocalScopeDepth)
: Binder.ExternalScope;
this.CoerceArguments<PropertySymbol>(resolutionResult, analyzedArguments.Arguments, diagnostics, receiver.Type, receiverEscapeScope);

var isExpanded = resolutionResult.Result.Kind == MemberResolutionKind.ApplicableInExpandedForm;
var argsToParams = resolutionResult.Result.ArgsToParamsOpt;
Expand All @@ -8028,6 +8016,8 @@ private BoundExpression BindIndexerOrIndexedPropertyAccess(

receiver = ReplaceTypeOrValueReceiver(receiver, property.IsStatic, diagnostics);

this.CoerceArguments<PropertySymbol>(resolutionResult, analyzedArguments.Arguments, diagnostics, receiver);

if (!gotError && receiver != null && receiver.Kind == BoundKind.ThisReference && receiver.WasCompilerGenerated)
{
gotError = IsRefOrOutThisParameterCaptured(syntax, diagnostics);
Expand Down
23 changes: 15 additions & 8 deletions src/Compilers/CSharp/Portable/Binder/Binder_InterpolatedString.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -808,8 +808,8 @@ private BoundExpression BindInterpolatedStringHandlerInMemberCall(
ImmutableArray<ParameterSymbol> parameters,
ref MemberAnalysisResult memberAnalysisResult,
int interpolatedStringArgNum,
TypeSymbol? receiverType,
uint receiverEscapeScope,
BoundExpression? receiver,
bool requiresInstanceReceiver,
BindingDiagnosticBag diagnostics)
{
Debug.Assert(unconvertedString is BoundUnconvertedInterpolatedString or BoundBinaryOperator { IsUnconvertedInterpolatedStringAddition: true });
Expand Down Expand Up @@ -916,9 +916,9 @@ private BoundExpression BindInterpolatedStringHandlerInMemberCall(
switch (argumentIndex)
{
case BoundInterpolatedStringArgumentPlaceholder.InstanceParameter:
Debug.Assert(receiverType is not null);
Debug.Assert(receiver!.Type is not null);
refKind = RefKind.None;
placeholderType = receiverType;
placeholderType = receiver.Type;
break;
case BoundInterpolatedStringArgumentPlaceholder.UnspecifiedParameter:
{
Expand Down Expand Up @@ -964,33 +964,40 @@ private BoundExpression BindInterpolatedStringHandlerInMemberCall(

SyntaxNode placeholderSyntax;
uint valSafeToEscapeScope;
bool isSuppressed;

switch (argumentIndex)
{
case BoundInterpolatedStringArgumentPlaceholder.InstanceParameter:
placeholderSyntax = unconvertedString.Syntax;
valSafeToEscapeScope = receiverEscapeScope;
Debug.Assert(receiver != null);
valSafeToEscapeScope = requiresInstanceReceiver
? receiver.GetRefKind().IsWritableReference() == true ? GetRefEscape(receiver, LocalScopeDepth) : GetValEscape(receiver, LocalScopeDepth)
: Binder.ExternalScope;
isSuppressed = receiver.IsSuppressed;
placeholderSyntax = receiver.Syntax;
break;
case BoundInterpolatedStringArgumentPlaceholder.UnspecifiedParameter:
placeholderSyntax = unconvertedString.Syntax;
valSafeToEscapeScope = Binder.ExternalScope;
isSuppressed = false;
break;
case >= 0:
placeholderSyntax = arguments[argumentIndex].Syntax;
valSafeToEscapeScope = GetValEscape(arguments[argumentIndex], LocalScopeDepth);
isSuppressed = arguments[argumentIndex].IsSuppressed;
break;
default:
throw ExceptionUtilities.UnexpectedValue(argumentIndex);
}

argumentPlaceholdersBuilder.Add(
new BoundInterpolatedStringArgumentPlaceholder(
(BoundInterpolatedStringArgumentPlaceholder)(new BoundInterpolatedStringArgumentPlaceholder(
placeholderSyntax,
argumentIndex,
valSafeToEscapeScope,
placeholderType,
hasErrors: argumentIndex == BoundInterpolatedStringArgumentPlaceholder.UnspecifiedParameter)
{ WasCompilerGenerated = true });
{ WasCompilerGenerated = true }.WithSuppression(isSuppressed)));
// We use the parameter refkind, rather than what the argument was actually passed with, because that will suppress duplicated errors
// about arguments being passed with the wrong RefKind. The user will have already gotten an error about mismatched RefKinds or it will
// be a place where refkinds are allowed to differ
Expand Down
13 changes: 2 additions & 11 deletions src/Compilers/CSharp/Portable/Binder/Binder_Invocation.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1022,11 +1022,7 @@ private BoundCall BindInvocationExpressionContinued(

var receiver = ReplaceTypeOrValueReceiver(methodGroup.Receiver, !method.RequiresInstanceReceiver && !invokedAsExtensionMethod, diagnostics);

var receiverRefKind = receiver?.GetRefKind();
uint receiverValEscapeScope = method.RequiresInstanceReceiver && receiver != null
? receiverRefKind?.IsWritableReference() == true ? GetRefEscape(receiver, LocalScopeDepth) : GetValEscape(receiver, LocalScopeDepth)
: Binder.ExternalScope;
this.CoerceArguments(methodResult, analyzedArguments.Arguments, diagnostics, receiver?.Type, receiverValEscapeScope);
this.CoerceArguments(methodResult, analyzedArguments.Arguments, diagnostics, receiver);

var expanded = methodResult.Result.Kind == MemberResolutionKind.ApplicableInExpandedForm;
var argsToParams = methodResult.Result.ArgsToParamsOpt;
Expand Down Expand Up @@ -2029,12 +2025,7 @@ private BoundFunctionPointerInvocation BindFunctionPointerInvocation(SyntaxNode
methodsBuilder.Free();

MemberResolutionResult<FunctionPointerMethodSymbol> methodResult = overloadResolutionResult.ValidResult;
CoerceArguments(
methodResult,
analyzedArguments.Arguments,
diagnostics,
receiverType: null,
receiverEscapeScope: Binder.ExternalScope);
CoerceArguments(methodResult, analyzedArguments.Arguments, diagnostics, receiver: null);

var args = analyzedArguments.Arguments.ToImmutable();
var refKinds = analyzedArguments.RefKinds.ToImmutableOrNull();
Expand Down
10 changes: 10 additions & 0 deletions src/Compilers/CSharp/Portable/BoundTree/BoundNodeExtensions.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -245,5 +245,15 @@ static void pushLeftNodes(BoundBinaryOperator binary, ArrayBuilder<BoundBinaryOp
}
}
}

public static InterpolatedStringHandlerData GetInterpolatedStringHandlerData(this BoundExpression e, bool throwOnMissing = true)
=> e switch
{
BoundBinaryOperator { InterpolatedStringHandlerData: { } d } => d,
BoundInterpolatedString { InterpolationData: { } d } => d,
BoundBinaryOperator or BoundInterpolatedString when !throwOnMissing => default,
BoundBinaryOperator or BoundInterpolatedString => throw ExceptionUtilities.Unreachable,
_ => throw ExceptionUtilities.UnexpectedValue(e.Kind),
};
}
}
2 changes: 2 additions & 0 deletions src/Compilers/CSharp/Portable/BoundTree/InterpolatedStringHandlerData.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ internal readonly struct InterpolatedStringHandlerData

public readonly BoundInterpolatedStringHandlerPlaceholder ReceiverPlaceholder;

public bool IsDefault => Construction is null;

public InterpolatedStringHandlerData(
TypeSymbol builderType,
BoundExpression construction,
Expand Down
9 changes: 7 additions & 2 deletions src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1114,7 +1114,7 @@ public override BoundNode VisitDynamicInvocation(BoundDynamicInvocation node)
{ } d => (d.Construction, d.UsesBoolReturns, d.HasTrailingHandlerValidityParameter)
};

VisitRvalue(construction);
VisitInterpolatedStringHandlerConstructor(construction);
bool hasConditionalEvaluation = useBoolReturns || firstPartIsConditional;
TLocalState? shortCircuitState = hasConditionalEvaluation ? State.Clone() : default;

Expand All @@ -1128,6 +1128,11 @@ public override BoundNode VisitDynamicInvocation(BoundDynamicInvocation node)

return null;
}

protected virtual void VisitInterpolatedStringHandlerConstructor(BoundExpression? constructor)
{
VisitRvalue(constructor);
}
#nullable disable

public override BoundNode VisitInterpolatedString(BoundInterpolatedString node)
Expand Down Expand Up @@ -2459,7 +2464,7 @@ protected void VisitBinaryInterpolatedStringAddition(BoundBinaryOperator node)

Debug.Assert(parts.Count >= 2);

VisitRvalue(data.Construction);
VisitInterpolatedStringHandlerConstructor(data.Construction);

bool visitedFirst = false;
bool hasTrailingHandlerValidityParameter = data.HasTrailingHandlerValidityParameter;
Expand Down
25 changes: 10 additions & 15 deletions src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.DebugVerifier.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -169,11 +169,6 @@ private void VerifyExpression(BoundExpression expression, bool overrideSkippedEx

public override BoundNode? VisitBinaryOperator(BoundBinaryOperator node)
{
if (node.InterpolatedStringHandlerData is { } data)
{
Visit(data.Construction);
}

VisitBinaryOperatorChildren(node);
return null;
}
Expand Down Expand Up @@ -253,23 +248,23 @@ private void VisitBinaryOperatorChildren(BoundBinaryOperatorBase node)
return null;
}

public override BoundNode? VisitInterpolatedString(BoundInterpolatedString node)
{
if (node.InterpolationData is { Construction: var construction })
{
Visit(construction);
}
base.VisitInterpolatedString(node);
return null;
}

public override BoundNode? VisitImplicitIndexerAccess(BoundImplicitIndexerAccess node)
{
Visit(node.Receiver);
Visit(node.Argument);
Visit(node.IndexerOrSliceAccess);
return null;
}

public override BoundNode? VisitConversion(BoundConversion node)
{
if (node.ConversionKind == ConversionKind.InterpolatedStringHandler)
{
Visit(node.Operand.GetInterpolatedStringHandlerData().Construction);
}

return base.VisitConversion(node);
}
}
#endif
}
Expand Down
Loading

0 comments on commit 9cbcb29

Please sign in to comment.