Merge pull request swiftlang#34095 from Interfere/SR-13490-fix-compar…

…eImports

include/swift/AST/Import.h

@@ -22,9 +22,10 @@
 #include "swift/AST/Identifier.h"
 #include "swift/Basic/Located.h"
 #include "llvm/ADT/ArrayRef.h"
-#include "llvm/ADT/StringRef.h"
-#include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/StringRef.h"
+#include <algorithm>
 
 namespace swift {
 class ASTContext;
@@ -191,6 +192,20 @@ namespace detail {
   };
 }
 
+/// @name ImportPathBase Comparison Operators
+/// @{
+template <typename Subclass>
+inline bool operator<(const detail::ImportPathBase<Subclass> &LHS,
+                      const detail::ImportPathBase<Subclass> &RHS) {
+  using Element = typename detail::ImportPathBase<Subclass>::Element;
+  auto Comparator = [](const Element &l, const Element &r) {
+    return l.Item.compare(r.Item) < 0;
+  };
+  return std::lexicographical_compare(LHS.begin(), LHS.end(), RHS.begin(),
+                                      RHS.end(), Comparator);
+}
+/// @}
+
 /// An undifferentiated series of dotted identifiers in an \c import statement,
 /// like \c Foo.Bar. Each identifier is packaged with its corresponding source
 /// location.

lib/IDE/ModuleInterfacePrinting.cpp

