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

feat(rome_js_analyze): no unused starting with underscore; small hack for React and removing suggested fix #3124

Merged
merged 2 commits into from Aug 29, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -1,16 +1,13 @@
use crate::{semantic_services::Semantic, utils::batch::JsBatchMutation, JsRuleAction};
use rome_analyze::{
context::RuleContext, declare_rule, ActionCategory, Rule, RuleCategory, RuleDiagnostic,
};
use crate::semantic_services::Semantic;
use rome_analyze::{context::RuleContext, declare_rule, Rule, RuleCategory, RuleDiagnostic};
use rome_console::markup;
use rome_diagnostics::Applicability;
use rome_js_semantic::{AllReferencesExtensions, SemanticScopeExtensions};
use rome_js_syntax::{
JsCatchDeclaration, JsConstructorParameterList, JsConstructorParameters, JsFormalParameter,
JsFunctionDeclaration, JsIdentifierBinding, JsParameterList, JsParameters, JsSyntaxKind,
JsClassExpression, JsConstructorParameterList, JsConstructorParameters, JsFunctionDeclaration,
JsFunctionExpression, JsIdentifierBinding, JsParameterList, JsParameters, JsSyntaxKind,
JsVariableDeclarator,
};
use rome_rowan::{AstNode, BatchMutationExt};
use rome_rowan::AstNode;

declare_rule! {
/// Disallow unused variables.
Expand Down Expand Up @@ -65,6 +62,20 @@ declare_rule! {
/// };
/// foo();
/// ```
///
/// ```js
/// function foo(_unused) {
/// };
/// foo();
/// ```
///
/// ```jsx
/// import React from 'react';
/// function foo() {
/// return <div />;
/// };
/// foo();
/// ```
pub(crate) NoUnusedVariables {
version: "0.9.0",
name: "noUnusedVariables",
Expand Down Expand Up @@ -117,12 +128,28 @@ impl Rule for NoUnusedVariables {

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

// Old code import React but do not used directly
// only indirectly after transpiling JSX.
if name.starts_with('_') || name == "React" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The check for React should only apply to JSX files, although at the moment I don't think we have access to the SourceType for the file in the RuleContext

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To add more, we were discussing about implementing JSX parsing to JS files. If that happens in the future, we would need access to that information too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, we don´t.
It is a little bit hacky now. I expect this to be turned into an option in the future.

return None;
}

// Ignore expressions
if binding.parent::<JsFunctionExpression>().is_some()
|| binding.parent::<JsClassExpression>().is_some()
{
return None;
}

if is_typescript_unused_ok(binding).is_some() {
return None;
}

let model = ctx.model();
if model.is_exported(binding) {
return None;
}
Expand Down Expand Up @@ -198,43 +225,4 @@ impl Rule for NoUnusedVariables {

Some(diag)
}

fn action(ctx: &RuleContext<Self>, _: &Self::State) -> Option<JsRuleAction> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could still keep the code action for variables, but instead of removing the whole declaration it could suggest to prefix the name with an underscore ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to remove it for now. And we can return it after it is a little bit more mature.

let root = ctx.root();
let binding = ctx.query();

let mut batch = root.begin();

// Try to remove node
let removed = if let Some(declaration) = binding.parent::<JsFunctionDeclaration>() {
batch.remove_node(declaration);
true
} else if let Some(variable_declarator) = binding.parent::<JsVariableDeclarator>() {
batch.remove_js_variable_declarator(&variable_declarator)
} else if let Some(formal_parameter) = binding.parent::<JsFormalParameter>() {
batch.remove_js_formal_parameter(&formal_parameter)
} else if let Some(catch_declaration) = binding.parent::<JsCatchDeclaration>() {
batch.remove_node(catch_declaration);
true
} else {
false
};

if removed {
let symbol_type = match binding.syntax().parent().unwrap().kind() {
JsSyntaxKind::JS_FORMAL_PARAMETER => "parameter",
JsSyntaxKind::JS_FUNCTION_DECLARATION => "function",
_ => "variable",
};

Some(JsRuleAction {
category: ActionCategory::QuickFix,
applicability: Applicability::MaybeIncorrect,
message: markup! { "Remove this " {symbol_type} "." }.to_owned(),
mutation: batch,
})
} else {
None
}
}
}
@@ -1,3 +1,5 @@
import React from 'react';

const a = 1;
const b = 2,
c = 3;
Expand Down Expand Up @@ -47,4 +49,8 @@ let value;
function Button() {}
console.log(<Button att={value}/>);

(function f(){})()
(function f(_a){})()

new (class C {

})