-
Notifications
You must be signed in to change notification settings - Fork 10.5k
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 4 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 | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -478,6 +478,52 @@ static void checkInheritanceClause( | |||||||
} | ||||||||
} | ||||||||
|
||||||||
// Check for static properties that produce empty option sets | ||||||||
static void checkForEmptyOptionSet(const VarDecl *VD, TypeChecker &tc) { | ||||||||
harlanhaskins marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
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); | ||||||||
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. Nitpick: We have an 80-column limit; please wrap and align to the first open paren. Also nitpick: I'd probably just use 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. Will do. It is actually returning an Optional of swift::ProtocolConformanceRef. Should I check if the optional is empty to produce a bool instead? 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. Would it be ok to use a double negation to convert it into a bool? 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. 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) | ||||||||
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. Small comment here would be nice
Suggested change
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. For that matter, it might make sense to name the variable |
||||||||
return; | ||||||||
|
||||||||
auto type = VD->getType(); | ||||||||
if (!type->isEqual(DC->getSelfTypeInContext())) | ||||||||
return; | ||||||||
|
||||||||
auto PBD = VD->getParentPatternBinding(); | ||||||||
if (!PBD) | ||||||||
return; | ||||||||
|
||||||||
for (auto entry : PBD->getPatternList()) { | ||||||||
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. You should be able to get the entry with 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. Oops, I forgot about this API. This one's my bad. >< |
||||||||
if (entry.getPattern()->getSingleVar() != VD) continue; | ||||||||
|
||||||||
auto ctor = dyn_cast<CallExpr>(entry.getInit()); | ||||||||
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. 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; | ||||||||
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. Some of these can be inlined as they're not being used later.
Suggested change
|
||||||||
|
||||||||
auto loc = VD->getLoc(); | ||||||||
tc.diagnose(loc, diag::option_set_zero_constant, DescriptiveDeclKind::StaticProperty, VD->getName()); | ||||||||
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. It's better to pass 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. Actually, I just realised that we want to emit this for static properties only, so you don't even need the |
||||||||
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, | ||||||||
|
@@ -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 | ||||||||
|
@@ -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); }); | ||||||||
} | ||||||||
|
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}} | ||
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 verify if the fix-it is correctly applied? You need to add it at the end of this diagnostic. The format is:
Example:
Also, you need to update this diagnostic as you've changed the text! 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. Thanks! I'd just noticed that. 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. The fix-it would be a part of the note though right? 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. Also, is there any way to run a specific unit test instead of the whole suite? I'm currently using 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. You can invoke
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. Yeah, that was just an example. In your case, you need to do:
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. @harlanhaskins It's saying "Test has no run line!". Sorry if I'm missing something obvious. 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. Nevermind. Figured it out. |
||
} |
There was a problem hiding this comment.
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.