+
Skip to content
This repository was archived by the owner on Aug 31, 2023. It is now read-only.

fix(rome_js_analyze): fix for unused flag on functional types on ts #3860

Merged
merged 13 commits into from
Dec 5, 2022
Merged

fix(rome_js_analyze): fix for unused flag on functional types on ts #3860

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
8b87f15
fix for unused flag on functional types on ts
xunilrj Nov 25, 2022
357b7ee
codegen issues
xunilrj Nov 28, 2022
ef7f8bd
codegen issues
xunilrj Nov 28, 2022
313c388
rebase and fixing type alias
xunilrj Nov 29, 2022
6c5f6e2
renaming node unions
xunilrj Nov 29, 2022
11bb8f1
renaming functions
xunilrj Nov 29, 2022
fcd053b
fix ts property parameter
xunilrj Nov 30, 2022
9c2bf93
renaming function
xunilrj Nov 30, 2022
65b63fb
clippy and fmt issues
xunilrj Nov 30, 2022
4eb5586
codegen issues
xunilrj Nov 30, 2022
27fe41d
clippy and fmt issues
xunilrj Nov 30, 2022
b99354d
clippy and fmt issues
xunilrj Dec 2, 2022
de7d4fe
more cases of invalid import
xunilrj Dec 3, 2022
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
275 changes: 185 additions & 90 deletions crates/rome_js_analyze/src/semantic_analyzers/correctness/no_unused_variables.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,15 @@
use crate::semantic_services::Semantic;
use crate::JsRuleAction;
use crate::{semantic_services::Semantic, utils::rename::RenameSymbolExtensions};
use rome_analyze::{context::RuleContext, declare_rule, ActionCategory, Rule, RuleDiagnostic};
use rome_console::markup;
use rome_diagnostics::Applicability;
use rome_js_factory::make::{ident, js_identifier_binding};
use rome_js_semantic::{ReferencesExtensions, SemanticScopeExtensions};
use rome_js_syntax::{
JsClassExpression, JsConstructorParameterList, JsConstructorParameters, JsFunctionDeclaration,
JsFunctionExpression, JsIdentifierBinding, JsParameterList, JsParameters, JsSyntaxKind,
JsVariableDeclarator, TsDeclareStatement, TsPropertyParameter,
binding_ext::{AnyJsBindingDeclaration, AnyJsIdentifierBinding, JsAnyParameterParentFunction},
JsClassExpression, JsFunctionDeclaration, JsFunctionExpression, JsSyntaxKind, JsSyntaxNode,
JsVariableDeclarator, TsPropertyParameter,
};
use rome_rowan::{AstNode, BatchMutationExt, SyntaxNodeCast};
use rome_rowan::{AstNode, BatchMutationExt};

declare_rule! {
/// Disallow unused variables.
Expand Down Expand Up @@ -106,83 +105,167 @@ declare_rule! {
}
}

/// Suggestion if the bindnig is unused
#[derive(Copy, Clone)]
pub enum SuggestedFix {
/// No suggestion will be given
NoSuggestion,
/// Suggest to prefix the name of the binding with underscore
PrefixUnderscore,
}

fn is_function_that_is_ok_parameter_not_be_used(
parent_function: Option<JsAnyParameterParentFunction>,
) -> bool {
matches!(
parent_function,
Some(
// bindings in signatures are ok to not be used
JsAnyParameterParentFunction::TsMethodSignatureClassMember(_)
| JsAnyParameterParentFunction::TsCallSignatureTypeMember(_)
| JsAnyParameterParentFunction::TsConstructSignatureTypeMember(_)
| JsAnyParameterParentFunction::TsConstructorSignatureClassMember(_)
| JsAnyParameterParentFunction::TsMethodSignatureTypeMember(_)
| JsAnyParameterParentFunction::TsSetterSignatureClassMember(_)
| JsAnyParameterParentFunction::TsSetterSignatureTypeMember(_)
// bindings in function types are ok to not be used
| JsAnyParameterParentFunction::TsFunctionType(_)
// binding in declare are ok to not be used
| JsAnyParameterParentFunction::TsDeclareFunctionDeclaration(_)
)
)
}

