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 1 commit
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
27 changes: 27 additions & 0 deletions lib/Sema/TypeCheckProtocol.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4959,6 +4959,33 @@ void TypeChecker::checkConformancesInContext(DeclContext *dc,
diagnoseMissingAppendInterpolationMethod(*this, typeDecl);
}
}

if (conformance->getProtocol()->
isSpecificProtocol(KnownProtocolKind::OptionSet)) {
for (auto member : idc->getMembers()) {
if (auto pbd = dyn_cast<PatternBindingDecl>(member)) {
if (pbd->isStatic())
for (auto entry : pbd->getPatternList()) {
if (auto ctor = dyn_cast<CallExpr>(entry.getInit())) {
auto type = ctor->getType();
auto argLabels = ctor->getArgumentLabels();
auto *args = cast<TupleExpr>(ctor->getArg());
if (type->isEqual(dc->getSelfTypeInContext())) {
if (argLabels[0].is(StringRef("rawValue"))) {
if (auto i = dyn_cast<IntegerLiteralExpr>(args->getElement(0))) {
auto a = i->getValue();
if (a == 0) {
auto loc = member->getLoc();
diagnose(loc, diag::option_set_zero_constant, type);
}
}
}
}
}
}
}
Copy link
Contributor

@beccadax beccadax Sep 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

You're probably going to throw out this code, but if you don't, beware—this closing brace is indented incorrectly. It's indented to match the isStatic() check, but it actually goes with the PatternBindingDecl cast.

Copy link
Contributor Author

@Vercantez Vercantez Sep 9, 2019

Choose a reason for hiding this comment

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

Thanks! I'll fix it in case I keep any of it

}
}
Copy link
Contributor

@harlanhaskins harlanhaskins Sep 9, 2019

Choose a reason for hiding this comment

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

If you're looking for advice how to make this more readable, the two biggest recommendations are

  • Split it into a function, and
  • Early-return

Something like this could help, with comments explaining the checks you're making:

void diagnoseEmptyOptionSetCase(/* args you need... */) {
  auto protocol = conformance->getProtocol();
  if (!protocol->isSpecificProtocol(KnownProtocolKind::OptionSet))
    return;

  for (auto member : idc->getMembers()) {
    auto pbd = dyn_cast<PatternBindingDecl>(member);
    if (!pbd || !pbd->isStatic()) continue;
    // ... rest of the stuff, but early-returning/continuing when the condition is not met
  }
}

Copy link
Contributor

@harlanhaskins harlanhaskins Sep 9, 2019

Choose a reason for hiding this comment

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

Or take Jordan's suggestion, which should simplify the implementation a bit 😄

Copy link
Contributor Author

@Vercantez Vercantez Sep 9, 2019

Choose a reason for hiding this comment

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

Ah, thanks!

Copy link
Contributor Author

@Vercantez Vercantez Sep 9, 2019

Choose a reason for hiding this comment

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

Would you be able to point me in right direction in the codebase? Would that be done at parsing time or at type checking?

Copy link
Contributor

@harlanhaskins harlanhaskins Sep 9, 2019

Choose a reason for hiding this comment

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

This would happen during type checking, after we've assigned types to stored properties. visitBoundVariable in TypeCheckDecl.cpp might be a good place to put this check.

Copy link
Contributor Author

@Vercantez Vercantez Sep 9, 2019

Choose a reason for hiding this comment

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

Ok thanks!

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 took Jordan's and your own suggestions. I didn't know where to put the new function though. I saw there's a MiscDiagnostics file. Would it make sense to put it there? Thanks

}

// Check all conformances.
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.

}