Skip to content
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

warn on empty OptionSet static constants #27089

Open
wants to merge 3 commits into
base: master
from

Conversation

@Vercantez
Copy link
Contributor

commented Sep 9, 2019

This adds a warning when declaring a static 'none' property for an OptionSet-conforming type. e.g.:
struct MyOptions: OptionSet {
var rawValue: Int
static let none = MyOptions(rawValue: 0) // Warning: 'static let' constant inside MyOptions that conforms to OptionSet produces an empty option set
}

Resolves SR-10118.

This is my first PR for the compiler and it feels like I'm going about adding this warning the wrong way. There's a huge pyramid of ifs and for loops. At the bare minimum I think I should refactor this out into a function. Although I feel like there's a much simpler way to do this. Currently 5 tests are failing. Any pointers would be greatly appreciated. Thank you!

@jrose-apple

This comment has been minimized.

Copy link
Member

commented Sep 9, 2019

Welcome to the compiler! Your logic here is sound, but I think it would be easier to do this the other way around: check when declaring a constant if the current type is an OptionSet, rather than checking for all zero-value constants when handling the OptionSet conformance. @brentdax, did you have an implementation strategy in mind when you marked it as a Starter Bug?

}
}
}
}

This comment has been minimized.

Copy link
@harlanhaskins

harlanhaskins Sep 9, 2019

Collaborator

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

This comment has been minimized.

Copy link
@harlanhaskins

harlanhaskins Sep 9, 2019

Collaborator

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

This comment has been minimized.

Copy link
@Vercantez

Vercantez Sep 9, 2019

Author Contributor

Ah, thanks!

This comment has been minimized.

Copy link
@Vercantez

Vercantez Sep 9, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@harlanhaskins

harlanhaskins Sep 9, 2019

Collaborator

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.

This comment has been minimized.

Copy link
@Vercantez

Vercantez Sep 9, 2019

Author Contributor

Ok thanks!

This comment has been minimized.

Copy link
@Vercantez

Vercantez Sep 12, 2019

Author Contributor

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

@brentdax

This comment has been minimized.

Copy link
Collaborator

commented Sep 9, 2019

@jrose-apple I don't think I did. This might be a little tougher than I anticipated—sorry, @Vercantez!

}
}
}
}

This comment has been minimized.

Copy link
@brentdax

brentdax Sep 9, 2019

Collaborator

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.

This comment has been minimized.

Copy link
@Vercantez

Vercantez Sep 9, 2019

Author Contributor

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

@jrose-apple jrose-apple self-requested a review Sep 12, 2019

@@ -2383,6 +2385,42 @@ class DeclChecker : public DeclVisitor<DeclChecker> {
if (VD->getAttrs().hasAttribute<DynamicReplacementAttr>())
TC.checkDynamicReplacementAttribute(VD);
}

void checkForEmptyOptionSet(VarDecl *VD) {

This comment has been minimized.

Copy link
@Vercantez

Vercantez Sep 12, 2019

Author Contributor

I'm not sure where this should go

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple Sep 17, 2019

Member

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.

@jrose-apple
Copy link
Member

left a comment

This is looking good so far! I have a number of additional suggestions, but you're on the right track.

@@ -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",

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple Sep 17, 2019

Member

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.

This comment has been minimized.

Copy link
@theblixguy

theblixguy Sep 17, 2019

Collaborator

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.


void checkForEmptyOptionSet(VarDecl *VD) {
if (!VD->isStatic())
return;

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple Sep 17, 2019

Member

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

if (!VD->isStatic())
return;
auto DC = VD->getDeclContext();
auto protocols = DC->getLocalProtocols();

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple Sep 17, 2019

Member

"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 ctor = dyn_cast<CallExpr>(entry.getInit());
if (!ctor) continue;
auto argLabels = ctor->getArgumentLabels();
if (!argLabels.front().is(StringRef("rawValue"))) continue;

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple Sep 17, 2019

Member

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

This comment has been minimized.

Copy link
@theblixguy

theblixguy Sep 17, 2019

Collaborator

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

auto ctor = dyn_cast<CallExpr>(entry.getInit());
if (!ctor) continue;
auto argLabels = ctor->getArgumentLabels();
if (!argLabels.front().is(StringRef("rawValue"))) continue;

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple Sep 17, 2019

Member

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

auto PBD = VD->getParentPatternBinding();
if (!PBD)
return;
for (auto entry : PBD->getPatternList()) {

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple Sep 17, 2019

Member

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.

@@ -2383,6 +2385,42 @@ class DeclChecker : public DeclVisitor<DeclChecker> {
if (VD->getAttrs().hasAttribute<DynamicReplacementAttr>())
TC.checkDynamicReplacementAttribute(VD);
}

void checkForEmptyOptionSet(VarDecl *VD) {

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple Sep 17, 2019

Member

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.

auto val = intArg->getValue();
if (val == 0) {
auto loc = VD->getLoc();
TC.diagnose(loc, diag::option_set_zero_constant, type);

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple Sep 17, 2019

Member

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

return;
for (auto entry : PBD->getPatternList()) {
auto ctor = dyn_cast<CallExpr>(entry.getInit());
if (!ctor) continue;

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple Sep 17, 2019

Member

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

@@ -2383,6 +2385,42 @@ class DeclChecker : public DeclVisitor<DeclChecker> {
if (VD->getAttrs().hasAttribute<DynamicReplacementAttr>())
TC.checkDynamicReplacementAttribute(VD);
}

void checkForEmptyOptionSet(VarDecl *VD) {

This comment has been minimized.

Copy link
@jrose-apple

jrose-apple Sep 17, 2019

Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.