warn on empty OptionSet static constants #27089

Vercantez · 2019-09-09T17:25:33Z

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 · 2019-09-09T17:44:52Z

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?

harlanhaskins · 2019-09-09T17:47:36Z

lib/Sema/TypeCheckProtocol.cpp

+                }
+              }
+          }
+      }


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

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

Ah, thanks!

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

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

beccadax · 2019-09-09T18:31:11Z

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

beccadax · 2019-09-09T18:34:03Z

lib/Sema/TypeCheckProtocol.cpp

+                    }
+                  }
+                }
+              }


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.

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

Vercantez · 2019-09-12T22:43:13Z

lib/Sema/TypeCheckDecl.cpp

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


I'm not sure where this should go

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

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

jrose-apple · 2019-09-16T23:41:05Z

include/swift/AST/DiagnosticsSema.def

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


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.

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.

jrose-apple · 2019-09-16T23:41:32Z

lib/Sema/TypeCheckDecl.cpp

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


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

jrose-apple · 2019-09-16T23:45:05Z

lib/Sema/TypeCheckDecl.cpp

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


"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);

jrose-apple · 2019-09-16T23:45:56Z

lib/Sema/TypeCheckDecl.cpp

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


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

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

jrose-apple · 2019-09-16T23:52:02Z

lib/Sema/TypeCheckDecl.cpp

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


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

jrose-apple · 2019-09-16T23:56:21Z

lib/Sema/TypeCheckDecl.cpp

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


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.

jrose-apple · 2019-09-16T23:57:08Z

lib/Sema/TypeCheckDecl.cpp

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


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 · 2019-09-16T23:58:07Z

lib/Sema/TypeCheckDecl.cpp

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


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

jrose-apple · 2019-09-16T23:59:24Z

lib/Sema/TypeCheckDecl.cpp

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


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

jrose-apple · 2019-09-17T00:13:40Z

lib/Sema/TypeCheckDecl.cpp

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


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

theblixguy · 2019-09-18T13:23:10Z

test/Sema/option-set-empty.swift

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


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!

Thanks! I'd just noticed that.

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

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

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

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

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

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

Nevermind. Figured it out.

theblixguy · 2019-09-18T13:24:15Z

lib/Sema/TypeCheckDecl.cpp

+    if (val != 0) continue;
+
+    auto loc = VD->getLoc();
+    tc.diagnose(loc, diag::option_set_zero_constant, DescriptiveDeclKind::StaticProperty, VD->getName());


It's better to pass VD->getDescriptiveKind() rather than passing a kind directly

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.

theblixguy · 2019-09-18T13:26:41Z

lib/Sema/TypeCheckDecl.cpp

+    if (!intArg) continue;
+
+    auto val = intArg->getValue();
+    if (val != 0) continue;


Some of these can be inlined as they're not being used later.

Suggested change

if (val != 0) continue;

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

theblixguy · 2019-09-18T13:27:47Z

Also, seems like you now have a conflict!

theblixguy · 2019-09-18T16:25:33Z

include/swift/AST/DiagnosticsSema.def

@@ -4623,6 +4623,12 @@ NOTE(previous_function_builder_here, none,
 ERROR(function_builder_arguments, none,
      "function builder attributes cannot have arguments", ())

+WARNING(option_set_zero_constant,none,
+      "static property %0 produces an empty option set",


Nit: Can you align the start with the text above?

theblixguy · 2019-09-18T16:25:53Z

include/swift/AST/DiagnosticsSema.def

+      "static property %0 produces an empty option set",
+      (Identifier))
+NOTE(option_set_empty_set_init,none,
+      "use [] to silence this warning", ())


Nit: Same here as well.

jrose-apple

Looking good!

jrose-apple · 2019-09-18T16:32:07Z

lib/Sema/TypeCheckDecl.cpp

+  auto DC = VD->getDeclContext();
+
+  auto *optionSetProto = tc.Context.getProtocol(KnownProtocolKind::OptionSet);
+  auto protocolConformance = tc.containsProtocol(DC->getSelfTypeInContext(), optionSetProto, DC, /*Flags*/None);


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

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

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

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.

jrose-apple · 2019-09-18T16:32:38Z

lib/Sema/TypeCheckDecl.cpp

+  if (!protocolConformance)
+    return;
+
+  if (!VD->getType()->isEqual(DC->getSelfTypeInContext()))


This is a cheaper check than the conformance one, so maybe it should go first. (Not like you should have known this ahead of time.)

jrose-apple · 2019-09-18T16:35:36Z

test/Sema/option-set-empty.swift

+
+struct MyOptions: OptionSet {
+    var rawValue: Int
+    static let none = MyOptions(rawValue: 0) // expected-warning {{static property 'none' produces an empty option set}} expected-note {{use [] to silence this warning}}{{32-45=([])}}


Can you add some test cases that shouldn't cause the error? I can think of a few:

Other types

Other constructors of MyOptions

Values other than 0

vars

Non-static let too, maybe! And anything else you can think of.

Still would like you to add this last test too. :-)

Sorry! I did, but then rolled it back to try to figure out where it was crashing. I think I figured it out now though. Just need to confirm it.

I added the tests back but I had to make the non-static let of another type or else I'd get a build error because a struct cannot contain itself. Should I add that case anyways and expect the error?

Ah, right. And if it's for another type it's not a new test. I guess it's still good to add the test case and expect the one error to make sure the second one doesn't pop up.

…thub.com/Vercantez/swift into warn-on-empty-optionset-static-constants

harlanhaskins

This is super close, thanks again @Vercantez!

lib/Sema/TypeCheckDecl.cpp

harlanhaskins · 2019-09-18T17:15:20Z

lib/Sema/TypeCheckDecl.cpp

+                                                  DC,
+                                                  /*Flags*/None);
+
+  if (!protocolConformance)


