Parse partial events and constructors #76860
Conversation
dotnet-issue-labeler
bot
added
the
untriaged
jjonescz
force-pushed
the
PartialEventsCtors-01-Parsing
branch
from
91ccc2a to
ba8ca9d
Compare
This comment was marked as resolved.
This comment was marked as resolved.
|
We should bring to ldm if we can just make partial a keyword in the next version. Like we've done for many other cases at this point. There's a simple workaround (using @partial). |
Sounds good to me. Added the question: dotnet/csharplang#9085 |
|
Went ahead with the breaking change. I can address any LDM feedback later, I wouldn't like to block on that since it shouldn't have any effect on subsequent PRs of the feature. |
src/Compilers/CSharp/Test/Syntax/Parsing/PartialEventsAndConstructorsParsingTests.cs
Show resolved
Hide resolved
src/Compilers/CSharp/Test/Syntax/Parsing/LocalFunctionParsingTests.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Test/Syntax/Parsing/PartialEventsAndConstructorsParsingTests.cs
Outdated
Show resolved
Hide resolved
| public void Event_Definition_CSharp13() | ||
| { | ||
| UsingDeclaration(""" | ||
| partial event Action E; |
There was a problem hiding this comment.
I suspect we are in the same position we were in with records, where we can't really prompt for LangVersion with forms like record Rec() { } in old language versions.
I don't recall receiving much in the way of complaints about that, just something to be conscious of.
There was a problem hiding this comment.
Yeah, I haven't investigated that but I imagine that would require some complicated changes in the parser.
There was a problem hiding this comment.
You might do it by reporting a special error when a lookup fails for ‘record’ or ‘partial’ when binding a return type.
There was a problem hiding this comment.
That would work for constructors, not events. I'm also not sure how the diagnostic would work exactly - if it was warning or error, it would be a breaking change even for older langversions. I guess that might be fine, it seems similar to the field breaking change diagnostics.
There was a problem hiding this comment.
if it was warning or error, it would be a breaking change even for older langversions.
The scenario I am describing is one where an error already occurs, for example, partial is used in a type position, but no such type is in scope.
That would work for constructors, not events
I think events would work by simply parsing the partial modifier for them in any LangVersion, then reporting an error on the declaration.
But, to reiterate, I don't think there is a strong need to do these, it's probably something to only do if it is found to be convenient.
There was a problem hiding this comment.
The scenario I am describing is one where an error already occurs, for example, partial is used in a type position, but no such type is in scope.
I see, thanks. I'm going to investigate that possibility in a future PR.
I think events would work by simply parsing the partial modifier for them in any LangVersion, then reporting an error on the declaration.
Yes, you're right, that's what actually happens in the next PR I have prepared.
src/Compilers/CSharp/Test/Syntax/Parsing/PartialEventsAndConstructorsParsingTests.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Test/Syntax/Parsing/PartialEventsAndConstructorsParsingTests.cs
Outdated
Show resolved
Hide resolved
| // (5,50): error CS1513: } expected | ||
| // System.Console.Write(F().GetType().Name); | ||
| Diagnostic(ErrorCode.ERR_RbraceExpected, "").WithLocation(5, 50), | ||
| // (6,9): error CS0267: The 'partial' modifier can only appear immediately before 'class', 'record', 'struct', 'interface', or a method or property return type. |
There was a problem hiding this comment.
Yes, it will be in the next PR (I already have that locally so I would skip adding a prototype comment for that if that's okay)
|
@RikkiGibson for a second review, thanks |
| // partial event | ||
| if (this.PeekToken(1).Kind == SyntaxKind.EventKeyword) | ||
| { | ||
| return IsFeatureEnabled(MessageID.IDS_FeaturePartialEventsAndConstructors); |
There was a problem hiding this comment.
not to belabor it, but, it wasn't obvious to me why event parsing should be conditional on LangVersion. Maybe it's simpler to just be consistent? Perhaps there is some existing form we are trying to avoid breaking? Feel free to add as an open question for a future PR, if any further investigation/work is found to be needed here.
There was a problem hiding this comment.
Good point, I did this for consistency but now I realize in parsing we want to avoid being conditional like this whenever possible. I don't think this can break anyone since event is a keyword anywhere (it's not contextual).
|
@RikkiGibson can you take another look? Thanks. |
jjonescz
merged commit 8b63d50
into
dotnet:features/PartialEventsCtors
Test plan: #76859