@@ -374,15 +374,7 @@ static bool printModuleInterfaceDecl(Decl *D,
 
 /// Sorts import declarations for display.
 static bool compareImports(ImportDecl *LHS, ImportDecl *RHS) {
-  // TODO(SR-13490): Probably buggy--thinks "import Foo" == "import Foo.Bar".
-  // ImportPathBase should provide universal comparison functions to avoid this.
-  auto LHSPath = LHS->getImportPath();
-  auto RHSPath = RHS->getImportPath();
-  for (unsigned i: range(std::min(LHSPath.size(), RHSPath.size()))) {
-    if (int Ret = LHSPath[i].Item.str().compare(RHSPath[i].Item.str()))
-      return Ret < 0;
-  }
-  return false;
+  return LHS->getImportPath() < RHS->getImportPath();
 };
 
 /// Sorts Swift declarations for display.

test/IDE/print_ast_overlay.swift

@@ -41,6 +41,6 @@ public class FooOverlayClassDerived : FooOverlayClassBase {
 
 // PASS_NO_INTERNAL-NOT: overlay_func_internal
 
-// PASS_ANNOTATED: <decl:Import>@_exported import <ref:module>Foo</ref>.<ref:module>FooSub</ref></decl>
 // PASS_ANNOTATED: <decl:Import>@_exported import <ref:module>Foo</ref></decl>
+// PASS_ANNOTATED: <decl:Import>@_exported import <ref:module>Foo</ref>.<ref:module>FooSub</ref></decl>
 // PASS_ANNOTATED: <decl:Import>@_exported import <ref:module>FooHelper</ref></decl>

test/SourceKit/InterfaceGen/gen_clang_module.swift

@@ -73,21 +73,21 @@ var x: FooClassBase
 
 // RUN: %sourcekitd-test -req=interface-gen-open -module Foo -- -I %t.overlays -F %S/../Inputs/libIDE-mock-sdk \
 // RUN:         -target %target-triple %clang-importer-sdk-nosource -I %t \
-// RUN:      == -req=cursor -pos=1:8 == -req=cursor -pos=1:12 \
-// RUN:      == -req=cursor -pos=2:10 \
-// RUN:      == -req=cursor -pos=3:10 | %FileCheck -check-prefix=CHECK-IMPORT %s
+// RUN:      == -req=cursor -pos=1:8 \
+// RUN:      == -req=cursor -pos=2:8 == -req=cursor -pos=2:12 \
+// RUN:      == -req=cursor -pos=3:8 | %FileCheck -check-prefix=CHECK-IMPORT %s
 // The cursors point to module names inside the imports, see 'gen_clang_module.swift.response'
 
 // CHECK-IMPORT: 	  source.lang.swift.ref.module ()
 // CHECK-IMPORT-NEXT: Foo{{$}}
 // CHECK-IMPORT-NEXT: Foo{{$}}
 // CHECK-IMPORT: 	  source.lang.swift.ref.module ()
-// CHECK-IMPORT-NEXT: FooSub{{$}}
-// CHECK-IMPORT-NEXT: Foo.FooSub{{$}}
-// CHECK-IMPORT: 	  source.lang.swift.ref.module ()
 // CHECK-IMPORT-NEXT: Foo{{$}}
 // CHECK-IMPORT-NEXT: Foo{{$}}
 // CHECK-IMPORT: 	  source.lang.swift.ref.module ()
+// CHECK-IMPORT-NEXT: FooSub{{$}}
+// CHECK-IMPORT-NEXT: Foo.FooSub{{$}}
+// CHECK-IMPORT: 	  source.lang.swift.ref.module ()
 // CHECK-IMPORT-NEXT: FooHelper{{$}}
 // CHECK-IMPORT-NEXT: FooHelper{{$}}
 

test/SourceKit/InterfaceGen/gen_clang_module.swift.response

@@ -1,5 +1,5 @@
-import Foo.FooSub
 import Foo
+import Foo.FooSub
 import FooHelper
 import SwiftOnoneSupport
 
@@ -370,19 +370,19 @@ public class FooOverlayClassDerived : Foo.FooOverlayClassBase {
     key.length: 3
   },
   {
-    key.kind: source.lang.swift.syntaxtype.identifier,
+    key.kind: source.lang.swift.syntaxtype.keyword,
     key.offset: 11,
     key.length: 6
   },
   {
-    key.kind: source.lang.swift.syntaxtype.keyword,
+    key.kind: source.lang.swift.syntaxtype.identifier,
     key.offset: 18,
-    key.length: 6
+    key.length: 3
   },
   {
     key.kind: source.lang.swift.syntaxtype.identifier,
-    key.offset: 25,
-    key.length: 3
+    key.offset: 22,
+    key.length: 6
   },
   {
     key.kind: source.lang.swift.syntaxtype.keyword,
@@ -3608,13 +3608,13 @@ public class FooOverlayClassDerived : Foo.FooOverlayClassBase {
   },
   {
     key.kind: source.lang.swift.ref.module,
-    key.offset: 11,
-    key.length: 6
+    key.offset: 18,
+    key.length: 3
   },
   {
     key.kind: source.lang.swift.ref.module,
-    key.offset: 25,
-    key.length: 3
+    key.offset: 22,
+    key.length: 6
   },
   {
     key.kind: source.lang.swift.ref.module,

unittests/AST/CMakeLists.txt

@@ -6,6 +6,7 @@ add_swift_unittest(SwiftASTTests
   TestContext.cpp
   TypeMatchTests.cpp
   VersionRangeLattice.cpp
+  ImportTests.cpp
 )
 
 target_link_libraries(SwiftASTTests

unittests/AST/ImportTests.cpp

@@ -0,0 +1,60 @@
+//===--- ImportTests.cpp - Tests for representation of imports ------------===//
+//
+// This source file is part of the Swift.org open source project
+//
+// Copyright (c) 2014 - 2017 Apple Inc. and the Swift project authors
+// Licensed under Apache License v2.0 with Runtime Library Exception
+//
+// See https://swift.org/LICENSE.txt for license information
+// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
+//
+//===----------------------------------------------------------------------===//
+
+#include "TestContext.h"
+#include "swift/AST/Import.h"
+#include "gtest/gtest.h"
+
+using namespace swift;
+using namespace swift::unittest;
+
+namespace {
+/// Helper class used to create ImportPath and hold all strings for identifiers
+class ImportPathContext {
+  TestContext Ctx;
+
+public:
+  ImportPathContext() = default;
+
+  /// Hepler routine for building ImportPath
+  /// Build()
+  /// @see ImportPathBuilder
+  inline ImportPath Build(StringRef Name) noexcept {
+    return ImportPath::Builder(Ctx.Ctx, Name, '.').copyTo(Ctx.Ctx);
+  }
+};
+
+} // namespace
+
+TEST(ImportPath, Comparison) {
+  ImportPathContext ctx;
+
+  /// Simple sanity check:
+  EXPECT_FALSE(ctx.Build("A.B.C") < ctx.Build("A.B.C"));
+
+  /// Check order chain:
+  /// A < A.A < A.A.A < A.A.B < A.B < A.B.A < AA < B < B.A
+  EXPECT_LT(ctx.Build("A"), ctx.Build("A.A"));
+  EXPECT_LT(ctx.Build("A.A"), ctx.Build("A.A.A"));
+  EXPECT_LT(ctx.Build("A.A.A"), ctx.Build("A.A.B"));
+  EXPECT_LT(ctx.Build("A.A.B"), ctx.Build("A.B"));
+  EXPECT_LT(ctx.Build("A.B"), ctx.Build("A.B.A"));
+  EXPECT_LT(ctx.Build("A.B.A"), ctx.Build("AA"));
+  EXPECT_LT(ctx.Build("B"), ctx.Build("B.A"));
+
+  /// Further ImportPaths are semantically incorrect, but we must
+  /// check that comparing them does not cause compiler to crash.
+  EXPECT_LT(ctx.Build("."), ctx.Build("A"));
+  EXPECT_LT(ctx.Build("A"), ctx.Build("AA."));
+  EXPECT_LT(ctx.Build("A"), ctx.Build("AA.."));
+  EXPECT_LT(ctx.Build(".A"), ctx.Build("AA"));
+}