[PATCH] D118361: clang-format: [JS] sort import aliases.

[Vitaly Buka via Phabricator via cfe-commits]

vitalybuka added a comment.

reverted with f750c3d95a0c8bf1d21380ae753fce12010a7561 



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118361/new/

https://reviews.llvm.org/D118361

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118361: clang-format: [JS] sort import aliases.

[Vitaly Buka via Phabricator via cfe-commits]

vitalybuka added a comment.

After this patch 
https://lab.llvm.org/buildbot/#/builders/74/builds/9049/steps/15/logs/stdio

  Note: Google Test filter = SortImportsTestJS.ImportEqAliases
  [==] Running 1 test from 1 test suite.
  [--] Global test environment set-up.
  [--] 1 test from SortImportsTestJS
  [ RUN  ] SortImportsTestJS.ImportEqAliases
  ==6361==WARNING: MemorySanitizer: use-of-uninitialized-value
  #0 0x1341207 in 
clang::format::JavaScriptImportSorter::parseModuleReference(clang::format::AdditionalKeywords
 const&, clang::format::JsModuleReference&) 
/b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/clang/lib/Format/SortJavaScriptImports.cpp:456:9
  #1 0x133ac9d in 
clang::format::JavaScriptImportSorter::parseModuleReferences(clang::format::AdditionalKeywords
 const&, llvm::SmallVectorImpl&) 
/b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/clang/lib/Format/SortJavaScriptImports.cpp:406:12
  #2 0x1336c97 in 
clang::format::JavaScriptImportSorter::analyze(clang::format::TokenAnnotator&, 
llvm::SmallVectorImpl&, 
clang::format::FormatTokenLexer&) 
/b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/clang/lib/Format/SortJavaScriptImports.cpp:149:9
  #3 0x1358d64 in clang::format::TokenAnalyzer::process() 
/b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/clang/lib/Format/TokenAnalyzer.cpp:126:9
  #4 0x133635f in 
clang::format::sortJavaScriptImports(clang::format::FormatStyle const&, 
llvm::StringRef, llvm::ArrayRef, llvm::StringRef) 
/b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/clang/lib/Format/SortJavaScriptImports.cpp:575:46
  #5 0x11cddd1 in clang::format::sortIncludes(clang::format::FormatStyle 
const&, llvm::StringRef, llvm::ArrayRef, 
llvm::StringRef, unsigned int*) 
/b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/clang/lib/Format/Format.cpp:2889:12
  #6 0xd9ead7 in sort 
/b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/clang/unittests/Format/SortImportsTestJS.cpp:28:36
  #7 0xd9ead7 in clang::format::(anonymous 
namespace)::SortImportsTestJS::_verifySort(char const*, int, llvm::StringRef, 
llvm::StringRef, unsigned int, unsigned int) 
/b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/clang/unittests/Format/SortImportsTestJS.cpp:40:26
  #8 0xda4ea6 in clang::format::(anonymous 
namespace)::SortImportsTestJS_ImportEqAliases_Test::TestBody() 
/b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/clang/unittests/Format/SortImportsTestJS.cpp:450:3
  #9 0x106fe3d in HandleExceptionsInMethodIfSupported 
/b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc
  #10 0x106fe3d in testing::Test::Run() 
/b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:2508:5
  #11 0x1075215 in testing::TestInfo::Run() 
/b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:2684:11
  #12 0x10776b0 in testing::TestSuite::Run() 
/b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:2816:28
  #13 0x10b8336 in testing::internal::UnitTestImpl::RunAllTests() 
/b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:5338:44
  #14 0x10b5f4d in 
HandleExceptionsInMethodIfSupported 
/b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc
  #15 0x10b5f4d in testing::UnitTest::Run() 
/b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:4925:10
  #16 0x1047984 in RUN_ALL_TESTS 
/b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/llvm/utils/unittest/googletest/include/gtest/gtest.h:2473:46
  #17 0x1047984 in main 
/b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/llvm/utils/unittest/UnitTestMain/TestMain.cpp:50:10
  #18 0x7f9bc766509a in __libc_start_main 
(/lib/x86_64-linux-gnu/libc.so.6+0x2409a) (BuildId: 
18b9a9a8c523e5cfe5b5d946d605d09242f09798)
  #19 0x459779 in _start 
