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.

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

Merged
jrose-apple merged 31 commits into from
Oct 31, 2019
Merged

warn on empty OptionSet static constants #27089

Show file tree
Hide file tree
Changes from 3 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
4 changes: 4 additions & 0 deletions include/swift/AST/DiagnosticsSema.def
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4423,6 +4423,10 @@ ERROR(property_delegate_type_not_usable_from_inline,none,
"must be '@usableFromInline' or public",
(bool, bool))

WARNING(option_set_zero_constant,none,
"'static let' constant inside %0 that conforms to OptionSet produces an empty option set",
Copy link
Contributor

@jrose-apple jrose-apple Sep 16, 2019

Choose a reason for hiding this comment

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

I think you can drop "that conforms to OptionSet". You've already got "option set" in the diagnostic text, and the type name probably has to do with options already.

Copy link
Collaborator

@theblixguy theblixguy Sep 17, 2019

Choose a reason for hiding this comment

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

Might be simpler to say %0 %1 produces an empty option set where %0 is DescriptiveDeclKind and %1 is an Identifier (property name). Example: static property 'foo' produces an empty option set.

(Type))

#ifndef DIAG_NO_UNDEF
# if defined(DIAG)
# undef DIAG
Expand Down
38 changes: 38 additions & 0 deletions lib/Sema/TypeCheckDecl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2360,6 +2360,8 @@ class DeclChecker : public DeclVisitor<DeclChecker> {

TC.checkDeclAttributes(VD);

checkForEmptyOptionSet(VD);

triggerAccessorSynthesis(TC, VD);

// Under the Swift 3 inference rules, if we have @IBInspectable or
Expand All @@ -2383,6 +2385,42 @@ class DeclChecker : public DeclVisitor<DeclChecker> {
if (VD->getAttrs().hasAttribute<DynamicReplacementAttr>())
TC.checkDynamicReplacementAttribute(VD);
}

void checkForEmptyOptionSet(VarDecl *VD) {
Copy link
Contributor Author

@Vercantez Vercantez Sep 12, 2019

Choose a reason for hiding this comment

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

I'm not sure where this should go

Copy link
Contributor

@jrose-apple jrose-apple Sep 16, 2019

Choose a reason for hiding this comment

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

I think this is an okay place. Another option is to make a static helper function above the definition of DeclChecker. That seems to be where most of the other check* functions are that are only used in this file.

Copy link
Contributor

@jrose-apple jrose-apple Sep 17, 2019

Choose a reason for hiding this comment

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

I might suggest making VD const just to make it clear that it's not going to modify the AST to fix any issues.

if (!VD->isStatic())
return;
Copy link
Contributor

@jrose-apple jrose-apple Sep 16, 2019

Choose a reason for hiding this comment

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

Do you want this to apply to both let and var? If not, this would be a good place to check it too.

auto DC = VD->getDeclContext();
auto protocols = DC->getLocalProtocols();
Copy link
Contributor

@jrose-apple jrose-apple Sep 16, 2019

Choose a reason for hiding this comment

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

"Local" protocols means protocols declared in this same declaration, but if we're in an extension, we probably still want to do this check. Fortunately, I think you can still do it pretty easily:

auto *optionSetProto = TC.Context.getProtocol(KnownProtocolKind::OptionSet);
bool conformsToOptionalSet = TC.containsProtocol(DC->getSelfTypeInContext(), optionSetProto, DC, /*Flags*/None);

auto conformsToOptionSet = false;
for (auto protocol : protocols) {
if (protocol->isSpecificProtocol(KnownProtocolKind::OptionSet)) {
conformsToOptionSet = true;
break;
}
}
if (!conformsToOptionSet)
return;
auto type = VD->getType();
if (!type->isEqual(DC->getSelfTypeInContext()))
return;
auto PBD = VD->getParentPatternBinding();
if (!PBD)
return;
for (auto entry : PBD->getPatternList()) {
Copy link
Contributor

@jrose-apple jrose-apple Sep 16, 2019

Choose a reason for hiding this comment

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

Since one PatternBindingDecl can bind multiple variables:

let (x, y) = foo(), z = bar()

…you should probably be a little more careful to only look at the one you want, so you don't end up emitting duplicate diagnostics. In this case limiting it to when entry.getPattern()->getSingleVar() == VD is probably good enough! That'll ignore the more complex cases, which is what we want for a diagnostic like this.

auto ctor = dyn_cast<CallExpr>(entry.getInit());
if (!ctor) continue;
Copy link
Contributor

@jrose-apple jrose-apple Sep 16, 2019

Choose a reason for hiding this comment

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

It's also worth checking that the thing being called is a ConstructorDecl. ctor->getCalledValue() will give you that.

auto argLabels = ctor->getArgumentLabels();
if (!argLabels.front().is(StringRef("rawValue"))) continue;
Copy link
Contributor

@jrose-apple jrose-apple Sep 16, 2019

Choose a reason for hiding this comment

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

Nitpick: There's an implicit conversion from string literals to StringRef, so you can drop that.

Copy link
Collaborator

@theblixguy theblixguy Sep 17, 2019

Choose a reason for hiding this comment

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

Also I think this should use Id_rawValue from ASTContext, rather than a hardcoded literal.

Copy link
Contributor

@jrose-apple jrose-apple Sep 16, 2019

Choose a reason for hiding this comment

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

It's worth double-checking that there's exactly one label too! In particular, if there are zero labels this will crash.

auto *args = cast<TupleExpr>(ctor->getArg());
auto intArg = dyn_cast<IntegerLiteralExpr>(args->getElement(0));
if (!intArg) continue;
auto val = intArg->getValue();
if (val == 0) {
auto loc = VD->getLoc();
TC.diagnose(loc, diag::option_set_zero_constant, type);
Copy link
Contributor

@jrose-apple jrose-apple Sep 16, 2019

Choose a reason for hiding this comment

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

How about adding a note with a fix-it as well? "use [] to silence this warning"

}
}
}

void visitBoundVars(Pattern *P) {
P->forEachVariable([&] (VarDecl *VD) { this->visitBoundVariable(VD); });
Expand Down
4 changes: 4 additions & 0 deletions test/Sema/option-set-empty.swift
View file
Open in desktop
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}}
Copy link
Collaborator

