Skip to content

Improve diagnostics for generics in enum case #69055

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.

Sign up for GitHub

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

Closed
dancamarotto wants to merge 9 commits into from
Closed

Improve diagnostics for generics in enum case #69055

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
1a5359b
implementing required changes
dancamarotto Oct 8, 2023
7453af9
implementing unit tests
dancamarotto Oct 8, 2023
601e11d
checking if list is not empty and making some improvements
dancamarotto Oct 9, 2023
109e138
improving error handling by allowing types to be checked
dancamarotto Oct 9, 2023
266fa16
Merge branch 'main' into 69036-generics-enum
dancamarotto Oct 10, 2023
67694b7
implementing fixItInsert
dancamarotto Oct 15, 2023
49026e6
fixing typo
dancamarotto Oct 15, 2023
1be158a
implementing fixItReplace
dancamarotto Oct 15, 2023
e2c26c9
updating unit tests
dancamarotto Oct 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions include/swift/AST/DiagnosticsParse.def
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,8 @@ ERROR(number_cant_start_decl_name,none,
(StringRef))
ERROR(expected_identifier_after_case_comma, PointsToFirstBadToken,
"expected identifier after comma in enum 'case' declaration", ())
ERROR(generic_param_cant_be_used_in_enum_case_decl, PointsToFirstBadToken,
"generic signature cannot be declared in enum 'case'. did you mean to attach it to enum declaration?", ())
ERROR(let_cannot_be_computed_property,none,
"'let' declarations cannot be computed properties", ())
ERROR(let_cannot_be_observing_property,none,
Expand Down
12 changes: 12 additions & 0 deletions lib/Parse/ParseDecl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Copy link
Contributor

@mininny mininny Oct 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

.getPtrOrNull();
if (genericParams) {
auto param = genericParams->getParams().front()->getStartLoc();
Copy link
Contributor

@CodaFi CodaFi Oct 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just because we parsed a generic parameter clause doesn’t mean it’s non-empty. Please check for that

Copy link
Contributor Author

@dancamarotto dancamarotto Oct 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added validation for that check.

Copy link
Contributor

@mininny mininny Oct 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this change, could case bar<>(param: Int) pass through without any diagnostics? I think this should trap as well.

Copy link
Contributor Author

@dancamarotto dancamarotto Oct 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

case bar<>(param: Int) presents the message: error: expected an identifier to name generic parameter

Copy link
Contributor

@mininny mininny Oct 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (param) {
diagnose(param, diag::generic_param_cant_be_used_in_enum_case_decl);
Status.setIsParseError();
return Status;
Copy link
Contributor

@CodaFi CodaFi Oct 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t think we need to stop parsing here

Copy link
Contributor Author

@dancamarotto dancamarotto Oct 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed the return and moved the validation to be executed before checking for params.

Also added a few more tests to cover this.

}
}

// See if there's a raw value expression.
SourceLoc EqualsLoc;
Expand Down
6 changes: 6 additions & 0 deletions test/decl/enum/enumtest.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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?}}
Copy link
Contributor

@CodaFi CodaFi Oct 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we offer the second clause here as a fixit that reparents the generic parameters into the parent enum declaration?

Copy link
Contributor Author

@dancamarotto dancamarotto Oct 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@dancamarotto dancamarotto Oct 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

case bar<T>(param: T) // expected-error {{generic signature cannot be declared in enum 'case'. did you mean to attach it to enum declaration?}}
case baz<T> // expected-error {{generic signature cannot be declared in enum 'case'. did you mean to attach it to enum declaration?}}
}