Skip to content

Commit

Permalink
Relax parameter-argument modifier matching for ref readonly parameters (
Browse files Browse the repository at this point in the history 
#68224)

* Relax parameter-argument modifier matching

* Remove sequence point verification

* Adjust argument emit

* Test more value kinds and argument modifiers

* Simplify ref kind mismatch logic

* Skip arglist arguments explicitly

* Merge argument checking and coercion

* Check feature availability before emitting mismatch warnings

* Verify success interpolated string cases

* Fix wrong error when overload resolution fails

* Revert assert in argument ref kind emit

* Remove a prototype comment about method conversions

* Use warning level 1

* Revert argument expression passing to `GetEffectiveParameterRefKind`

* Adjust wording of `WRN_BadArgRef`

* Update patching comments in local rewriter

* Lower `ref readonly` arguments to `in` ref kinds

* Make a note to improve value kind error wording

* Reduce ref kind assert before emit

* Assert `ref readonly` params have ref kind before emit

* Assert synthesized argument ref kinds

* Fix `ref`/`in` pair error reporting logic

* Store `ref readonly` temps same as `in`

* Disallow `ref readonly` with none/`in` for old LangVersion

* Fix `ref readonly` parameter value kind

* Improve wording for rvalues passed to `ref readonly`

* Add more value check tests

* Improve assert code in synthesized constructor

* Fix ref kinds default or empty assert

* Suggest upgrading to `C#12` for `ref`/`in` error

* Add separate error for readonly refs

* Transform method ref kinds in synthesized calls

* Skip arglist also in older LangVersion

* Use non-strict ref kinds in synthesized calls

* Ignore missing methods in an assert

* Remove unnecessary `using`

* Suggest upgrading on `ref`/`in` mismatch

* Move local `argRefKind` closer to its use-site

* Pass strict in ref kind to `StoreToTemp`

* Improve nested ternary operator readability

* Consider strict `in` during codegen

* Make change consistent with `main`

* Suppress `ref`/`in` warning for interpolated string handlers

* Verify cross assembly scenarios in latest LangVersion

* Move and extend interpolated string constructor diagnostic filtering

* Handle one more interpolated string diagnostic collection
  • Loading branch information
@jjonescz
jjonescz authored Jul 10, 2023
1 parent 644742d commit ba7606d
Show file tree
Hide file tree
Showing 40 changed files with 2,777 additions and 84 deletions.
2 changes: 2 additions & 0 deletions src/Analyzers/CSharp/CodeFixes/UpgradeProject/CSharpUpgradeProjectCodeFixProvider.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,8 @@ public CSharpUpgradeProjectCodeFixProvider()
"CS8885", // warning CS8885: The 'this' object cannot be used before all of its fields have been assigned. Consider updating to language version 'preview' to auto-default the unassigned fields.
"CS8936", // error CS8936: Feature '{0}' is not available in C# 10.0. Please use language version {1} or greater.
"CS9058", // error CS9058: Feature '{0}' is not available in C# 11.0. Please use language version {1} or greater.
// PROTOTYPE: Fixup error numbers.
"CS9505", // error CS9505: Argument {0} may not be passed with the 'ref' keyword in language version {1}. To pass 'ref' arguments to 'in' parameters, upgrade to language version {2} or greater.
});

public override string UpgradeThisProjectResource => CSharpCodeFixesResources.Upgrade_this_project_to_csharp_language_version_0;
Expand Down
17 changes: 17 additions & 0 deletions src/Analyzers/CSharp/Tests/UpgradeProject/UpgradeProjectTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1267,5 +1267,22 @@ struct Test
expected: LanguageVersion.CSharp11,
new CSharpParseOptions(LanguageVersion.CSharp10));
}