Small comment here would be nice

Suggested change

if (!protocolConformance)

// Make sure this type conforms to OptionSet

if (!protocolConformance)

For that matter, it might make sense to name the variable optionSetConformance—it's more descriptive.

harlanhaskins · 2019-09-18T17:18:32Z

lib/Sema/TypeCheckDecl.cpp

+  if (!PBD)
+    return;
+
+  for (auto entry : PBD->getPatternList()) {


You should be able to get the entry with PBD->getPatternEntryForVarDecl(VD) instead of searching through the list yourself.

Oops, I forgot about this API. This one's my bad. ><

harlanhaskins · 2019-09-18T17:18:52Z

lib/Sema/TypeCheckDecl.cpp

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


A comment specifying the pattern you're looking for would help here.

jrose-apple · 2019-09-23T16:46:17Z

Let's see how we're doing.

@swift-ci Please smoke test

Vercantez · 2019-09-23T22:07:33Z

Oops. I'll try to fix the failures.

Vercantez · 2019-10-01T21:18:34Z

I couldn't figure out why the smoke tests were failing. I'm trying to install Xcode 11 beta 6 so I can have a similar environment to the CI server but I can't seem to find a download. Should the release version of Xcode 11 be ok to run the test suite locally?

jrose-apple · 2019-10-01T21:21:40Z

It should be fine, yeah. I'm running with one of the Xcode 11 GMs myself (forget which one), which is very close to the release.

theblixguy · 2019-10-23T14:14:15Z

@swift-ci please smoke test

theblixguy · 2019-10-23T17:13:47Z

Seems like you need to rebase and update your implementation as there have been some API changes.

jrose-apple · 2019-10-29T00:19:01Z

lib/Sema/TypeCheckDecl.cpp

  // Make sure property is being set with a constructor
-  auto ctor = dyn_cast<CallExpr>(entry.getInit());
+  auto ctor = dyn_cast<CallExpr>(init);


Hm, why the indirection through the index?

@CodaFi removed the direct accessors for requestification

Gotcha. Not that it's important, but you could also compact this a little by using dyn_cast_or_null.

Oh, that's useful! Didn't know about that. Done!

jrose-apple · 2019-10-29T16:21:46Z

@swift-ci please smoke test

Vercantez · 2019-10-30T13:08:00Z

I'm having trouble interpreting the error on the Mac server. Can somebody please give me a pointer?

jrose-apple · 2019-10-30T16:16:14Z

I think these LLDB tests were failing yesterday independently of your change. Let's try again.

@swift-ci Please test macOS

swift-ci · 2019-10-30T16:16:42Z

Build failed
Swift Test OS X Platform
Git Sha - 06c0571

jrose-apple · 2019-10-30T16:16:55Z

@harlanhaskins Any remaining concerns?

jrose-apple · 2019-10-30T16:18:26Z

…or not. @shahmishal, do we need a workspace clean?

09:17:07 ======UPDATE FAILURES======
09:17:07 /Users/buildnode/jenkins/workspace/swift-PR-osx/branch-master/llvm-project failed (ret=1): ['git', 'checkout', u'swift/master']
09:17:07 error: Your local changes to the following files would be overwritten by checkout:
09:17:07 clang/test/Driver/flang/flang.F90
09:17:07 Please commit your changes or stash them before you switch branches.
09:17:07 Aborting

harlanhaskins · 2019-10-30T17:13:05Z

No remaining concerns from me, this looks good to go. Thanks @Vercantez!

shahmishal · 2019-10-30T17:18:47Z

…or not. @shahmishal, do we need a workspace clean?

09:17:07 ======UPDATE FAILURES======
09:17:07 /Users/buildnode/jenkins/workspace/swift-PR-osx/branch-master/llvm-project failed (ret=1): ['git', 'checkout', u'swift/master']
09:17:07 error: Your local changes to the following files would be overwritten by checkout:
09:17:07 clang/test/Driver/flang/flang.F90
09:17:07 Please commit your changes or stash them before you switch branches.
09:17:07 Aborting

swiftlang/llvm-project#63

theblixguy · 2019-10-30T19:42:26Z

@swift-ci Please test macOS

swift-ci · 2019-10-30T20:09:32Z

Build failed
Swift Test OS X Platform
Git Sha - cba8684

theblixguy · 2019-10-30T20:29:18Z

/Users/buildnode/jenkins/workspace/swift-PR-osx/branch-master/swift/lib/Sema/TypeCheckDecl.cpp:506:66: error: no member named 'hasValue' in 'swift::ProtocolConformanceRef'
20:09:13 /Flags/None).hasValue();
20:09:13 ~~~~~~~~~~~~~~ ^