(/b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm_build_msan_track_origins/tools/clang/unittests/Format/FormatTests+0x459779)
  
Uninitialized value was created by an allocation of 'ref.tmp3' in the stack 
frame of function 
'_ZN5clang6format21sortJavaScriptImportsERKNS0_11FormatStyleEN4llvm9StringRefENS4_8ArrayRefINS_7tooling5RangeEEES5_'
  #0 0x1335d20 in 
clang::format::sortJavaScriptImports(clang::format::FormatStyle const&, 
llvm::StringRef, llvm::ArrayRef, llvm::StringRef) 
/b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/clang/lib/Format/SortJavaScriptImports.cpp:570
  SUMMARY: MemorySanitizer: use-of-uninitialized-value 
/b/sanitizer-x86_64-linux-bootstrap-msan/build/llvm-project/clang/lib/Format/SortJavaScriptImports.cpp:456:9
 in 

[PATCH] D118361: clang-format: [JS] sort import aliases.

[Martin Probst via Phabricator via cfe-commits]

mprobst updated this revision to Diff 403646.
mprobst added a comment.

- make test break if we used alphasort


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118361/new/

https://reviews.llvm.org/D118361

Files:
  clang/unittests/Format/SortImportsTestJS.cpp


Index: clang/unittests/Format/SortImportsTestJS.cpp
===
--- clang/unittests/Format/SortImportsTestJS.cpp
+++ clang/unittests/Format/SortImportsTestJS.cpp
@@ -450,17 +450,17 @@
   verifySort("import {B} from 'bar';\n"
  "import {A} from 'foo';\n"
  "\n"
- "import C = A.C;\n"
- "import Z = B.C.Z;\n"
+ "import Z = A.C;\n"
+ "import Y = B.C.Z;\n"
  "\n"
- "export {C};\n"
+ "export {Z};\n"
  "\n"
  "console.log(Z);\n",
  "import {A} from 'foo';\n"
- "import C = A.C;\n"
- "export {C};\n"
+ "import Z = A.C;\n"
+ "export {Z};\n"
  "import {B} from 'bar';\n"
- "import Z = B.C.Z;\n"
+ "import Y = B.C.Z;\n"
  "\n"
  "console.log(Z);\n");
 }


Index: clang/unittests/Format/SortImportsTestJS.cpp
===
--- clang/unittests/Format/SortImportsTestJS.cpp
+++ clang/unittests/Format/SortImportsTestJS.cpp
@@ -450,17 +450,17 @@
   verifySort("import {B} from 'bar';\n"
  "import {A} from 'foo';\n"
  "\n"
- "import C = A.C;\n"
- "import Z = B.C.Z;\n"
+ "import Z = A.C;\n"
+ "import Y = B.C.Z;\n"
  "\n"
- "export {C};\n"
+ "export {Z};\n"
  "\n"
  "console.log(Z);\n",
  "import {A} from 'foo';\n"
- "import C = A.C;\n"
- "export {C};\n"
+ "import Z = A.C;\n"
+ "export {Z};\n"
  "import {B} from 'bar';\n"
- "import Z = B.C.Z;\n"
+ "import Y = B.C.Z;\n"
  "\n"
  "console.log(Z);\n");
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118361: clang-format: [JS] sort import aliases.

[Martin Probst via Phabricator via cfe-commits]

mprobst abandoned this revision.
mprobst added a comment.

Superseded by https://reviews.llvm.org/D118363 (sorry for the diff confusion).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118361/new/

https://reviews.llvm.org/D118361

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118361: clang-format: [JS] sort import aliases.

[Martin Probst via Phabricator via cfe-commits]

mprobst created this revision.
mprobst added a reviewer: krasimir.
Herald added a subscriber: jeroen.dobbelaere.
mprobst requested review of this revision.
Herald added a project: clang.

Users can define aliases for long symbols using import aliases:

  import X = A.B.C;

Previously, these were unhandled and would terminate import sorting.
With this change, aliases sort as their own group, coming last after all
other imports.

Aliases are not sorted within their group, as they may reference each
other, so order is significant. Aliases sort before ES module exports,
as exports may reference aliases.

  import {A} from 'foo';
  
  import X = A.B.C;
  
  export {X};


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D118361

Files:
  clang/lib/Format/SortJavaScriptImports.cpp
  clang/unittests/Format/SortImportsTestJS.cpp