[Fact]
public async Task UpgradeProjectForRefInMismatch()
{
await TestLanguageVersionUpgradedAsync("""
class C
{
void M1(in int x) { }
void M2(ref int y)
{
M1(ref [|y|]);
}
}
""",
expected: LanguageVersion.Preview,
new CSharpParseOptions(LanguageVersion.CSharp11));
}
}
}
4 changes: 2 additions & 2 deletions src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -919,8 +919,8 @@ private bool CheckParameterValueKind(SyntaxNode node, BoundParameter parameter,
ParameterSymbol parameterSymbol = parameter.ParameterSymbol;

// all parameters can be passed by ref/out or assigned to
// except "in" parameters, which are readonly
if (parameterSymbol.RefKind == RefKind.In && RequiresAssignableVariable(valueKind))
// except "in" and "ref readonly" parameters, which are readonly
if (parameterSymbol.RefKind is RefKind.In or RefKind.RefReadOnlyParameter && RequiresAssignableVariable(valueKind))
{
ReportReadOnlyError(parameterSymbol, node, valueKind, checkingReceiver, diagnostics);
return false;
Expand Down
69 changes: 65 additions & 4 deletions src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3213,14 +3213,15 @@ private BoundExpression BindArgumentExpression(BindingDiagnosticBag diagnostics,
}

#nullable enable
private void CoerceArguments<TMember>(
private void CheckAndCoerceArguments<TMember>(
MemberResolutionResult<TMember> methodResult,
ArrayBuilder<BoundExpression> arguments,
AnalyzedArguments analyzedArguments,
BindingDiagnosticBag diagnostics,
BoundExpression? receiver)
where TMember : Symbol
{
var result = methodResult.Result;
var arguments = analyzedArguments.Arguments;

// Parameter types should be taken from the least overridden member:
var parameters = methodResult.LeastOverriddenMember.GetParameters();
Expand All @@ -3230,6 +3231,66 @@ private void CoerceArguments<TMember>(
var kind = result.ConversionForArg(arg);
BoundExpression argument = arguments[arg];

if (argument is not BoundArgListOperator && !argument.HasAnyErrors)
{
var argRefKind = analyzedArguments.RefKind(arg);

if (!Compilation.IsFeatureEnabled(MessageID.IDS_FeatureRefReadonlyParameters))
{
// Disallow using `ref readonly` parameters with no or `in` argument modifier,
// same as older versions of the compiler would (since they would see the parameter as `ref`).
if (argRefKind is RefKind.None or RefKind.In &&
GetCorrespondingParameter(ref result, parameters, arg).RefKind == RefKind.RefReadOnlyParameter)
{
var available = CheckFeatureAvailability(argument.Syntax, MessageID.IDS_FeatureRefReadonlyParameters, diagnostics);
Debug.Assert(!available);
}
}
else
{
// Warn for `ref`/`in` or None/`ref readonly` mismatch.
if (argRefKind == RefKind.Ref)
{
if (GetCorrespondingParameter(ref result, parameters, arg).RefKind == RefKind.In)
{
// The 'ref' modifier for argument {0} corresponding to 'in' parameter is equivalent to 'in'. Consider using 'in' instead.
diagnostics.Add(
ErrorCode.WRN_BadArgRef,
argument.Syntax,
arg + 1);
}
}
else if (argRefKind == RefKind.None &&
GetCorrespondingParameter(ref result, parameters, arg).RefKind == RefKind.RefReadOnlyParameter)
{
if (!this.CheckValueKind(argument.Syntax, argument, BindValueKind.RefersToLocation, checkingReceiver: false, BindingDiagnosticBag.Discarded))
{
// Argument {0} should be a variable because it is passed to a 'ref readonly' parameter
diagnostics.Add(
ErrorCode.WRN_RefReadonlyNotVariable,
argument.Syntax,
arg + 1);
}
else if (this.CheckValueKind(argument.Syntax, argument, BindValueKind.Assignable, checkingReceiver: false, BindingDiagnosticBag.Discarded))
{
// Argument {0} should be passed with 'ref' or 'in' keyword
diagnostics.Add(
ErrorCode.WRN_ArgExpectedRefOrIn,
argument.Syntax,
arg + 1);
}
else
{
// Argument {0} should be passed with the 'in' keyword
diagnostics.Add(
ErrorCode.WRN_ArgExpectedIn,
argument.Syntax,
arg + 1);
}
}
}
}

if (kind.IsInterpolatedStringHandler)
{
Debug.Assert(argument is BoundUnconvertedInterpolatedString or BoundBinaryOperator { IsUnconvertedInterpolatedStringAddition: true });
Expand Down Expand Up @@ -6137,7 +6198,7 @@ internal bool TryPerformConstructorOverloadResolution(

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

// Fill in the out parameter with the result, if there was one; it might be inaccessible.
Expand Down Expand Up @@ -8557,7 +8618,7 @@ private BoundExpression BindIndexerOrIndexedPropertyAccess(

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

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

if (!gotError && receiver != null && receiver.Kind == BoundKind.ThisReference && receiver.WasCompilerGenerated)
{
Expand Down
34 changes: 28 additions & 6 deletions src/Compilers/CSharp/Portable/Binder/Binder_InterpolatedString.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -601,7 +601,7 @@ private BoundBinaryOperator BindUnconvertedBinaryOperatorToInterpolatedStringHan
{
// We successfully bound the out version, so set all the final data based on that binding
constructorCall = nonOutConstructorCall;
diagnostics.AddRangeAndFree(nonOutConstructorDiagnostics);
addAndFreeConstructorDiagnostics(target: diagnostics, source: nonOutConstructorDiagnostics);
outConstructorDiagnostics.Free();
}
else
Expand All @@ -622,27 +622,27 @@ private BoundBinaryOperator BindUnconvertedBinaryOperatorToInterpolatedStringHan
case (true, false):
constructorCall = outConstructorCall;
additionalConstructorArguments = outConstructorAdditionalArguments;
diagnostics.AddRangeAndFree(outConstructorDiagnostics);
addAndFreeConstructorDiagnostics(target: diagnostics, source: outConstructorDiagnostics);
nonOutConstructorDiagnostics.Free();
break;
case (false, true):
constructorCall = nonOutConstructorCall;
diagnostics.AddRangeAndFree(nonOutConstructorDiagnostics);
addAndFreeConstructorDiagnostics(target: diagnostics, source: nonOutConstructorDiagnostics);
outConstructorDiagnostics.Free();
break;
default:
// For the final output binding info, we'll go with the shorter constructor in the absence of any tiebreaker,
// but we'll report all diagnostics
constructorCall = nonOutConstructorCall;
diagnostics.AddRangeAndFree(nonOutConstructorDiagnostics);
diagnostics.AddRangeAndFree(outConstructorDiagnostics);
addAndFreeConstructorDiagnostics(target: diagnostics, source: nonOutConstructorDiagnostics);
addAndFreeConstructorDiagnostics(target: diagnostics, source: outConstructorDiagnostics);
break;
}
}
}
else
{
diagnostics.AddRangeAndFree(outConstructorDiagnostics);
addAndFreeConstructorDiagnostics(target: diagnostics, source: outConstructorDiagnostics);
constructorCall = outConstructorCall;
additionalConstructorArguments = outConstructorAdditionalArguments;
}
Expand Down Expand Up @@ -677,6 +677,28 @@ static void populateArguments(SyntaxNode syntax, ImmutableArray<BoundInterpolate
// Any other arguments from the call site
argumentsBuilder.AddRange(additionalConstructorArguments);
}

static void addAndFreeConstructorDiagnostics(BindingDiagnosticBag target, BindingDiagnosticBag source)
{
target.AddDependencies(source);

if (source.DiagnosticBag is { IsEmptyWithoutResolution: false } bag)
{
foreach (var diagnostic in bag.AsEnumerableWithoutResolution())
{
// Filter diagnostics that cannot be fixed since they are on the hidden interpolated string constructor.
if (!((ErrorCode)diagnostic.Code is ErrorCode.WRN_BadArgRef
or ErrorCode.WRN_RefReadonlyNotVariable
or ErrorCode.WRN_ArgExpectedRefOrIn
or ErrorCode.WRN_ArgExpectedIn))
{
target.Add(diagnostic);
}
}
}

source.Free();
}
}

private ImmutableArray<BoundExpression> BindInterpolatedStringParts(BoundUnconvertedInterpolatedString unconvertedInterpolatedString, BindingDiagnosticBag diagnostics)
Expand Down
4 changes: 2 additions & 2 deletions src/Compilers/CSharp/Portable/Binder/Binder_Invocation.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1072,7 +1072,7 @@ private BoundCall BindInvocationExpressionContinued(

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

this.CoerceArguments(methodResult, analyzedArguments.Arguments, diagnostics, receiver);
this.CheckAndCoerceArguments(methodResult, analyzedArguments, diagnostics, receiver);

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

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

var args = analyzedArguments.Arguments.ToImmutable();
var refKinds = analyzedArguments.RefKinds.ToImmutableOrNull();
Expand Down
24 changes: 21 additions & 3 deletions src/Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/OverloadResolution.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2192,7 +2192,7 @@ private static BetterResult PreferValOverInOrRefInterpolatedHandlerParameters<TM
static bool isAcceptableRefMismatch(RefKind refKind, bool isInterpolatedStringHandlerConversion)
=> refKind switch
{
RefKind.In => true,
RefKind.In or RefKind.RefReadOnlyParameter => true,
RefKind.Ref when isInterpolatedStringHandlerConversion => true,
_ => false
};
Expand Down Expand Up @@ -3190,9 +3190,27 @@ private static RefKind GetEffectiveParameterRefKind(

// 'None' argument is allowed to match 'In' parameter and should behave like 'None' for the purpose of overload resolution
// unless this is a method group conversion where 'In' must match 'In'
if (!isMethodGroupConversion && argRefKind == RefKind.None && paramRefKind == RefKind.In)
// There are even more relaxations with 'ref readonly' parameters feature:
// - 'ref' argument is allowed to match 'in' parameter,
// - 'ref', 'in', none argument is allowed to match 'ref readonly' parameter.
if (!isMethodGroupConversion)
{
return RefKind.None;
if (paramRefKind == RefKind.In)
{
if (argRefKind == RefKind.None)
{
return RefKind.None;
}

if (argRefKind == RefKind.Ref && binder.Compilation.IsFeatureEnabled(MessageID.IDS_FeatureRefReadonlyParameters))
{
return RefKind.Ref;
}
}
else if (paramRefKind == RefKind.RefReadOnlyParameter && argRefKind is RefKind.None or RefKind.Ref or RefKind.In)
{
return argRefKind;
}
}

// Omit ref feature for COM interop: We can pass arguments by value for ref parameters if we are calling a method/property on an instance of a COM imported type.
Expand Down
20 changes: 17 additions & 3 deletions ...Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/OverloadResolutionResult.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1213,11 +1213,25 @@ private static void ReportBadArgumentError(
new FormattedSymbol(UnwrapIfParamsArray(parameter, isLastParameter), SymbolDisplayFormat.CSharpErrorMessageNoParameterNamesFormat));
}
}
else if (refArg != refParameter && !(refArg == RefKind.None && refParameter == RefKind.In))
else if (refArg != refParameter &&
!(refArg == RefKind.None && refParameter == RefKind.In) &&
!(refArg == RefKind.Ref && refParameter == RefKind.In && binder.Compilation.IsFeatureEnabled(MessageID.IDS_FeatureRefReadonlyParameters)) &&
!(refParameter == RefKind.RefReadOnlyParameter && refArg is RefKind.None or RefKind.Ref or RefKind.In))
{
if (refParameter == RefKind.None || refParameter == RefKind.In)
if (refArg == RefKind.Ref && refParameter == RefKind.In && !binder.Compilation.IsFeatureEnabled(MessageID.IDS_FeatureRefReadonlyParameters))
{
// Argument {0} should not be passed with the {1} keyword
// Argument {0} may not be passed with the 'ref' keyword in language version {1}. To pass 'ref' arguments to 'in' parameters, upgrade to language version {2} or greater.
diagnostics.Add(
ErrorCode.ERR_BadArgExtraRefLangVersion,
sourceLocation,
symbols,
arg + 1,
binder.Compilation.LanguageVersion.ToDisplayString(),
new CSharpRequiredLanguageVersion(MessageID.IDS_FeatureRefReadonlyParameters.RequiredVersion()));
}
else if (refParameter is RefKind.None or RefKind.In or RefKind.RefReadOnlyParameter)
{
// Argument {0} may not be passed with the '{1}' keyword
diagnostics.Add(
ErrorCode.ERR_BadArgExtraRef,
sourceLocation,
Expand Down
Loading

0 comments on commit ba7606d

Please sign in to comment.