Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(useImportType): remove useless inline type import qualifier #4201

Merged
merged 1 commit into from
Oct 7, 2024
Merged

fix(useImportType): remove useless inline type import qualifier #4201

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ our [guidelines for writing a good changelog entry](https://github.com/biomejs/b

### Linter

#### Bug Fixes

- Biome no longer crashes when it encounters a string that contain a multibyte character ([#4181](https://github.com/biomejs/biome/issues/4181)).

This fixes a regression introduced in Biome 1.9.3
Expand All @@ -49,6 +51,17 @@ our [guidelines for writing a good changelog entry](https://github.com/biomejs/b

- Fix [#4041](https://github.com/biomejs/biome/issues/4041). Now the rule `useSortedClasses` won't be triggered if `className` is composed only by inlined variables. Contributed by @ematipico

- [useImportType](https://biomejs.dev/linter/rules/use-import-type/) now reports useless inline type qualifiers ([#4178](https://github.com/biomejs/biome/issues/4178)).

The following fix is now proposed:

```diff
- import type { type A, B } from "";
+ import type { A, B } from "";
```

Contributed by @Conaclos

### Parser

#### Bug Fixes
Expand Down
130 changes: 93 additions & 37 deletions crates/biome_js_analyze/src/lint/style/use_import_type.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,10 +136,8 @@ impl Rule for UseImportType {
if import_clause.assertion().is_some() {
return None;
}
if import_clause.type_token().is_some() ||
// Import attributes and type-only imports are not compatible.
import_clause.assertion().is_some()
{
// Import attributes and type-only imports are not compatible.
if import_clause.assertion().is_some() {
return None;
}
let model = ctx.model();
Expand All @@ -157,7 +155,7 @@ impl Rule for UseImportType {
};
match clause.specifier().ok()? {
AnyJsCombinedSpecifier::JsNamedImportSpecifiers(named_specifiers) => {
match named_import_type_fix(model, &named_specifiers) {
match named_import_type_fix(model, &named_specifiers, false) {
Some(NamedImportTypeFix::UseImportType(specifiers)) => {
if is_default_used_as_type {
Some(ImportTypeFix::UseImportType)
Expand All @@ -180,6 +178,10 @@ impl Rule for UseImportType {
Some(ImportTypeFix::AddInlineTypeQualifiers(specifiers))
}
}
Some(NamedImportTypeFix::RemoveInlineTypeQualifiers(_)) => {
// Should not be reached because we pass `false` to `named_import_type_fix`.
None
}
None => is_default_used_as_type
.then_some(ImportTypeFix::ExtractDefaultImportType(vec![])),
}
Expand All @@ -206,6 +208,9 @@ impl Rule for UseImportType {
}
}
AnyJsImportClause::JsImportDefaultClause(clause) => {
if clause.type_token().is_some() {
return None;
}
let default_binding = clause.default_specifier().ok()?.local_name().ok()?;
let default_binding = default_binding.as_js_identifier_binding()?;
if ctx.jsx_runtime() == JsxRuntime::ReactClassic
Expand All @@ -217,14 +222,24 @@ impl Rule for UseImportType {
is_only_used_as_type(model, default_binding).then_some(ImportTypeFix::UseImportType)
}
AnyJsImportClause::JsImportNamedClause(clause) => {
match named_import_type_fix(model, &clause.named_specifiers().ok()?)? {
match named_import_type_fix(
model,
&clause.named_specifiers().ok()?,
clause.type_token().is_some(),
)? {
NamedImportTypeFix::UseImportType(_) => Some(ImportTypeFix::UseImportType),
NamedImportTypeFix::AddInlineTypeQualifiers(specifiers) => {
Some(ImportTypeFix::AddInlineTypeQualifiers(specifiers))
}
NamedImportTypeFix::RemoveInlineTypeQualifiers(type_tokens) => {
Some(ImportTypeFix::RemoveTypeQualifiers(type_tokens))
}
}
}
AnyJsImportClause::JsImportNamespaceClause(clause) => {
if clause.type_token().is_some() {
return None;
}
let namespace_binding = clause.namespace_specifier().ok()?.local_name().ok()?;
let namespace_binding = namespace_binding.as_js_identifier_binding()?;
if ctx.jsx_runtime() == JsxRuntime::ReactClassic
Expand Down Expand Up @@ -300,6 +315,20 @@ impl Rule for UseImportType {
}
diagnostic
}
ImportTypeFix::RemoveTypeQualifiers(type_tokens) => {
let mut diagnostic = RuleDiagnostic::new(
rule_category!(),
import.import_clause().ok()?.type_token()?.text_trimmed_range(),
"The import has this type qualifier that makes all inline type qualifiers useless.",
);
for type_token in type_tokens {
diagnostic = diagnostic.detail(
type_token.text_trimmed_range(),
"This inline type qualifier is useless.",
)
}
return Some(diagnostic);
}
};
Some(diagnostic.note(markup! {
"Importing the types with "<Emphasis>"import type"</Emphasis>" ensures that they are removed by the transpilers and avoids loading unnecessary modules."
Expand Down Expand Up @@ -527,6 +556,11 @@ impl Rule for UseImportType {
mutation.replace_node(specifier.clone(), new_specifier);
}
}
ImportTypeFix::RemoveTypeQualifiers(type_tokens) => {
for type_token in type_tokens {
mutation.remove_token(type_token.clone());
}
}
}
Some(JsRuleAction::new(
ActionCategory::QuickFix,
Expand All @@ -543,6 +577,7 @@ pub enum ImportTypeFix {
ExtractDefaultImportType(Vec<AnyJsNamedImportSpecifier>),
ExtractCombinedImportType,
AddInlineTypeQualifiers(Vec<AnyJsNamedImportSpecifier>),
RemoveTypeQualifiers(Vec<JsSyntaxToken>),
}

/// Returns `true` if all references of `binding` are only used as a type.
Expand All @@ -564,50 +599,71 @@ fn is_only_used_as_type(model: &SemanticModel, binding: &JsIdentifierBinding) ->
pub enum NamedImportTypeFix {
UseImportType(Vec<AnyJsNamedImportSpecifier>),
AddInlineTypeQualifiers(Vec<AnyJsNamedImportSpecifier>),
RemoveInlineTypeQualifiers(Vec<JsSyntaxToken>),
}

fn named_import_type_fix(
model: &SemanticModel,
named_specifiers: &JsNamedImportSpecifiers,
has_type_token: bool,
) -> Option<NamedImportTypeFix> {
let specifiers = named_specifiers.specifiers();
if specifiers.is_empty() {
return None;
};
let mut imports_only_types = true;
let mut specifiers_requiring_type_marker = Vec::with_capacity(specifiers.len());
for specifier in specifiers.iter() {
let Ok(specifier) = specifier else {
imports_only_types = false;
continue;
};
if specifier.type_token().is_none() {
if specifier
.local_name()
.and_then(|local_name| {
Some(is_only_used_as_type(
model,
local_name.as_js_identifier_binding()?,
))
})
.unwrap_or(false)
{
specifiers_requiring_type_marker.push(specifier);
} else {
imports_only_types = false;
if has_type_token {
let mut useless_type_tokens = Vec::with_capacity(specifiers.len());
for specifier in specifiers.iter() {
let Ok(specifier) = specifier else {
continue;
};
if let Some(type_token) = specifier.type_token() {
useless_type_tokens.push(type_token);
}
}
}
if imports_only_types {
Some(NamedImportTypeFix::UseImportType(
specifiers_requiring_type_marker,
))
} else if specifiers_requiring_type_marker.is_empty() {
None
if useless_type_tokens.is_empty() {
None
} else {
Some(NamedImportTypeFix::RemoveInlineTypeQualifiers(
useless_type_tokens,
))
}
} else {
Some(NamedImportTypeFix::AddInlineTypeQualifiers(
specifiers_requiring_type_marker,
))
let mut imports_only_types = true;
let mut specifiers_requiring_type_marker = Vec::with_capacity(specifiers.len());
for specifier in specifiers.iter() {
let Ok(specifier) = specifier else {
imports_only_types = false;
continue;
};
if specifier.type_token().is_none() {
if specifier
.local_name()
.and_then(|local_name| {
Some(is_only_used_as_type(
model,
local_name.as_js_identifier_binding()?,
))
})
.unwrap_or(false)
{
specifiers_requiring_type_marker.push(specifier);
} else {
imports_only_types = false;
}
}
}
if imports_only_types {
Some(NamedImportTypeFix::UseImportType(
specifiers_requiring_type_marker,
))
} else if specifiers_requiring_type_marker.is_empty() {
None
} else {
Some(NamedImportTypeFix::AddInlineTypeQualifiers(
specifiers_requiring_type_marker,
))
}
}
}

Expand Down
7 changes: 3 additions & 4 deletions crates/biome_js_analyze/tests/specs/style/useImportType/invalid-named-imports.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,10 @@ import { X, Y } from "";
type XX = X;
const YY = Y;

//import { type U, V } from "";
//type VV = V;
import { type H, type I, type J } from "";
export type { H, I, J };

import { type X, type Y, type Z } from "";
export type { X, Y, Z };
import type { type M, N, type O } from "";

// multiline
import {
Expand Down
Loading