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 4 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 @@ -4423,6 +4423,12 @@ ERROR(property_delegate_type_not_usable_from_inline,none,
"must be '@usableFromInline' or public",
(bool, bool))

WARNING(option_set_zero_constant,none,
"%0 %1 produces an empty option set",
(DescriptiveDeclKind, Identifier))
NOTE(option_set_empty_set_init,none,
"use [] to silence this warning", ())
Copy link
Collaborator

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.


#ifndef DIAG_NO_UNDEF
# if defined(DIAG)
# undef DIAG
Expand Down
50 changes: 49 additions & 1 deletion lib/Sema/TypeCheckDecl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -478,6 +478,52 @@ static void checkInheritanceClause(
}
}

// Check for static properties that produce empty option sets
static void checkForEmptyOptionSet(const VarDecl *VD, TypeChecker &tc) {
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);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: We have an 80-column limit; please wrap and align to the first open paren.

Also nitpick: I'd probably just use bool here, but if you want to use auto please pick a name that sounds like a true/false value, like "conformsToOptionSet".

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.

Will do. It is actually returning an Optional of swift::ProtocolConformanceRef. Should I check if the optional is empty to produce a bool instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would it be ok to use a double negation to convert it into a bool?

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. I don't think we use the double negation thing too much in this codebase; an explicit call to Optional::hasValue would be most clear, but also longer.

Now that I know what the type is, leaving it the way it is is also okay. It does, in fact, represent a possibly-absent conformance.

if (!protocolConformance)
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.

Small comment here would be nice

Suggested change
if (!protocolConformance)
// Make sure this type conforms to OptionSet
if (!protocolConformance)

Copy link
Contributor

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.

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

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.

Copy link
Contributor

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

if (entry.getPattern()->getSingleVar() != VD) continue;

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

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.

if (!ctor) continue;
if (!isa<ConstructorDecl>(ctor->getCalledValue())) continue;

if (ctor->getNumArguments() != 1) continue;
auto argLabels = ctor->getArgumentLabels();
if (argLabels.front() != tc.Context.Id_rawValue) continue;

auto *args = cast<TupleExpr>(ctor->getArg());
auto intArg = dyn_cast<IntegerLiteralExpr>(args->getElement(0));
if (!intArg) continue;

auto val = intArg->getValue();
if (val != 0) continue;
Copy link
Collaborator

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.

Suggested change
if (val != 0) continue;
if (intArg->getValue() != 0) continue;


auto loc = VD->getLoc();
tc.diagnose(loc, diag::option_set_zero_constant, DescriptiveDeclKind::StaticProperty, VD->getName());
Copy link
Collaborator

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

Copy link
Collaborator

@theblixguy theblixguy Sep 18, 2019

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.

auto range = ctor->getArg()->getSourceRange();
tc.diagnose(loc, diag::option_set_empty_set_init).fixItReplace(range, "([])");
}
}


/// 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 @@ -2360,6 +2406,8 @@ class DeclChecker : public DeclVisitor<DeclChecker> {

TC.checkDeclAttributes(VD);

checkForEmptyOptionSet(VD, TC);

triggerAccessorSynthesis(TC, VD);

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

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

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

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

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

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

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

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

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

Choose a reason for hiding this comment

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

Nevermind. Figured it out.

}