-
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 4 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 | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -478,6 +478,52 @@ 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.
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.
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.
Some of these can be inlined as they're not being used later.
| if (val != 0) continue; | |
| if (intArg->getValue() != 0) continue; |
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.
It's better to pass VD->getDescriptiveKind() rather than passing a kind directly
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.
Actually, I just realised that we want to emit this for static properties only, so you don't even need the DescriptiveDeclKind - the diagnostic can directly mention it. It could simply be static property %0 produces an empty option set. Although if you want to handle non-static properties as well then yeah you'd need the kind.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| struct MyOptions: OptionSet { | ||
| var rawValue: Int | ||
| static let none = MyOptions(rawValue: 0) // expected-warning {{'static let' constant inside 'MyOptions' that conforms to OptionSet produces an empty option set}} | ||
|
||
| } | ||
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: Same here as well.