-
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 27 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 |
|---|---|---|
|
|
@@ -484,6 +484,70 @@ static void checkInheritanceClause( | |
| } | ||
| } | ||
|
|
||
| // Check for static properties that produce empty option sets | ||
| // using a rawValue initializer with a value of '0' | ||
| static void checkForEmptyOptionSet(const VarDecl *VD, TypeChecker &tc) { | ||
| // Check if property is a 'static let' | ||
| if (!VD->isStatic() || !VD->isLet()) | ||
| return; | ||
|
|
||
| auto DC = VD->getDeclContext(); | ||
|
|
||
| // Make sure property is of same type as the type it is declared in | ||
| if (!VD->getType()->isEqual(DC->getSelfTypeInContext())) | ||
harlanhaskins marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| return; | ||
|
|
||
| // Make sure this type conforms to OptionSet | ||
| auto *optionSetProto = tc.Context.getProtocol(KnownProtocolKind::OptionSet); | ||
| bool conformsToOptionSet = tc.containsProtocol( | ||
| DC->getSelfTypeInContext(), | ||
| optionSetProto, | ||
| DC, | ||
| /*Flags*/None).hasValue(); | ||
|
|
||
| if (!conformsToOptionSet) | ||
| return; | ||
|
|
||
| auto PBD = VD->getParentPatternBinding(); | ||
| if (!PBD) | ||
| return; | ||
|
|
||
| auto initIndex = PBD->getPatternEntryIndexForVarDecl(VD); | ||
| auto init = PBD->getInit(initIndex); | ||
| if (!init) | ||
| return; | ||
|
|
||
| // Make sure property is being set with a constructor | ||
| auto ctor = dyn_cast<CallExpr>(init); | ||
|
||
| if (!ctor) | ||
| return; | ||
| auto ctorCalledVal = ctor->getCalledValue(); | ||
| if (!ctorCalledVal) | ||
| return; | ||
| if (!isa<ConstructorDecl>(ctorCalledVal)) | ||
| return; | ||
|
|
||
| // Make sure it is calling the rawValue constructor | ||
| if (ctor->getNumArguments() != 1) | ||
| return; | ||
| if (ctor->getArgumentLabels().front() != tc.Context.Id_rawValue) | ||
| return; | ||
|
|
||
| // Make sure the rawValue parameter is a '0' integer literal | ||
| auto *args = cast<TupleExpr>(ctor->getArg()); | ||
| auto intArg = dyn_cast<IntegerLiteralExpr>(args->getElement(0)); | ||
| if (!intArg) | ||
| return; | ||
| if (intArg->getValue() != 0) | ||
| return; | ||
|
|
||
| auto loc = VD->getLoc(); | ||
| tc.diagnose(loc, diag::option_set_zero_constant, VD->getName()); | ||
| tc.diagnose(loc, diag::option_set_empty_set_init) | ||
| .fixItReplace(args->getSourceRange(), "([])"); | ||
| } | ||
|
|
||
|
|
||
| /// Check the inheritance clauses generic parameters along with any | ||
| /// requirements stored within the generic parameter list. | ||
| static void checkGenericParams(GenericParamList *genericParams, | ||
|
|
@@ -2402,6 +2466,8 @@ class DeclChecker : public DeclVisitor<DeclChecker> { | |
| VD->diagnose(diag::dynamic_self_in_mutable_property); | ||
| } | ||
| } | ||
|
|
||
| checkForEmptyOptionSet(VD, TC); | ||
|
|
||
| // Under the Swift 3 inference rules, if we have @IBInspectable or | ||
| // @GKInspectable but did not infer @objc, warn that the attribute is | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| // RUN: %target-typecheck-verify-swift | ||
|
|
||
| struct SomeOptions: OptionSet { | ||
| var rawValue: Int | ||
|
|
||
| static let some = MyOptions(rawValue: 4) | ||
| static let empty = SomeOptions(rawValue: 0) // expected-warning {{static property 'empty' produces an empty option set}} expected-note {{use [] to silence this warning}}{{35-48=([])}} | ||
| static var otherVal = SomeOptions(rawValue: 0) | ||
|
|
||
| let someVal = MyOptions(rawValue: 6) | ||
| let option = MyOptions(float: Float.infinity) | ||
| let none = MyOptions(rawValue: 0) | ||
| } | ||
|
|
||
| struct MyOptions: OptionSet { | ||
| let rawValue: Int | ||
|
|
||
| init(rawValue: Int) { | ||
| self.rawValue = rawValue | ||
| } | ||
|
|
||
| init(float: Float) { | ||
| self.rawValue = float.exponent | ||
| } | ||
|
|
||
| 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. |
||
| static var nothing = MyOptions(rawValue: 0) | ||
| static let nope = MyOptions() | ||
| static let other = SomeOptions(rawValue: 8) | ||
| static let piVal = MyOptions(float: Float.pi) | ||
| static let zero = MyOptions(float: 0.0) | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.