Oops... we got rid of the Optional in #27949 so unfortunately you'll have to rebase again and adjust your usage. You'll have to do something like:

bool conformsToOptionSet = !tc.containsProtocol(DC->getSelfTypeInContext(),
                                                optionSetProto, DC, /*Flags*/None).isInvalid();

(might be worth breaking this check into two lines)

jrose-apple · 2019-10-30T21:51:37Z

It's bool-convertible now, I think, so you can drop the double negative.

theblixguy · 2019-10-30T21:57:13Z

Yep (just checked) 👍

Vercantez · 2019-10-31T13:16:14Z

Done!

jrose-apple · 2019-10-31T16:19:24Z

@swift-ci Please test

swift-ci · 2019-10-31T16:21:48Z

Build failed
Swift Test Linux Platform
Git Sha - 06c0571

swift-ci · 2019-10-31T16:23:44Z

Build failed
Swift Test OS X Platform
Git Sha - cba8684

Harlan acknowledged no further changes.

jrose-apple · 2019-10-31T20:32:42Z

Thank you, @Vercantez!

warn on empty OptionSet static constants

1a5a47f

harlanhaskins reviewed Sep 9, 2019

View reviewed changes

beccadax reviewed Sep 9, 2019

View reviewed changes

Vercantez added 2 commits September 12, 2019 16:23

refactor

acb99fb

cleanup

9c9a327

jrose-apple self-requested a review September 12, 2019 22:02

Vercantez commented Sep 12, 2019

View reviewed changes

jrose-apple reviewed Sep 17, 2019

View reviewed changes

feedback

6152f8e

theblixguy reviewed Sep 18, 2019

View reviewed changes

Vercantez added 5 commits September 18, 2019 11:17

inline variables

5ae0484

simplify diagnostic

76a8fa6

fix test

a665524

Merge branch 'master' into warn-on-empty-optionset-static-constants

bc109fd

cleanup

1ba47ff

theblixguy reviewed Sep 18, 2019

View reviewed changes

jrose-apple reviewed Sep 18, 2019

View reviewed changes

Vercantez added 2 commits September 18, 2019 12:12

feedback

cc0afc0

Merge branch 'warn-on-empty-optionset-static-constants' of https://gi…

13acf10

…thub.com/Vercantez/swift into warn-on-empty-optionset-static-constants

harlanhaskins previously requested changes Sep 18, 2019

View reviewed changes

Vercantez added 2 commits September 21, 2019 17:03

more unit test cases

2104984

add comments

bc308e0

test more cases

06c0571

Vercantez added 3 commits October 24, 2019 16:03

Merge branch 'master' into warn-on-empty-optionset-static-constants

d04663a

use new API

0ba2980

add test

9090d7f

jrose-apple reviewed Oct 29, 2019

View reviewed changes

use dyn_cast_or_null

cba8684

Vercantez added 2 commits October 31, 2019 07:56

Merge branch 'master' into warn-on-empty-optionset-static-constants

3d9573f

use bool conversion for protocol conformance instead of hasValue()

3dba00f

jrose-apple merged commit 43d2904 into swiftlang:master Oct 31, 2019

swift-ci mentioned this pull request Sep 9, 2019

[SR-10118] Warn when OptionSet static constants are initialized with a zero value #52520

Closed

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

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

warn on empty OptionSet static constants #27089

warn on empty OptionSet static constants #27089

Uh oh!

Conversation

Vercantez commented Sep 9, 2019

Uh oh!

jrose-apple commented Sep 9, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

beccadax commented Sep 9, 2019

Uh oh!

beccadax Sep 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrose-apple left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theblixguy Sep 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theblixguy Sep 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

beccadax Sep 9, 2019 •

edited

Loading

theblixguy Sep 18, 2019 •

edited

Loading

theblixguy Sep 18, 2019 •

edited

Loading

theblixguy Sep 18, 2019 •

edited

Loading

theblixguy Sep 18, 2019 •

edited

Loading

Vercantez Sep 18, 2019 •

edited

Loading