From 72b9c3da5d30c39a4dcb677c7a46d2ddae8aca7e Mon Sep 17 00:00:00 2001
From: cherryblossom <31467609+cherryblossom000@users.noreply.github.com>
Date: Mon, 17 May 2021 19:23:39 +1000
Subject: [PATCH] [Fix] `no-cycle`: fix false negative when file imports a type
 after importing a value in Flow

This fixes this situation:

`a.js`:
```js
import { foo } from './b'
```

`b.js`:
```js
// @flow
import { bar, type Baz } from './a'
```

Previously, `no-cycle` would have not reported a dependency cycle for the import in `a.js`, even
though `b.js` is importing `bar`, which is not a type import, from `a.js`. This commit fixes that.
---
 CHANGELOG.md                                       |  1 +
 src/ExportMap.js                                   | 11 ++++++-----
 tests/files/cycles/flow-types-some-type-imports.js |  3 +++
 tests/src/rules/no-cycle.js                        |  5 +++++
 4 files changed, 15 insertions(+), 5 deletions(-)
 create mode 100644 tests/files/cycles/flow-types-some-type-imports.js

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 712bf21a2..03526c700 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -9,6 +9,7 @@ This change log adheres to standards from [Keep a CHANGELOG](http://keepachangel
 ### Fixed
 - [`no-restricted-paths`]: fix false positive matches ([#2090], thanks [@malykhinvi])
 - [`no-cycle`]: ignore imports where imported file only imports types of importing file ([#2083], thanks [@cherryblossom000])
+- [`no-cycle`]: fix false negative when file imports a type after importing a value in Flow ([#2083], thanks [@cherryblossom000])
 
 ### Changed
 - [Docs] Add `no-relative-packages` to list of to the list of rules ([#2075], thanks [@arvigeus])
diff --git a/src/ExportMap.js b/src/ExportMap.js
index 9cc7a089e..76b07f9dc 100644
--- a/src/ExportMap.js
+++ b/src/ExportMap.js
@@ -495,7 +495,9 @@ ExportMap.parse = function (path, content, context) {
     if (n.type === 'ImportDeclaration') {
       // import type { Foo } (TS and Flow)
       const declarationIsType = n.importKind === 'type';
-      let isOnlyImportingTypes = declarationIsType;
+      // import './foo' or import {} from './foo' (both 0 specifiers) is a side effect and
+      // shouldn't be considered to be just importing types
+      let specifiersOnlyImportingTypes = n.specifiers.length;
       const importedSpecifiers = new Set();
       n.specifiers.forEach(specifier => {
         if (supportedImportTypes.has(specifier.type)) {
@@ -506,11 +508,10 @@ ExportMap.parse = function (path, content, context) {
         }
 
         // import { type Foo } (Flow)
-        if (!declarationIsType) {
-          isOnlyImportingTypes = specifier.importKind === 'type';
-        }
+        specifiersOnlyImportingTypes =
+          specifiersOnlyImportingTypes && specifier.importKind === 'type';
       });
-      captureDependency(n, isOnlyImportingTypes, importedSpecifiers);
+      captureDependency(n, declarationIsType || specifiersOnlyImportingTypes, importedSpecifiers);
 
       const ns = n.specifiers.find(s => s.type === 'ImportNamespaceSpecifier');
       if (ns) {
diff --git a/tests/files/cycles/flow-types-some-type-imports.js b/tests/files/cycles/flow-types-some-type-imports.js
new file mode 100644
index 000000000..9008ba1af
--- /dev/null
+++ b/tests/files/cycles/flow-types-some-type-imports.js
@@ -0,0 +1,3 @@
+// @flow
+
+import { foo, type BarType } from './depth-zero'
diff --git a/tests/src/rules/no-cycle.js b/tests/src/rules/no-cycle.js
index 9620eac44..302db8351 100644
--- a/tests/src/rules/no-cycle.js
+++ b/tests/src/rules/no-cycle.js
@@ -87,6 +87,11 @@ ruleTester.run('no-cycle', rule, {
       code: 'import { foo } from "./depth-one"',
       errors: [error(`Dependency cycle detected.`)],
     }),
+    test({
+      code: 'import { bar } from "./flow-types-some-type-imports"',
+      parser: require.resolve('babel-eslint'),
+      errors: [error(`Dependency cycle detected.`)],
+    }),
     test({
       code: 'import { foo } from "cycles/external/depth-one"',
       errors: [error(`Dependency cycle detected.`)],