@theblixguy theblixguy Sep 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Can you verify if the fix-it is correctly applied? You need to add it at the end of this diagnostic. The format is:

{{column_start-column_end=<text>}}

Example:

// expected-warning {{warning_text}}{{30-34=([])}}

Also, you need to update this diagnostic as you've changed the text!

Copy link
Contributor Author

@Vercantez Vercantez Sep 18, 2019

Choose a reason for hiding this comment

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

Thanks! I'd just noticed that.

Copy link
Contributor Author

@Vercantez Vercantez Sep 18, 2019

Choose a reason for hiding this comment

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

The fix-it would be a part of the note though right?

Copy link
Contributor Author

@Vercantez Vercantez Sep 18, 2019

Choose a reason for hiding this comment

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

Also, is there any way to run a specific unit test instead of the whole suite? I'm currently using ninja check-swift

Copy link
Contributor

@harlanhaskins harlanhaskins Sep 18, 2019

Choose a reason for hiding this comment

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

You can invoke lit directly, it lives inside the LLVM checkout

$LLVM_DIR/utils/lit/lit.py -sv $BUILD_DIR/swift-macosx-x84_64/test-macosx-x86_64 --filter=option-set-empty

Copy link
Collaborator

@theblixguy theblixguy Sep 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, that was just an example. In your case, you need to do:

// expected-warning {{text}} expected-note {{text}}{{30-34=([])}}

Copy link
Contributor Author

@Vercantez Vercantez Sep 18, 2019

Choose a reason for hiding this comment

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

@harlanhaskins It's saying "Test has no run line!". Sorry if I'm missing something obvious.

Copy link
Contributor Author

@Vercantez Vercantez Sep 18, 2019

Choose a reason for hiding this comment

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

Nevermind. Figured it out.

}