Index: clang/unittests/Format/SortImportsTestJS.cpp
===
--- clang/unittests/Format/SortImportsTestJS.cpp
+++ clang/unittests/Format/SortImportsTestJS.cpp
@@ -446,6 +446,25 @@
  "const x =   1;");
 }
 
+TEST_F(SortImportsTestJS, ImportEqAliases) {
+  verifySort("import {B} from 'bar';\n"
+ "import {A} from 'foo';\n"
+ "\n"
+ "import C = A.C;\n"
+ "import Z = B.C.Z;\n"
+ "\n"
+ "export {C};\n"
+ "\n"
+ "console.log(Z);\n",
+ "import {A} from 'foo';\n"
+ "import C = A.C;\n"
+ "export {C};\n"
+ "import {B} from 'bar';\n"
+ "import Z = B.C.Z;\n"
+ "\n"
+ "console.log(Z);\n");
+}
+
 } // end namespace
 } // end namespace format
 } // end namespace clang
Index: clang/lib/Format/SortJavaScriptImports.cpp
===
--- clang/lib/Format/SortJavaScriptImports.cpp
+++ clang/lib/Format/SortJavaScriptImports.cpp
@@ -71,8 +71,14 @@
 // required for sorting module references.
 struct JsModuleReference {
   bool FormattingOff = false;
-  bool IsExport = false;
-  // Module references are sorted into these categories, in order.
+  // Module references are sorted coarsely into these three groups, in order.
+  enum ReferenceKind {
+IMPORT,  // import ... from or side-effect imports.
+ALIAS,   // import A = B.C;
+EXPORT,  // export ...
+  };
+  ReferenceKind Kind = ReferenceKind::IMPORT;
+  // Module references are then sorted by these categories, in order.
   enum ReferenceCategory {
 SIDE_EFFECT, // "import 'something';"
 ABSOLUTE,// from 'something'
@@ -101,14 +107,16 @@
 };
 
 bool operator<(const JsModuleReference , const JsModuleReference ) {
-  if (LHS.IsExport != RHS.IsExport)
-return LHS.IsExport < RHS.IsExport;
+  if (LHS.Kind != RHS.Kind)
+return LHS.Kind < RHS.Kind;
   if (LHS.Category != RHS.Category)
 return LHS.Category < RHS.Category;
-  if (LHS.Category == JsModuleReference::ReferenceCategory::SIDE_EFFECT)
-// Side effect imports might be ordering sensitive. Consider them equal so
-// that they maintain their relative order in the stable sort below.
-// This retains transitivity because LHS.Category == RHS.Category here.
+  if (LHS.Category == JsModuleReference::ReferenceCategory::SIDE_EFFECT ||
+  LHS.Kind == JsModuleReference::ReferenceKind::ALIAS)
+// Side effect imports and aliases might be ordering sensitive. Consider
+// them equal so that they maintain their relative order in the stable sort
+// below. This retains transitivity because LHS.Category == RHS.Category
+// here.
 return false;
   // Empty URLs sort *last* (for export {...};).
   if (LHS.URL.empty() != RHS.URL.empty())
@@ -162,9 +170,10 @@
 // Insert breaks between imports and exports.
 ReferencesText += "\n";
 // Separate imports groups with two line breaks, but keep all exports
-// in a single group.
-if (!Reference.IsExport &&
-(Reference.IsExport != References[I + 1].IsExport ||
+// and aliases in a single group.
+// NB: exports sort last, so there is no need to check the next.
+if (Reference.Kind != JsModuleReference::ReferenceKind::EXPORT &&
+(Reference.Kind != References[I + 1].Kind ||
  Reference.Category != References[I + 1].Category))
   ReferencesText += "\n";
   }
@@ -298,7 +307,7 @@
   //   mismatching
   if (Reference->Category == JsModuleReference::SIDE_EFFECT ||
   PreviousReference->Category == JsModuleReference::SIDE_EFFECT ||
-  Reference->IsExport != PreviousReference->IsExport ||
+  Reference->Kind != PreviousReference->Kind ||
   !PreviousReference->Prefix.empty() || !Reference->Prefix.empty() ||
   !PreviousReference->DefaultImport.empty() ||
   !Reference->DefaultImport.empty() || Reference->Symbols.empty() ||
@@ -398,6 +407,8 @@