-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Move IDS_FeatureRefLocalsReturns out of the parser #65710
Changes from 129 commits
cf0ff82
2e14765
c77a8df
9214e58
57f76d1
95190e5
b9d15d0
6ebd08e
5465712
8b23be3
3f573a6
d6fcf67
7ddd2b3
8efce60
13db8fc
e643c1c
7470991
541e46c
f47b8bf
ce53f95
e952cca
f463c06
dce4b3f
fb50dd4
8914d2e
de1d7c8
f5a8263
94702e9
c538085
43bcf61
550828e
0aa23d6
499c7e4
f7bfbfc
33185d6
01b8d6f
ece1590
66350f1
8b7ae44
ac9f64b
0dd7d23
158eea0
ff4d75b
5d316ad
4badea9
22de551
b378a29
eb6a35b
ad7c696
d2a504a
7d471b1
0654a20
7279de9
0648826
9705697
662f71a
d18cdbf
49b6f6f
03a2623
eaf7e8d
3e5826b
a9f8451
2fabba1
0263fee
0b663a1
29ae829
f1592dc
e7a983f
9442adc
4ebf39b
ce9db2d
7900e3a
a27cb92
4670c78
636d6fd
e4d9d3d
7437a9e
dd1a55d
933db45
c0a1ad7
8b66b65
5470687
f14cf24
18dabb5
b910b94
a666886
a23e082
c56a716
e008fc3
c4a42bc
966a269
1877677
a671a90
165f266
eafb6d0
5e7505d
71ef8d2
74d11be
d84dd99
376773e
fedc280
40f1612
641fe28
c7e8640
ef2d6e7
aa09a1b
2362fa8
e179d0d
3813462
f52d0ce
dd34e81
899c48f
2b2ba15
85dc588
b075975
9bd07ce
1a1d171
94ddd02
ce4eb33
40c7415
5fb16ee
fdf71e5
049b812
6b43371
cca1319
4d05f1d
788ecf7
e99a3d6
5677838
d42839c
21fcaca
693ecd0
2f9e73d
099a8fa
0d2a458
c145880
532b2d0
bfaf1d5
bc203e0
ba7ca5f
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 |
---|---|---|
|
@@ -282,8 +282,8 @@ static void checkAttributes(AnonymousFunctionExpressionSyntax syntax, SyntaxList | |
MessageID.IDS_FeatureLambdaReturnType.CheckFeatureAvailability(diagnostics, syntax); | ||
|
||
Debug.Assert(syntax is not ScopedTypeSyntax); | ||
syntax = syntax.SkipScoped(out _).SkipRef(out RefKind refKind); | ||
if ((syntax as IdentifierNameSyntax)?.Identifier.ContextualKind() == SyntaxKind.VarKeyword) | ||
syntax = syntax.SkipScoped(out _).SkipRef(diagnostics, out RefKind refKind); | ||
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. here's an example of where we actually use |
||
if (syntax is IdentifierNameSyntax { Identifier.RawContextualKind: (int)SyntaxKind.VarKeyword }) | ||
{ | ||
diagnostics.Add(ErrorCode.ERR_LambdaExplicitReturnTypeVar, syntax.Location); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -50,7 +50,7 @@ internal static GlobalExpressionVariable Create( | |
Debug.Assert(nodeToBind.Kind() == SyntaxKind.VariableDeclarator || nodeToBind is ExpressionSyntax); | ||
|
||
var syntaxReference = syntax.GetReference(); | ||
return (typeSyntax == null || typeSyntax.SkipScoped(out _).SkipRef(out _).IsVar) | ||
return (typeSyntax == null || typeSyntax.SkipScoped(out _).SkipRef().IsVar) | ||
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. sidenote: we have a ton of code taht does this unwrapping to check .IsVar. We may want to consider having that property just do this unwinding anyways. |
||
? new InferrableGlobalExpressionVariable(containingType, modifiers, typeSyntax, name, syntaxReference, location, containingFieldOpt, nodeToBind) | ||
: new GlobalExpressionVariable(containingType, modifiers, typeSyntax, name, syntaxReference, location); | ||
} | ||
|
@@ -88,7 +88,7 @@ internal override TypeWithAnnotations GetFieldType(ConsList<FieldSymbol> fieldsB | |
|
||
if (typeSyntax != null) | ||
{ | ||
type = binder.BindTypeOrVarKeyword(typeSyntax.SkipScoped(out _).SkipRef(out _), diagnostics, out isVar); | ||
type = binder.BindTypeOrVarKeyword(typeSyntax.SkipScoped(out _).SkipRef(), diagnostics, out isVar); | ||
} | ||
else | ||
{ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -73,23 +73,7 @@ public LocalFunctionSymbol( | |
ReportAttributesDisallowed(param.AttributeLists, _declarationDiagnostics); | ||
} | ||
|
||
if (syntax.ReturnType.Kind() == SyntaxKind.RefType) | ||
{ | ||
var returnType = (RefTypeSyntax)syntax.ReturnType; | ||
if (returnType.ReadOnlyKeyword.Kind() == SyntaxKind.ReadOnlyKeyword) | ||
{ | ||
_refKind = RefKind.RefReadOnly; | ||
} | ||
else | ||
{ | ||
_refKind = RefKind.Ref; | ||
} | ||
} | ||
else | ||
{ | ||
_refKind = RefKind.None; | ||
} | ||
|
||
syntax.ReturnType.SkipRef(_declarationDiagnostics, out _refKind); | ||
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. would 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. woudln't that avoid the error about using a |
||
_binder = binder; | ||
} | ||
|
||
|
@@ -235,7 +219,7 @@ internal void ComputeReturnType() | |
var diagnostics = BindingDiagnosticBag.GetInstance(_declarationDiagnostics); | ||
TypeSyntax returnTypeSyntax = Syntax.ReturnType; | ||
Debug.Assert(returnTypeSyntax is not ScopedTypeSyntax); | ||
TypeWithAnnotations returnType = WithTypeParametersBinder.BindType(returnTypeSyntax.SkipScoped(out _).SkipRef(out _), diagnostics); | ||
TypeWithAnnotations returnType = WithTypeParametersBinder.BindType(returnTypeSyntax.SkipScoped(out _).SkipRef(), diagnostics); | ||
|
||
var compilation = DeclaringCompilation; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4477,7 +4477,7 @@ private void AddNonTypeMembers( | |
{ | ||
var fieldSyntax = (FieldDeclarationSyntax)m; | ||
|
||
_ = fieldSyntax.Declaration.Type.SkipScoped(out _).SkipRef(out RefKind refKind); | ||
_ = fieldSyntax.Declaration.Type.SkipScoped(out _).SkipRef(diagnostics, out RefKind refKind); | ||
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. The same as when thsi was being reported in the parser. There was no knowledge at the point of time of parsing what context one was in, so you always got the same error about 'ref'. |
||
|
||
if (IsImplicitClass && reportMisplacedGlobalCode) | ||
{ | ||
|
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.
here's teh general intuition (which i commented in the PR). If a feature is ignoring 'ref', then it's a place where 'ref' is not legal (otherwise they wouldn't be able to ignore it). And those places already have a syntactic check to report that. i.e. we already hve code on line 841 in this file to error for that case. As such, we don't need to report a diagnostic about needing a particular lang version to use 'ref'.
However, if hte feature does care about ref-type and actually uses that ref-kind, then it is only legal to do that in a version of c# that supports
ref
. So if it uses the actual RefKind, it needs to pass in diagnostics to get the right version checks.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.
It seemed like in some places, we are skipping ref, but some other check is deciding conditionally whether ref is OK in that position. For example, in
BindVariableTypeWithAnnotations
. It looks like some of the time you deliberately called the overload to passdiagnostics: null
, like in SourceLocalSymbol.It looks like the
SkipRef
overload that takes a diagnostic bag is specifically checking for the ref locals and returns feature. I am wondering if the name of this method should be changed, e.g.SkipRef()
with no parameters sticks around, andSkipRef(DiagnosticBag, out RefKind
gets renamed to something likeSkipRefInLocalOrReturn(DiagnosticBag, out RefKind)
. Then it might be slightly easier to tell at a glance why it's expected to pass a diagnostic bag or not.