Skip to content

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

Merged
merged 31 commits into from
Oct 31, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
1a5a47f
warn on empty OptionSet static constants
Vercantez Sep 6, 2019
acb99fb
refactor
Vercantez Sep 12, 2019
9c9a327
cleanup
Vercantez Sep 12, 2019
6152f8e
feedback
Vercantez Sep 18, 2019
5ae0484
inline variables
Vercantez Sep 18, 2019
76a8fa6
simplify diagnostic
Vercantez Sep 18, 2019
a665524
fix test
Vercantez Sep 18, 2019
bc109fd
Merge branch 'master' into warn-on-empty-optionset-static-constants
Vercantez Sep 18, 2019
1ba47ff
cleanup
Vercantez Sep 18, 2019
cc0afc0
feedback
Vercantez Sep 18, 2019
13acf10
Merge branch 'warn-on-empty-optionset-static-constants' of https://gi…
Vercantez Sep 18, 2019
2104984
more unit test cases
Vercantez Sep 21, 2019
bc308e0
add comments
Vercantez Sep 21, 2019
06c0571
test more cases
Vercantez Oct 1, 2019
c6063b2
fix tests
Vercantez Oct 8, 2019
2d5438d
add more tests
Vercantez Oct 8, 2019
4250d7f
Revert "add more tests"
Vercantez Oct 8, 2019
b7e059d
Revert "fix tests"
Vercantez Oct 8, 2019
f7f9086
Revert "test more cases"
Vercantez Oct 8, 2019
138d731
Merge remote-tracking branch 'upstream/master' into warn-on-empty-opt…
Vercantez Oct 10, 2019
d70b0a4
check for null ctor called value
Vercantez Oct 22, 2019
b276a7e
Revert "Revert "test more cases""
Vercantez Oct 22, 2019
1da9215
Revert "Revert "fix tests""
Vercantez Oct 22, 2019
67cd809
Revert "Revert "add more tests""
Vercantez Oct 22, 2019
9cc2d65
refine test
Vercantez Oct 22, 2019
d04663a
Merge branch 'master' into warn-on-empty-optionset-static-constants
Vercantez Oct 24, 2019
0ba2980
use new API
Vercantez Oct 28, 2019
9090d7f
add test
Vercantez Oct 28, 2019
cba8684
use dyn_cast_or_null
Vercantez Oct 29, 2019
3d9573f
Merge branch 'master' into warn-on-empty-optionset-static-constants
Vercantez Oct 31, 2019
3dba00f
use bool conversion for protocol conformance instead of hasValue()
Vercantez Oct 31, 2019
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
6 changes: 6 additions & 0 deletions include/swift/AST/DiagnosticsSema.def
Original file line number Diff line number Diff line change
Expand Up @@ -1421,6 +1421,12 @@ ERROR(corresponding_param_not_defaulted,none,
NOTE(inherited_default_param_here,none,
"corresponding parameter declared here", ())

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", ())

// Alignment attribute
ERROR(alignment_not_power_of_two,none,
"alignment value must be a power of two", ())
Expand Down
64 changes: 64 additions & 0 deletions lib/Sema/TypeCheckDecl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -484,6 +484,68 @@ 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()))
return;

// Make sure this type conforms to OptionSet
auto *optionSetProto = tc.Context.getProtocol(KnownProtocolKind::OptionSet);
bool conformsToOptionSet = (bool)tc.containsProtocol(
DC->getSelfTypeInContext(),
optionSetProto,
DC,
/*Flags*/None);

if (!conformsToOptionSet)
return;

auto PBD = VD->getParentPatternBinding();
if (!PBD)
return;

auto initIndex = PBD->getPatternEntryIndexForVarDecl(VD);
auto init = PBD->getInit(initIndex);

// Make sure property is being set with a constructor
auto ctor = dyn_cast_or_null<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,
Expand Down Expand Up @@ -2387,6 +2449,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
Expand Down
32 changes: 32 additions & 0 deletions test/Sema/option-set-empty.swift
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 = SomeOptions(rawValue: 0) // expected-error {{value type 'SomeOptions' cannot have a stored property that recursively contains it}}
}

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=([])}}
Copy link
Contributor

Choose a reason for hiding this comment

The 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:

  • Other types
  • Other constructors of MyOptions
  • Values other than 0
  • vars

Copy link
Contributor

Choose a reason for hiding this comment

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

Non-static let too, maybe! And anything else you can think of.

Copy link
Contributor

Choose a reason for hiding this comment

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

Still would like you to add this last test too. :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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?

Copy link
Contributor

Choose a reason for hiding this comment

The 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)
}