Add @warn_unqualified_access, and apply it to imported methods named 'print'.

Otherwise, people subclassing NSView will accidentally call NSView.print
when they're trying to call Swift.print.

rdar://problem/18309853

Swift SVN r30334

include/swift/AST/Attr.def

@@ -233,6 +233,9 @@ SIMPLE_DECL_ATTR(indirect, Indirect,
                  OnEnum | OnEnumElement | DeclModifier,
                  60)
 
+SIMPLE_DECL_ATTR(warn_unqualified_access, WarnUnqualifiedAccess,
+                 OnFunc /*| OnVar*/ | LongAttribute, 61)
+
 #undef TYPE_ATTR
 #undef DECL_ATTR_ALIAS
 #undef SIMPLE_DECL_ATTR

include/swift/AST/DiagnosticsSema.def

@@ -573,6 +573,8 @@ ERROR(no_objc_tagged_pointer_not_class_protocol,sema_tcd,none,
 ERROR(swift_native_objc_runtime_base_not_on_root_class,sema_tcd,none,
       "@_swift_native_objc_runtime_base_not_on_root_class can only be applied "
       "to root classes", ())
+ERROR(attr_methods_only,sema_tcd,none,
+      "only methods can be declared %0", (DeclAttribute))
 ERROR(access_control_in_protocol,sema_tcd,none,
       "%0 modifier cannot be used in protocols", (DeclAttribute))
 ERROR(access_control_setter,sema_tcd,none,
@@ -1694,6 +1696,18 @@ NOTE(add_parens_to_type,tce_sema,none,
 NOTE(add_self_to_type,tce_sema,none,
      "use '.self' to reference the type object", ())
 
+WARNING(warn_unqualified_access,tce_sema,none,
+        "use of %0 treated as a reference to %1 in %2 %3",
+        (Identifier, DescriptiveDeclKind, DescriptiveDeclKind, DeclName))
+NOTE(fix_unqualified_access_member,tce_sema,none,
+     "use 'self.' to silence this warning", ())
+NOTE(fix_unqualified_access_top_level,tce_sema,none,
+     "use '%0' to reference the %1",
+     (StringRef, DescriptiveDeclKind, Identifier))
+NOTE(fix_unqualified_access_top_level_multi,tce_sema,none,
+     "use '%0' to reference the %1 in module %2",
+     (StringRef, DescriptiveDeclKind, Identifier))
+
 ERROR(type_of_metatype,tce_sema,none,
       "'.dynamicType' is not allowed after a type name", ())
 ERROR(invalid_noescape_use,tce_sema,none,

lib/ClangImporter/ImportDecl.cpp

@@ -5379,6 +5379,21 @@ void ClangImporter::Implementation::importAttributes(
         }
   }
 
+  // Hack: mark any method named "print" with less than two parameters as
+  // warn_unqualified_access.
+  if (auto MD = dyn_cast<FuncDecl>(MappedDecl)) {
+    if (MD->getName().str() == "print" &&
+        MD->getDeclContext()->isTypeContext()) {
+      auto *formalParams = MD->getBodyParamPatterns()[1];
+      if (formalParams->numTopLevelVariables() <= 1) {
+        // Use a non-implicit attribute so it shows up in the generated
+        // interface.
+        MD->getAttrs().add(
+            new (C) WarnUnqualifiedAccessAttr(/*implicit*/false));
+      }
+    }
+  }
+
   // Map __attribute__((warn_unused_result)).
   if (ClangDecl->hasAttr<clang::WarnUnusedResultAttr>()) {
     MappedDecl->getAttrs().add(new (C) WarnUnusedResultAttr(SourceLoc(),

lib/Sema/MiscDiagnostics.cpp

@@ -16,8 +16,9 @@
 
 #include "MiscDiagnostics.h"
 #include "TypeChecker.h"
-#include "swift/Basic/SourceManager.h"
 #include "swift/AST/ASTWalker.h"
+#include "swift/AST/NameLookup.h"
+#include "swift/Basic/SourceManager.h"
 #include "swift/Parse/Lexer.h"
 #include "llvm/ADT/MapVector.h"
 using namespace swift;
@@ -106,8 +107,11 @@ static void diagUnreachableCode(TypeChecker &TC, const Stmt *S) {
 ///   - Metatype names cannot generally be used as values: they need a "T.self"
 ///     qualification unless used in narrow case (e.g. T() for construction).
 ///   - '_' may only exist on the LHS of an assignment expression.
+///   - warn_unqualified_access values must not be accessed except via qualified
+///     lookup.
 ///
-static void diagSyntacticUseRestrictions(TypeChecker &TC, const Expr *E) {
+static void diagSyntacticUseRestrictions(TypeChecker &TC, const Expr *E,
+                                         const DeclContext *DC) {
   class DiagnoseWalker : public ASTWalker {
     SmallPtrSet<Expr*, 4> AlreadyDiagnosedMetatypes;
     SmallPtrSet<DeclRefExpr*, 4> AlreadyDiagnosedNoEscapes;
@@ -116,8 +120,9 @@ static void diagSyntacticUseRestrictions(TypeChecker &TC, const Expr *E) {
     SmallPtrSet<DiscardAssignmentExpr*, 2> CorrectDiscardAssignmentExprs;
   public:
     TypeChecker &TC;
+    const DeclContext *DC;
 
-    DiagnoseWalker(TypeChecker &TC) : TC(TC) {}
+    DiagnoseWalker(TypeChecker &TC, const DeclContext *DC) : TC(TC), DC(DC) {}
 
     // Not interested in going outside a basic expression.
     std::pair<bool, Stmt *> walkToStmtPre(Stmt *S) override {
@@ -148,6 +153,9 @@ static void diagSyntacticUseRestrictions(TypeChecker &TC, const Expr *E) {
 
         // Verify noescape parameter uses.
         checkNoEscapeParameterUse(DRE, nullptr);
+
+        // Verify warn_unqualified_access uses.
+        checkUnqualifiedAccessUse(DRE);
       }
       if (auto *MRE = dyn_cast<MemberRefExpr>(Base))
         if (isa<TypeDecl>(MRE->getMember().getDecl()))
@@ -322,9 +330,81 @@ static void diagSyntacticUseRestrictions(TypeChecker &TC, const Expr *E) {
       TC.diagnose(E->getEndLoc(), diag::add_self_to_type)
         .fixItInsertAfter(E->getEndLoc(), ".self");
     }
+
+    void checkUnqualifiedAccessUse(const DeclRefExpr *DRE) {
+      const Decl *D = DRE->getDecl();
+      if (!D->getAttrs().hasAttribute<WarnUnqualifiedAccessAttr>())
+        return;
+
+      if (auto *parentExpr = Parent.getAsExpr()) {
+        if (auto *ignoredBase = dyn_cast<DotSyntaxBaseIgnoredExpr>(parentExpr)){
+          if (!ignoredBase->isImplicit())
+            return;
+        }
+        if (auto *calledBase = dyn_cast<DotSyntaxCallExpr>(parentExpr)) {
+          if (!calledBase->isImplicit())
+            return;
+        }
+      }
+
+      const auto *VD = cast<ValueDecl>(D);
+      const TypeDecl *declParent =
+          VD->getDeclContext()->isNominalTypeOrNominalTypeExtensionContext();
+      if (!declParent) {
+        assert(VD->getDeclContext()->isModuleScopeContext());
+        declParent = VD->getDeclContext()->getParentModule();
+      }
+
+      TC.diagnose(DRE->getLoc(), diag::warn_unqualified_access,
+                  VD->getName(), VD->getDescriptiveKind(),
+                  declParent->getDescriptiveKind(), declParent->getFullName());
+      TC.diagnose(VD, diag::decl_declared_here, VD->getName());
+
+      if (VD->getDeclContext()->isTypeContext()) {
+        TC.diagnose(DRE->getLoc(), diag::fix_unqualified_access_member)
+          .fixItInsert(DRE->getStartLoc(), "self.");
+      }
+
+      DeclContext *topLevelContext = DC->getModuleScopeContext();
+      UnqualifiedLookup lookup(VD->getBaseName(), topLevelContext, &TC,
+                               /*knownPrivate*/true);
+
+      // Group results by module. Pick an arbitrary result from each module.
+      llvm::SmallDenseMap<const ModuleDecl*,const ValueDecl*,4> resultsByModule;
+      for (auto &result : lookup.Results) {
+        const ValueDecl *value = result.getValueDecl();
+        resultsByModule.insert(std::make_pair(value->getModuleContext(),value));
+      }
+
+      // Sort by module name.
+      using ModuleValuePair = std::pair<const ModuleDecl *, const ValueDecl *>;
+      SmallVector<ModuleValuePair, 4> sortedResults{
+        resultsByModule.begin(), resultsByModule.end()
+      };
+      llvm::array_pod_sort(sortedResults.begin(), sortedResults.end(),
+                           [](const ModuleValuePair *lhs,
+                              const ModuleValuePair *rhs) {
+        return lhs->first->getName().compare(rhs->first->getName());
+      });
+
+      auto topLevelDiag = diag::fix_unqualified_access_top_level;
+      if (sortedResults.size() > 1)
+        topLevelDiag = diag::fix_unqualified_access_top_level_multi;
+
+      for (const ModuleValuePair &pair : sortedResults) {
+        DescriptiveDeclKind k = pair.second->getDescriptiveKind();
+
+        SmallString<32> namePlusDot = pair.first->getName().str();
+        namePlusDot.push_back('.');
+
+        TC.diagnose(DRE->getLoc(), topLevelDiag,
+                    namePlusDot, k, pair.first->getName())
+          .fixItInsert(DRE->getStartLoc(), namePlusDot);
+      }
+    }
   };
 
-  DiagnoseWalker Walker(TC);
+  DiagnoseWalker Walker(TC, DC);
   const_cast<Expr *>(E)->walk(Walker);
 }
 
@@ -834,7 +914,7 @@ static void diagAvailability(TypeChecker &TC, const Expr *E,
 void swift::performSyntacticExprDiagnostics(TypeChecker &TC, const Expr *E,
                                             const DeclContext *DC) {
   diagSelfAssignment(TC, E);
-  diagSyntacticUseRestrictions(TC, E);
+  diagSyntacticUseRestrictions(TC, E, DC);
   diagRecursivePropertyAccess(TC, E, DC);
   diagnoseImplicitSelfUseInClosure(TC, E, DC);
   diagAvailability(TC, E, DC);

lib/Sema/TypeCheckAttr.cpp

@@ -123,6 +123,12 @@ public:
     }
   }
 
+  void visitWarnUnqualifiedAccessAttr(WarnUnqualifiedAccessAttr *attr) {
+    if (!D->getDeclContext()->isTypeContext()) {
+      diagnoseAndRemoveAttr(attr, diag::attr_methods_only, attr);
+    }
+  }
+
   void visitIBActionAttr(IBActionAttr *attr);
   void visitLazyAttr(LazyAttr *attr);
   void visitIBDesignableAttr(IBDesignableAttr *attr);
@@ -625,6 +631,7 @@ public:
     IGNORED_ATTR(RequiresStoredPropertyInits)
     IGNORED_ATTR(SILStored)
     IGNORED_ATTR(Testable)
+    IGNORED_ATTR(WarnUnqualifiedAccess)
 #undef IGNORED_ATTR
 
   void visitAvailableAttr(AvailableAttr *attr);

lib/Sema/TypeCheckDecl.cpp

@@ -4930,6 +4930,7 @@ public:
     UNINTERESTING_ATTR(Testable)
 
     UNINTERESTING_ATTR(WarnUnusedResult)
+    UNINTERESTING_ATTR(WarnUnqualifiedAccess)
 
 #undef UNINTERESTING_ATTR
 

test/ClangModules/AppKit_test.swift

@@ -20,3 +20,20 @@ func test(URL: NSURL, controller: NSDocumentController) {
 
   try! controller.makeDocumentWithContentsOfURL(URL, ofType: "")
 }
+
+extension NSBox {
+  func foo() {
+    print("abc") // expected-warning {{use of 'print' treated as a reference to instance method in class 'NSView'}}
+    // expected-note@-1 {{use 'self.' to silence this warning}}
+    // expected-note@-2 {{use 'Swift.' to reference the global function}}
+  }
+}
+
+class MyView : NSView {
+  func foo() {
+    print("abc") // expected-warning {{use of 'print' treated as a reference to instance method in class 'NSView'}}
+    // expected-note@-1 {{use 'self.' to silence this warning}}
+    // expected-note@-2 {{use 'Swift.' to reference the global function}}
+  }
+}
+

test/ClangModules/Inputs/custom-modules/ObjCParseExtras.h

@@ -143,3 +143,14 @@ __weak id globalWeakVar;
 
 typedef NSObject <NSCopying> *CopyableNSObject;
 typedef SomeCell <NSCopying> *CopyableSomeCell;
+
+
+@interface Printing : NSObject
+- (void)print;
+- (void)print:(id)thing;
+- (void)print:(id)thing options:(id)options;
+
++ (void)print;
++ (void)print:(id)thing;
++ (void)print:(id)thing options:(id)options;
+@end

test/ClangModules/objc_parse.swift

@@ -508,3 +508,29 @@ func testProtocolQualified(obj: CopyableNSObject, cell: CopyableSomeCell,
   _ = plainObj as CopyableNSObject // expected-error {{'NSObject' is not convertible to 'CopyableNSObject'; did you mean to use 'as!' to force downcast?}}
   _ = plainCell as CopyableSomeCell // FIXME: This is not really typesafe.
 }
+
+extension Printing {
+  func testImplicitWarnUnqualifiedAccess() {
+    print() // expected-warning {{use of 'print' treated as a reference to instance method in class 'Printing'}}
+    // expected-note@-1 {{use 'self.' to silence this warning}}
+    // expected-note@-2 {{use 'Swift.' to reference the global function}}
+
+    print(self) // expected-warning {{use of 'print' treated as a reference to instance method in class 'Printing'}}
+    // expected-note@-1 {{use 'self.' to silence this warning}}
+    // expected-note@-2 {{use 'Swift.' to reference the global function}}
+
+    print(self, options: self) // no-warning
+  }
+
+  static func testImplicitWarnUnqualifiedAccess() {
+    print() // expected-warning {{use of 'print' treated as a reference to class method in class 'Printing'}}
+    // expected-note@-1 {{use 'self.' to silence this warning}}
+    // expected-note@-2 {{use 'Swift.' to reference the global function}}
+
+    print(self) // expected-warning {{use of 'print' treated as a reference to class method in class 'Printing'}}
+    // expected-note@-1 {{use 'self.' to silence this warning}}
+    // expected-note@-2 {{use 'Swift.' to reference the global function}}
+
+    print(self, options: self) // no-warning
+  }
+}

test/IDE/complete_decl_attribute.swift

@@ -45,7 +45,7 @@ func method(@#^KEYWORD1^#) {}
 @#^KEYWORD2^#
 func method(){}
 
-// KEYWORD2:                  Begin completions, 7 items
+// KEYWORD2:                  Begin completions, 8 items
 // KEYWORD2-NEXT:             Keyword/None:                       available[#Func Attribute#]; name=available{{$}}
 // KEYWORD2-NEXT:             Keyword/None:                       objc[#Func Attribute#]; name=objc{{$}}
 // KEYWORD2-NEXT:             Keyword/None:                       noreturn[#Func Attribute#]; name=noreturn{{$}}
@@ -53,6 +53,7 @@ func method(){}
 // KEYWORD2-NEXT:             Keyword/None:                       inline[#Func Attribute#]; name=inline{{$}}
 // KEYWORD2-NEXT:             Keyword/None:                       nonobjc[#Func Attribute#]; name=nonobjc{{$}}
 // KEYWORD2-NEXT:             Keyword/None:                       warn_unused_result[#Func Attribute#]; name=warn_unused_result{{$}}
+// KEYWORD2-NEXT:             Keyword/None:                       warn_unqualified_access[#Func Attribute#]; name=warn_unqualified_access{{$}}
 // KEYWORD2-NEXT:             End completions
 
 @#^KEYWORD3^#
@@ -85,7 +86,7 @@ struct S{}
 
 @#^KEYWORD_LAST^#
 
-// KEYWORD_LAST:                  Begin completions, 18 items
+// KEYWORD_LAST:                  Begin completions, 19 items
 // KEYWORD_LAST-NEXT:             Keyword/None:                       available[#Declaration Attribute#]; name=available{{$}}
 // KEYWORD_LAST-NEXT:             Keyword/None:                       objc[#Declaration Attribute#]; name=objc{{$}}
 // KEYWORD_LAST-NEXT:             Keyword/None:                       noreturn[#Declaration Attribute#]; name=noreturn{{$}}
@@ -104,4 +105,5 @@ struct S{}
 // KEYWORD_LAST-NEXT:             Keyword/None:                       NSApplicationMain[#Declaration Attribute#]; name=NSApplicationMain{{$}}
 // KEYWORD_LAST-NEXT:             Keyword/None:                       objc_non_lazy_realization[#Declaration Attribute#]; name=objc_non_lazy_realization{{$}}
 // KEYWORD_LAST-NEXT:             Keyword/None:                       warn_unused_result[#Declaration Attribute#]; name=warn_unused_result
+// KEYWORD_LAST-NEXT:             Keyword/None:                       warn_unqualified_access[#Declaration Attribute#]; name=warn_unqualified_access
 // KEYWORD_LAST-NEXT:             End completions

test/Inputs/clang-importer-sdk/usr/include/AppKit.h

@@ -144,6 +144,11 @@
 @property (strong, nullable) CALayer *layer;
 @property (readonly, copy, nonnull) NSArray *trackingAreas;
 @property (copy, nonnull) NSArray *subviews;
+
+- (void)print:(id)sender;
+@end
+
+@interface NSBox : NSView
 @end
 
 @interface NSView(NSKeyboardUI)

test/attr/Inputs/warn_unqualified_access_other.swift

@@ -0,0 +1 @@
+public func overloaded() {}

test/attr/warn_unqualified_access.swift

@@ -0,0 +1,83 @@
+// RUN: rm -rf %t && mkdir %t
+// RUN: %target-swift-frontend -emit-module-path %t/Other1.swiftmodule -module-name Other1 %S/Inputs/warn_unqualified_access_other.swift
+// RUN: %target-swift-frontend -emit-module-path %t/Other2.swiftmodule -module-name Other2 %S/Inputs/warn_unqualified_access_other.swift
+// RUN: %target-swift-frontend -I %t -parse %s -verify
+
+import Other1
+import Other2
+
+@warn_unqualified_access // expected-error {{@warn_unqualified_access may only be used on 'func' declarations}}
+var x: Int { return 0 }
+
+@warn_unqualified_access // expected-error {{@warn_unqualified_access may only be used on 'func' declarations}}
+struct X {}
+
+@warn_unqualified_access // expected-error {{only methods can be declared @warn_unqualified_access}}
+func topLevel() {
+}
+
+class Base {
+  @warn_unqualified_access
+  func a() {} // expected-note + {{declared here}}
+
+  @warn_unqualified_access
+  func toBeOverridden() {} // no-warning
+}
+
+extension Base {
+  @warn_unqualified_access
+  func b() {} // expected-note + {{declared here}}
+}
+
+class Sub : Base {
+  @warn_unqualified_access
+  func c() {} // expected-note + {{declared here}}
+
+  override func toBeOverridden() {}
+
+  func test() {
+    a() // expected-warning {{use of 'a' treated as a reference to instance method in class 'Base'}} expected-note{{use 'self.' to silence this warning}} {{5-5=self.}}
+    self.a()
+    b() // expected-warning {{use of 'b' treated as a reference to instance method in class 'Base'}} expected-note{{use 'self.' to silence this warning}} {{5-5=self.}}
+    self.b()
+    c() // expected-warning {{use of 'c' treated as a reference to instance method in class 'Sub'}} expected-note{{use 'self.' to silence this warning}} {{5-5=self.}}
+    self.c()
+
+    toBeOverridden() // no-warning
+  }
+
+  func testWithoutCalling() {
+    _ = a // expected-warning {{use of 'a' treated as a reference to instance method in class 'Base'}} expected-note{{use 'self.' to silence this warning}} {{9-9=self.}}
+    _ = b // expected-warning {{use of 'b' treated as a reference to instance method in class 'Base'}} expected-note{{use 'self.' to silence this warning}} {{9-9=self.}}
+    _ = c // expected-warning {{use of 'c' treated as a reference to instance method in class 'Sub'}} expected-note{{use 'self.' to silence this warning}} {{9-9=self.}}
+  }
+}
+
+func test(sub: Sub) {
+  sub.a()
+  sub.b()
+  sub.c()
+
+  @warn_unqualified_access // expected-error {{only methods can be declared @warn_unqualified_access}}
+  func inner() {
+  }
+  inner()
+}
+
+class Recovery {
+  @warn_unqualified_access
+  func topLevel() {} // expected-note + {{declared here}}
+  @warn_unqualified_access
+  func overloaded(x: Float) {} // expected-note + {{declared here}}
+
+  func test() {
+    topLevel() // expected-warning {{use of 'topLevel' treated as a reference to instance method in class 'Recovery'}}
+    // expected-note@-1 {{use 'self.' to silence this warning}} {{5-5=self.}}
+    // expected-note@-2 {{use 'warn_unqualified_access.' to reference the global function}} {{5-5=warn_unqualified_access.}}
+
+    overloaded(5) // expected-warning {{use of 'overloaded' treated as a reference to instance method in class 'Recovery'}}
+    // expected-note@-1 {{use 'self.' to silence this warning}} {{5-5=self.}}
+    // expected-note@-2 {{use 'Other1.' to reference the global function in module 'Other1'}} {{5-5=Other1.}}
+    // expected-note@-3 {{use 'Other2.' to reference the global function in module 'Other2'}} {{5-5=Other2.}}
+  }
+}