fn is_property_parameter_ok_not_be_used(parameter: TsPropertyParameter) -> Option<bool> {
for modifier in parameter.modifiers() {
if let Some(modifier) = modifier.as_ts_accessibility_modifier() {
match modifier.modifier_token().ok()?.kind() {
// modifiers that are ok to not be used
JsSyntaxKind::PRIVATE_KW | JsSyntaxKind::PUBLIC_KW => return Some(true),
_ => {}
}
}
}

Some(false)
}

fn is_is_ambient_context(node: &JsSyntaxNode) -> bool {
node.ancestors()
.any(|x| x.kind() == JsSyntaxKind::TS_DECLARE_STATEMENT)
}

fn suggestion_for_binding(binding: &AnyJsIdentifierBinding) -> Option<SuggestedFix> {
if binding.is_under_object_pattern_binding()? {
Some(SuggestedFix::NoSuggestion)
} else {
Some(SuggestedFix::PrefixUnderscore)
}
}

// It is ok in some Typescripts constructs for a parameter to be unused.
fn is_typescript_unused_ok(binding: &JsIdentifierBinding) -> Option<()> {
let parent = binding.syntax().parent()?;
match parent.kind() {
JsSyntaxKind::JS_FORMAL_PARAMETER | JsSyntaxKind::JS_REST_PARAMETER => {
let parameter = binding.syntax().parent()?;
let parent = parameter.parent()?;
match parent.kind() {
// example: abstract f(a: number);
JsSyntaxKind::JS_PARAMETER_LIST => {
let parameters = JsParameterList::cast(parent)?.parent::<JsParameters>()?;
match parameters.syntax().parent()?.kind() {
JsSyntaxKind::TS_METHOD_SIGNATURE_CLASS_MEMBER
| JsSyntaxKind::TS_CALL_SIGNATURE_TYPE_MEMBER
| JsSyntaxKind::TS_METHOD_SIGNATURE_TYPE_MEMBER
| JsSyntaxKind::TS_FUNCTION_TYPE
| JsSyntaxKind::TS_DECLARE_FUNCTION_DECLARATION => Some(()),
_ => None,
}
}
// example: constructor(a: number);
JsSyntaxKind::JS_CONSTRUCTOR_PARAMETER_LIST => {
let parameters = JsConstructorParameterList::cast(parent)?
.parent::<JsConstructorParameters>()?;
match parameters.syntax().parent()?.kind() {
JsSyntaxKind::TS_CONSTRUCT_SIGNATURE_TYPE_MEMBER
| JsSyntaxKind::TS_CONSTRUCTOR_SIGNATURE_CLASS_MEMBER => Some(()),
_ => None,
}
}
// example: abstract set a(a: number);
// We don't need get because getter do not have parameters
JsSyntaxKind::TS_SETTER_SIGNATURE_TYPE_MEMBER
| JsSyntaxKind::TS_SETTER_SIGNATURE_CLASS_MEMBER => Some(()),
// example: constructor(a, private b, public c) {}
JsSyntaxKind::TS_PROPERTY_PARAMETER => {
if let Some(parent) = parent.cast::<TsPropertyParameter>() {
for modifier in parent.modifiers().into_iter() {
if let Some(modifier) = modifier.as_ts_accessibility_modifier() {
match modifier.modifier_token().ok()?.kind() {
JsSyntaxKind::PRIVATE_KW | JsSyntaxKind::PUBLIC_KW => {
return Some(())
}
_ => {}
}
}
}
}
// Returning None means is ok to be unused
fn suggested_fix_if_unused(binding: &AnyJsIdentifierBinding) -> Option<SuggestedFix> {
match binding.declaration()? {
// ok to not be used
AnyJsBindingDeclaration::TsIndexSignatureParameter(_)
| AnyJsBindingDeclaration::TsDeclareFunctionDeclaration(_)
| AnyJsBindingDeclaration::JsClassExpression(_)
| AnyJsBindingDeclaration::JsFunctionExpression(_) => None,

None
}
_ => None,
// Some parameters are ok to not be used
AnyJsBindingDeclaration::TsPropertyParameter(parameter) => {
let is_binding_ok =
is_function_that_is_ok_parameter_not_be_used(parameter.parent_function())
|| is_property_parameter_ok_not_be_used(parameter)?;
if !is_binding_ok {
suggestion_for_binding(binding)
} else {
None
}
}
// example: [key: string]: string;
JsSyntaxKind::TS_INDEX_SIGNATURE_PARAMETER => Some(()),
// example: declare function notUsedParameters(a);
JsSyntaxKind::TS_DECLARE_FUNCTION_DECLARATION => Some(()),
_ => {
// Anything below a declare
parent
.ancestors()
.any(|x| TsDeclareStatement::can_cast(x.kind()))
.then_some(())
AnyJsBindingDeclaration::JsFormalParameter(parameter) => {
let is_binding_ok =
is_function_that_is_ok_parameter_not_be_used(parameter.parent_function());
if !is_binding_ok {
suggestion_for_binding(binding)
} else {
None
}
}
AnyJsBindingDeclaration::JsRestParameter(parameter) => {
let is_binding_ok =
is_function_that_is_ok_parameter_not_be_used(parameter.parent_function());
if !is_binding_ok {
suggestion_for_binding(binding)
} else {
None
}
}

// declarations need to be check if they are under `declare`
node @ AnyJsBindingDeclaration::JsVariableDeclarator(_) => {
let is_binding_ok = is_is_ambient_context(&node.syntax().clone());
if !is_binding_ok {
suggestion_for_binding(binding)
} else {
None
}
}
node @ AnyJsBindingDeclaration::TsTypeAliasDeclaration(_)
| node @ AnyJsBindingDeclaration::JsClassDeclaration(_)
| node @ AnyJsBindingDeclaration::JsFunctionDeclaration(_)
| node @ AnyJsBindingDeclaration::TsInterfaceDeclaration(_)
| node @ AnyJsBindingDeclaration::TsEnumDeclaration(_)
| node @ AnyJsBindingDeclaration::TsModuleDeclaration(_)
| node @ AnyJsBindingDeclaration::TsImportEqualsDeclaration(_) => {
if is_is_ambient_context(&node.syntax().clone()) {
None
} else {
Some(SuggestedFix::NoSuggestion)
}
}

// Bindings under unknown parameter are never ok to be unused
AnyJsBindingDeclaration::JsBogusParameter(_) => Some(SuggestedFix::NoSuggestion),

// Bindings under catch are never ok to be unused
AnyJsBindingDeclaration::JsCatchDeclaration(_) => Some(SuggestedFix::PrefixUnderscore),

// Imports are never ok to be unused
AnyJsBindingDeclaration::JsImportDefaultClause(_)
| AnyJsBindingDeclaration::JsImportNamespaceClause(_)
| AnyJsBindingDeclaration::JsShorthandNamedImportSpecifier(_)
| AnyJsBindingDeclaration::JsNamedImportSpecifier(_)
| AnyJsBindingDeclaration::JsBogusNamedImportSpecifier(_)
| AnyJsBindingDeclaration::JsDefaultImportSpecifier(_)
| AnyJsBindingDeclaration::JsNamespaceImportSpecifier(_) => {
Some(SuggestedFix::NoSuggestion)
}

// exports with binding are ok to be unused
AnyJsBindingDeclaration::JsClassExportDefaultDeclaration(_)
| AnyJsBindingDeclaration::JsFunctionExportDefaultDeclaration(_)
| AnyJsBindingDeclaration::TsDeclareFunctionExportDefaultDeclaration(_) => {
Some(SuggestedFix::NoSuggestion)
}
}
}

impl Rule for NoUnusedVariables {
type Query = Semantic<JsIdentifierBinding>;
type State = ();
type Query = Semantic<AnyJsIdentifierBinding>;
type State = SuggestedFix;
type Signals = Option<Self::State>;
type Options = ();

fn run(ctx: &RuleContext<Self>) -> Option<Self::State> {
let binding = ctx.query();
let name = binding.name_token().ok()?;

let name = match binding {
AnyJsIdentifierBinding::JsIdentifierBinding(binding) => binding.name_token().ok()?,
AnyJsIdentifierBinding::TsIdentifierBinding(binding) => binding.name_token().ok()?,
};

let name = name.token_text_trimmed();
let name = name.text();

Expand All @@ -199,9 +282,9 @@ impl Rule for NoUnusedVariables {
return None;
}

if is_typescript_unused_ok(binding).is_some() {
let Some(suggestion) = suggested_fix_if_unused(binding) else {
return None;
}
};

let model = ctx.model();
if model.is_exported(binding) {
Expand All @@ -210,7 +293,7 @@ impl Rule for NoUnusedVariables {

let all_references = binding.all_references(model);
if all_references.count() == 0 {
Some(())
Some(suggestion)
} else {
// We need to check if all uses of this binding are somehow recursive

Expand Down Expand Up @@ -247,7 +330,7 @@ impl Rule for NoUnusedVariables {
}

if references_outside == 0 {
Some(())
Some(suggestion)
} else {
None
}
Expand Down Expand Up @@ -281,23 +364,35 @@ impl Rule for NoUnusedVariables {
Some(diag)
}

fn action(ctx: &RuleContext<Self>, _: &Self::State) -> Option<JsRuleAction> {
let node = ctx.query();
let mut mutation = ctx.root().begin();
let name = node.name_token().ok()?;
let name_trimmed = name.text_trimmed();
let new_text_trimmed = format!("_{}", name_trimmed);
mutation.replace_node(
node.clone(),
js_identifier_binding(ident(&new_text_trimmed)),
);
fn action(ctx: &RuleContext<Self>, suggestion: &Self::State) -> Option<JsRuleAction> {
match suggestion {
SuggestedFix::NoSuggestion => None,
SuggestedFix::PrefixUnderscore => {
let binding = ctx.query();
let mut mutation = ctx.root().begin();

let name = match binding {
AnyJsIdentifierBinding::JsIdentifierBinding(binding) => {
binding.name_token().ok()?
}
AnyJsIdentifierBinding::TsIdentifierBinding(binding) => {
binding.name_token().ok()?
}
};
let name_trimmed = name.text_trimmed();
let new_name = format!("_{}", name_trimmed);

Some(JsRuleAction {
mutation,
category: ActionCategory::QuickFix,
applicability: Applicability::MaybeIncorrect,
message: markup! { "If this is intentional, prepend "<Emphasis>{name_trimmed}</Emphasis>" with an underscore." }
.to_owned(),
})
let model = ctx.model();
mutation.rename_node_declaration(model, binding.clone(), &new_name);

Some(JsRuleAction {
mutation,
category: ActionCategory::QuickFix,
applicability: Applicability::MaybeIncorrect,
message: markup! { "If this is intentional, prepend "<Emphasis>{name_trimmed}</Emphasis>" with an underscore." }
.to_owned(),
})
}
}
}
}
11 changes: 9 additions & 2 deletions crates/rome_js_analyze/src/utils/rename.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use rome_js_semantic::{ReferencesExtensions, SemanticModel};
use rome_js_syntax::{
JsIdentifierAssignment, JsIdentifierBinding, JsLanguage, JsReferenceIdentifier, JsSyntaxKind,
JsSyntaxNode, JsSyntaxToken,
binding_ext::AnyJsIdentifierBinding, JsIdentifierAssignment, JsIdentifierBinding, JsLanguage,
JsReferenceIdentifier, JsSyntaxKind, JsSyntaxNode, JsSyntaxToken,
};
use rome_rowan::{AstNode, BatchMutation, SyntaxNodeCast, TriviaPiece};
use serde::{Deserialize, Serialize};
Expand All @@ -28,6 +28,13 @@ impl RenamableNode for JsIdentifierAssignment {
}
}

impl RenamableNode for AnyJsIdentifierBinding {
fn binding(&self, _: &SemanticModel) -> Option<JsSyntaxNode> {
let node = self.syntax();
Some(node.clone())
}
}

pub enum AnyJsRenamableDeclaration {
JsIdentifierBinding(JsIdentifierBinding),
JsReferenceIdentifier(JsReferenceIdentifier),
Expand Down
Loading
点击 这是indexloc提供的php浏览器服务，不要输入任何密码和下载