-
Notifications
You must be signed in to change notification settings - Fork 10.6k
warn on empty OptionSet static constants #27089
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
Changes from 9 commits
1a5a47f
acb99fb
9c9a327
6152f8e
5ae0484
76a8fa6
a665524
bc109fd
1ba47ff
cc0afc0
13acf10
2104984
bc308e0
06c0571
c6063b2
2d5438d
4250d7f
b7e059d
f7f9086
138d731
d70b0a4
b276a7e
1da9215
67cd809
9cc2d65
d04663a
0ba2980
9090d7f
cba8684
3d9573f
3dba00f
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 |
|---|---|---|
|
|
@@ -4623,6 +4623,12 @@ NOTE(previous_function_builder_here, none, | |
| ERROR(function_builder_arguments, none, | ||
| "function builder attributes cannot have arguments", ()) | ||
|
|
||
| WARNING(option_set_zero_constant,none, | ||
| "static property %0 produces an empty option set", | ||
| (Identifier)) | ||
| NOTE(option_set_empty_set_init,none, | ||
| "use [] to silence this warning", ()) | ||
|
||
|
|
||
| #ifndef DIAG_NO_UNDEF | ||
| # if defined(DIAG) | ||
| # undef DIAG | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -482,6 +482,47 @@ static void checkInheritanceClause( | |||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| // Check for static properties that produce empty option sets | ||||||||
| static void checkForEmptyOptionSet(const VarDecl *VD, TypeChecker &tc) { | ||||||||
harlanhaskins marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
| if (!VD->isStatic() || !VD->isLet()) | ||||||||
| return; | ||||||||
|
|
||||||||
| auto DC = VD->getDeclContext(); | ||||||||
|
|
||||||||
| auto *optionSetProto = tc.Context.getProtocol(KnownProtocolKind::OptionSet); | ||||||||
| auto protocolConformance = tc.containsProtocol(DC->getSelfTypeInContext(), optionSetProto, DC, /*Flags*/None); | ||||||||
|
||||||||
| if (!protocolConformance) | ||||||||
|
||||||||
| if (!protocolConformance) | |
| // Make sure this type conforms to OptionSet | |
| if (!protocolConformance) |
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.
For that matter, it might make sense to name the variable optionSetConformance—it's more descriptive.
Outdated
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.
This is a cheaper check than the conformance one, so maybe it should go first. (Not like you should have known this ahead of time.)
Outdated
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.
You should be able to get the entry with PBD->getPatternEntryForVarDecl(VD) instead of searching through the list yourself.
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.
Oops, I forgot about this API. This one's my bad. ><
Outdated
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.
A comment specifying the pattern you're looking for would help here.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| // RUN: %target-typecheck-verify-swift | ||
|
|
||
| struct MyOptions: OptionSet { | ||
| var rawValue: Int | ||
| static let none = MyOptions(rawValue: 0) // expected-warning {{static property 'none' produces an empty option set}} expected-note {{use [] to silence this warning}}{{32-45=([])}} | ||
|
Contributor
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. Can you add some test cases that shouldn't cause the error? I can think of a few:
Contributor
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. Non-static
Contributor
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. Still would like you to add this last test too. :-)
Contributor
Author
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. Sorry! I did, but then rolled it back to try to figure out where it was crashing. I think I figured it out now though. Just need to confirm it.
Contributor
Author
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. I added the tests back but I had to make the non-static let of another type or else I'd get a build error because a struct cannot contain itself. Should I add that case anyways and expect the error?
Contributor
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. Ah, right. And if it's for another type it's not a new test. I guess it's still good to add the test case and expect the one error to make sure the second one doesn't pop up. |
||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.
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.
Nit: Can you align the start with the text above?