Add support for replace/original to C# parser; add SourceGenerator tests; incorporate PR feedback #9532
Conversation
cston
changed the title
| /// <summary> | ||
| /// Return true if the current contextual keyword should be treated as a modifier. | ||
| /// </summary> | ||
| private bool IsContextualKeywordActualModifier() |
There was a problem hiding this comment.
I feel like the name would be better if it matched your comment. i.e. "CurrentContextualKeywordShoudlBeTreatedAsAModifier()"
There was a problem hiding this comment.
Renamed ShouldCurrentContextualKeywordBeTreatedAsModifier.
cston
force-pushed
the
gen
branch
2 times, most recently
from
60787d1
to
8608210
Compare
|
In LanguageParser.cs, reset points do not appear to save and restore The method |
|
Also should investigate if adding /removing the |
| @@ -643,6 +643,21 @@ | |||
| <summary>Creates an BaseExpressionSyntax node.</summary> | |||
| </FactoryComment> | |||
| </Node> | |||
| <Node Name="OriginalExpressionSyntax" Base="InstanceExpressionSyntax"> | |||
There was a problem hiding this comment.
An alternate implementation would be to just have original be an ordinary identifier token, like value in a property setter, and let semantic analysis take care of it (i.e. bind it to a method).
There was a problem hiding this comment.
@gafter I'll keep that approach in mind. For now, this follows the approach used for AwaitExpressionSyntax.
cston
force-pushed
the
gen
branch
3 times, most recently
from
330a0cc
to
2e176dc
Compare
|
@gafter, @AlekseyTs Thanks for identifying the incremental parsing issues. I've updated |
|
@gafter, @AlekseyTs, @VSadov Updated PR to include binding and metadata writing of replaced members. Replaced members are now represented in the containing type similar to partial methods - the replacing member is included in |
| @@ -513,6 +513,20 @@ private Symbol GetMemberSymbol(string memberName, TextSpan memberSpan, NamedType | |||
| return implementation; | |||
| } | |||
| } | |||
|
|
|||
| var method = (MethodSymbol)sym; | |||
| while (true) | |||
There was a problem hiding this comment.
Consider adding a comment explaining why do we need the loop.
|
All feedback has been addressed in most recent commits. |
| bool IInvocationExpression.IsVirtual => | ||
| (this.Method.IsVirtual || this.Method.IsAbstract || this.Method.IsOverride) && | ||
| ((object)this.Method.ReplacedBy == null) && | ||
| !this.ReceiverOpt.SuppressVirtualCalls; |
There was a problem hiding this comment.
I think there are two more similar places in this file.
|
|
||
| SourceUserDefinedConversionSymbol previousConversion; | ||
| if (conversionsAsConversions.TryGetValue(conversion, out previousConversion)) | ||
| if (!conversionsBySignature.Add(conversion)) |
There was a problem hiding this comment.
Was the conversionsAsConversions stuff unnecessary?
There was a problem hiding this comment.
I believe method signature collisions can be handled by methodsBySignature alone. As a result, for consistency, conversionsAsConversions was renamed to conversionsBySignature.
|
LGTM aside from the few comments I made. |
cston
force-pushed
the
gen
branch
2 times, most recently
from
e9e6cca
to
a3dfe8a
Compare
Add support for replace/original to C# parser; add tests; and incorporate PR feedback from 5613668.
@dotnet/roslyn-compiler, @dotnet/roslyn-ide, @CyrusNajmabadi, @mattwar please review