Improve diagnostics for generics in enum case #69055
Conversation
dancamarotto
requested review from
hborla,
slavapestov,
ahoppen,
bnbarham,
CodaFi,
DougGregor,
rintaro and
xedin
as code owners
lib/Parse/ParseDecl.cpp
Outdated
| auto genericParams = maybeParseGenericParams() | ||
| .getPtrOrNull(); | ||
| if (genericParams) { | ||
| auto param = genericParams->getParams().front()->getStartLoc(); |
There was a problem hiding this comment.
Just because we parsed a generic parameter clause doesn’t mean it’s non-empty. Please check for that
There was a problem hiding this comment.
Added validation for that check.
There was a problem hiding this comment.
With this change, could case bar<>(param: Int) pass through without any diagnostics? I think this should trap as well.
There was a problem hiding this comment.
case bar<>(param: Int) presents the message: error: expected an identifier to name generic parameter
There was a problem hiding this comment.
This diagnostic may be a little misleading as solving that error by adding a generic parameter name results in another error saying that generic parameters are not allowed.
I think we could do better by emitting the same generic parameter not allowed in such case as well. Any <(and or >) tokens preceding the case identifier would fall under this.
lib/Parse/ParseDecl.cpp
Outdated
| if (param) { | ||
| diagnose(param, diag::generic_param_cant_be_used_in_enum_case_decl); | ||
| Status.setIsParseError(); | ||
| return Status; |
There was a problem hiding this comment.
I don’t think we need to stop parsing here
There was a problem hiding this comment.
removed the return and moved the validation to be executed before checking for params.
Also added a few more tests to cover this.
test/decl/enum/enumtest.swift
Outdated
| @@ -636,3 +636,9 @@ if case nil = foo1 {} // Okay | |||
| if case .none? = foo1 {} // Okay | |||
| if case nil = foo2 {} // Okay | |||
| if case .none?? = foo2 {} // Okay | |||
|
|
|||
| enum EnumCaseWithGenericDeclaration { | |||
| case foo<T>(T) // expected-error {{generic signature cannot be declared in enum 'case'. did you mean to attach it to enum declaration?}} | |||
There was a problem hiding this comment.
Can we offer the second clause here as a fixit that reparents the generic parameters into the parent enum declaration?
There was a problem hiding this comment.
We probably can, I just have no idea how to implement this change however - will take a better look to see if I can come up with something.
lib/Parse/ParseDecl.cpp
Outdated
| @@ -8980,6 +8980,18 @@ Parser::parseDeclEnumCase(ParseDeclOptions Flags, | |||
| if (ArgParams.isNull() || ArgParams.hasCodeCompletion()) | |||
| return ParserStatus(ArgParams); | |||
| } | |||
|
|
|||
| // See if there are generic params. | |||
| auto genericParams = maybeParseGenericParams() | |||
There was a problem hiding this comment.
Can you please add tests for invalid generic clause like
case someCase<Partial
case someOtherCase<subscript>(param: subscript)It seems like maybeParseGenericParams may emit other diagnostics when the generic parmeters is invalid, which will be removed entirely when the generic clause is gone anyways.
lib/Parse/ParseDecl.cpp
Outdated
| // check if there's a following argument type after | ||
| // generic declaration and parse it to ignore it. | ||
| if (Tok.isFollowingLParen()) { |
There was a problem hiding this comment.
Because this diagnosis falls through without returning, you can move this generic result check to be done prior to the previous if statement (which parses the argument type)
There was a problem hiding this comment.
good suggestion. done.
|
Looks like this is a similar case in the new Swift parser (in swift-syntax), which we're moving towards being the only parser. Would you be interested in improving the diagnostic there as well? |
@bnbarham sure, could you provide more info though? should these changes be made within this PR? |
No, the new parser is implemented in apple/swift-syntax. Here's some relevant documentation for improving diagnostics in swift-syntax: https://swiftpackageindex.com/apple/swift-syntax/main/documentation/swiftparser/fixingbugs#Unhelpful-Diagnostic-Produced |
I've found the related issue. Will have a look and also at the documentation you sent - thanks! |
|
@CodaFi I've managed to implement the PR is ready for re-review. |
This PR enhances the error description when declaring generics in enum cases, making it more informative and helpful for developers encountering such issues:
Resolves #69036