-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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.
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
Relax parameter-argument modifier matching for ref readonly parameters #68224
Changes from all commits
732ccbb
ba4196c
95c47c8
6de3783
005decd
023d6a6
f14a901
157997f
10f9b05
6214282
ae921b5
30abc94
3a49016
7f9689b
a5af25b
d67b7fb
e6e46e1
42300a1
6dd4ba6
a0b0226
389d66e
7bf0117
cee5f6f
8326054
e779e56
31d5f80
ddae804
52b0a96
c05c5ce
81ebe2c
aed010e
44bcef2
1983e18
ed0c02d
c1c8da7
5888231
23a3c2a
e0b8403
a0be827
4e4f716
3708145
101906a
595b733
50ed0bc
ed895b4
ff4a182
f9f0f5c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Spec: Should the spec mention that we'll have a preference of by-value over |
||
RefKind.Ref when isInterpolatedStringHandlerConversion => true, | ||
_ => false | ||
}; | ||
|
@@ -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. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) && | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch. Will extend test |
||
!(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)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, |
||
{ | ||
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, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we check
IDS_FeatureRefReadonlyParameters
availability at the call site for any scenarios? #ClosedThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, thanks, I even have a test for it, but didn't notice it had wrong results.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you referring to
RefReadonlyParameter_CrossAssembly
unit-test? I actually thought that we would want to report the fact that the feature is not enabled at least when the corresponding parameter RefKind isref readonly
, the call site modifier isin
or omitted, and language version is below 12. Alternatively, we can complain on any attempt to consumeref readonly
parameter when language version is below 12.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Responded under the comment on that test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We had an offline discussion about this, and, I think, agreed that a follow-up is necessary.