fix(core): do not flag typescript type-only imports as duplicate (#129)

* fix(core): do not flag type-only imports as duplicate

* fix(core): run rustfmt

crates/rslint_core/src/groups/errors/no_duplicate_imports.rs

@@ -32,21 +32,23 @@ declare_lint! {
     errors,
     tags(Recommended),
     "no-duplicate-imports",
-    /// Whether to check if re-exported 
+    /// Whether to check if re-exported
     pub include_exports: bool
 }
 
 #[typetag::serde]
 impl CstRule for NoDuplicateImports {
     fn check_root(&self, node: &SyntaxNode, ctx: &mut RuleCtx) -> Option<()> {
-        let mut seen: HashMap<String, SyntaxNode> = HashMap::default();
+        // the key of the hashmap is the name of the import source, and
+        // a bool that is `true`, if the import/export has a `type` token.
+        let mut seen: HashMap<(String, bool), SyntaxNode> = HashMap::default();
 
         for child in node.children() {
             if let Some(import) = child.try_to::<ImportDecl>() {
-                self.process_node(&import.source(), false, &mut seen, ctx);
+                self.process_node(&import.source(), import.type_token(), false, &mut seen, ctx);
             } else if self.include_exports {
                 if let Some(export) = child.try_to::<ExportDecl>() {
-                    self.process_node(&export.source(), true, &mut seen, ctx);
+                    self.process_node(&export.source(), export.type_token(), true, &mut seen, ctx);
                 }
             }
         }
@@ -56,31 +58,59 @@ impl CstRule for NoDuplicateImports {
 }
 
 impl NoDuplicateImports {
-    fn process_node(&self, source: &Option<Literal>, is_export: bool, seen: &mut HashMap<String, SyntaxNode>, ctx: &mut RuleCtx) {
-        if let Some(source) = source {
-            if let Some(text) = source.inner_string_text() {
-                let text_as_str = text.to_string();
-                if let Some(old) = seen.get(&text_as_str) {
-                    let err = ctx
-                        .err(
-                            self.name(),
-                            if is_export {
-                                format!("`{}` import is duplicated as export", text_as_str)
-                            } else {
-                                format!("`{}` import is duplicated", text_as_str)
-                            }
-                        )
-                        .secondary(old, format!("`{}` is first used here", text_as_str))
-                        .primary(
-                            source.syntax(),
-                            format!("`{}` is then used again here", text_as_str),
-                        );
-                    ctx.add_err(err);
-                } else {
-                    seen.insert(text.to_string(), source.syntax().clone());
-                }
-            }
+    fn process_node(
+        &self,
+        source: &Option<Literal>,
+        type_token: Option<SyntaxToken>,
+        is_export: bool,
+        seen: &mut HashMap<(String, bool), SyntaxNode>,
+        ctx: &mut RuleCtx,
+    ) -> Option<()> {
+        let source = source.as_ref()?;
+        let text = source.inner_string_text()?;
+        let text_as_str = text.to_string();
+
+        if let Some(old) = seen.get(&(text_as_str, type_token.is_some())) {
+            let err = ctx
+                .err(
+                    self.name(),
+                    if is_export {
+                        format!("`{}` import is duplicated as export", text)
+                    } else {
+                        format!("`{}` import is duplicated", text)
+                    },
+                )
+                .secondary(old, format!("`{}` is first used here", text))
+                .primary(
+                    source.syntax(),
+                    format!("`{}` is then used again here", text),
+                );
+            ctx.add_err(err);
+        } else {
+            seen.insert(
+                (text.to_string(), type_token.is_some()),
+                source.syntax().clone(),
+            );
         }
+
+        None
+    }
+}
+
+ts_rule_tests! {
+    NoDuplicateImports::default(),
+    err: {
+        r#"
+        import type { TypeA } from 'bla';
+        import { a } from 'bla';
+        import type { TypeA } from 'bla';
+        "#,
+    },
+    ok: {
+        r#"
+        import type { TypeA } from 'bla';
+        import { a } from 'bla';
+        "#,
     }
 }
 
@@ -124,4 +154,4 @@ rule_tests! {
         export { foo };
         "#,
     }
-}
+}

crates/rslint_core/src/rule_prelude.rs

@@ -3,7 +3,8 @@
 #[doc(no_inline)]
 pub use crate::{
     autofix::{Fixer, Unwrappable, Wrapping},
-    declare_lint, rule_tests, util, CstRule, Diagnostic, Outcome, RuleCtx, RuleResult, Span,
+    declare_lint, rule_tests, ts_rule_tests, util, CstRule, Diagnostic, Outcome, RuleCtx,
+    RuleResult, Span,
 };
 
 #[doc(no_inline)]

crates/rslint_core/src/testing.rs

@@ -72,3 +72,70 @@ macro_rules! rule_tests {
         }
     };
 }
+
+/// A macro for generating linter rule tests, but parsing and linting typescript code.
+#[macro_export]
+macro_rules! ts_rule_tests {
+    ($rule:expr,
+    err: {
+        $(
+            // An optional tag to give to docgen
+            $(#[$err_meta:meta])*
+            $code:literal
+        ),* $(,)?
+    },
+
+    // Optional doc used in the user facing docs for the
+    // more valid code examples section.
+    $(#[_:meta])*
+    ok: {
+        $(
+            // An optional tag to give to docgen
+            $(#[$ok_meta:meta])*
+            $ok_code:literal
+        ),* $(,)?
+    } $(,)?) => {
+        ts_rule_tests!(typescript_valid, typescript_invalid, $rule, err: { $($code),* }, ok: { $($ok_code),* });
+    };
+    (
+    $ok_name:ident,
+    $err_name:ident,
+    $rule:expr,
+    err: {
+        $(
+            // An optional tag to give to docgen
+            $(#[$err_meta:meta])*
+            $code:literal
+        ),* $(,)?
+    },
+    ok: {
+        $(
+            // An optional tag to give to docgen
+            $(#[$ok_meta:meta])*
+            $ok_code:literal
+        ),* $(,)?
+    } $(,)?) => {
+        #[test]
+        fn $err_name() {
+            $(
+                let res = rslint_parser::parse_typescript($code, 0);
+                let errs = $crate::run_rule(&$rule, 0, res.syntax(), true, &[], std::sync::Arc::from($code.to_string()));
+                if errs.diagnostics.is_empty() {
+                    panic!("\nExpected:\n```\n{}\n```\nto fail linting, but instead it passed (with {} parsing errors)", $code, res.errors().len());
+                }
+            )*
+        }
+
+        #[test]
+        fn $ok_name() {
+            $(
+                let res = rslint_parser::parse_typescript($ok_code, 0);
+                let errs = $crate::run_rule(&$rule, 0, res.syntax(), true, &[], std::sync::Arc::from($ok_code.to_string()));
+
+                if !errs.diagnostics.is_empty() {
+                    panic!("\nExpected:\n```\n{}\n```\nto pass linting, but instead it threw errors (along with {} parsing errors):\n\n", $ok_code, res.errors().len());
+                }
+            )*
+        }
+    };
+}

crates/rslint_parser/src/parse.rs

@@ -231,6 +231,15 @@ pub fn parse_module(text: &str, file_id: usize) -> Parse<Module> {
     Parse::new(green, parse_errors)
 }
 
+/// Same as [`parse_text`] but configures the parser to parse Typescript code
+pub fn parse_typescript(text: &str, file_id: usize) -> Parse<Module> {
+    let (events, errors, tokens) = parse_common(text, file_id, Syntax::default().typescript());
+    let mut tree_sink = LosslessTreeSink::new(text, &tokens);
+    crate::process(&mut tree_sink, events, errors);
+    let (green, parse_errors) = tree_sink.finish();
+    Parse::new(green, parse_errors)
+}
+
 /// Losslessly Parse text into an expression [`Parse`](Parse) which can then be turned into an untyped root [`SyntaxNode`](SyntaxNode).
 /// Or turned into a typed [`Expr`](Expr) with [`tree`](Parse::tree).
 pub fn parse_expr(text: &str, file_id: usize) -> Parse<Expr> {

website/docs/rules/README.md

@@ -11,6 +11,6 @@ as well as allowing users to disable a whole group of rules.
 ## Groups 
 | Name | Description |
 | ---- | ----------- |
-| [style](./style) |  Rules which relate to code style and formatting. |
 | [regex](./regex) |  Rules which relate to regular expressions. |
 | [errors](./errors) |  Rules which relate to productions which are almost always erroneous or cause<br>unexpected behavior. |
+| [style](./style) |  Rules which relate to code style and formatting. |

website/docs/rules/errors/no-duplicate-imports.md

@@ -27,7 +27,7 @@ export { foo };
 ## Config
 | Name | Type | Description |
 | ---- | ---- | ----------- |
-| `includeExports` | bool |  Whether to check if re-exported  |
+| `includeExports` | bool |  Whether to check if re-exported |
 
 ::: details More incorrect examples