Skip to content

Commit

Permalink
fix(semantic_analyzers): style/noShoutyConstants does not recognize m…
Browse files Browse the repository at this point in the history 
…ultiple uses of a constant. (rome#3789)

Co-authored-by: Anchen Li <[email protected]>
  • Loading branch information
@mzbac
mzbac and Anchen Li authored Nov 24, 2022
1 parent 732a32f commit f1df39c
Show file tree
Hide file tree
Showing 3 changed files with 163 additions and 66 deletions.
62 changes: 46 additions & 16 deletions crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,15 @@ use crate::{semantic_services::Semantic, utils::batch::JsBatchMutation, JsRuleAc
use rome_analyze::{context::RuleContext, declare_rule, ActionCategory, Rule, RuleDiagnostic};
use rome_console::markup;
use rome_diagnostics::Applicability;
use rome_js_factory::make::{js_literal_member_name, js_property_object_member};
use rome_js_semantic::{Reference, ReferencesExtensions};
use rome_js_syntax::{
JsAnyExpression, JsAnyLiteralExpression, JsIdentifierBinding, JsIdentifierExpression,
JsStringLiteralExpression, JsVariableDeclaration, JsVariableDeclarator,
JsAnyExpression, JsAnyLiteralExpression, JsAnyObjectMemberName, JsIdentifierBinding,
JsIdentifierExpression, JsReferenceIdentifier, JsShorthandPropertyObjectMember,
JsStringLiteralExpression, JsSyntaxKind, JsVariableDeclaration, JsVariableDeclarator,
JsVariableDeclaratorList,
};
use rome_rowan::{AstNode, BatchMutationExt, SyntaxNodeCast};
use rome_rowan::{AstNode, BatchMutationExt, SyntaxNodeCast, SyntaxToken};

declare_rule! {
/// Disallow the use of constants which its value is the upper-case version of its name.
Expand Down Expand Up @@ -78,7 +80,7 @@ fn is_id_and_string_literal_inner_text_equal(

pub struct State {
literal: JsStringLiteralExpression,
references: Vec<Reference>,
reference: Reference,
}

impl Rule for NoShoutyConstants {
Expand All @@ -101,9 +103,13 @@ impl Rule for NoShoutyConstants {
return None;
}

if binding.all_references(model).count() > 1 {
return None;
}

return Some(State {
literal,
references: binding.all_references(model).collect(),
reference: binding.all_references(model).next()?,
});
}
}
Expand All @@ -122,10 +128,8 @@ impl Rule for NoShoutyConstants {
},
);

for reference in state.references.iter() {
let node = reference.syntax();
diag = diag.detail(node.text_trimmed_range(), "Used here.")
}
let node = state.reference.syntax();
diag = diag.detail(node.text_trimmed_range(), "Used here.");

let diag = diag.note(
markup! {"You should avoid declaring constants with a string that's the same
Expand All @@ -144,16 +148,42 @@ impl Rule for NoShoutyConstants {

batch.remove_js_variable_declarator(ctx.query());

for reference in state.references.iter() {
let node = reference
.syntax()
.parent()?
.cast::<JsIdentifierExpression>()?;

if let Some(node) = state
.reference
.syntax()
.parent()?
.cast::<JsIdentifierExpression>()
{
batch.replace_node(
JsAnyExpression::JsIdentifierExpression(node),
JsAnyExpression::JsAnyLiteralExpression(literal.clone()),
JsAnyExpression::JsAnyLiteralExpression(literal),
);
} else if let Some(node) = state
.reference
.syntax()
.parent()?
.cast::<JsShorthandPropertyObjectMember>()
{
// for replacing JsShorthandPropertyObjectMember
let new_element = js_property_object_member(
JsAnyObjectMemberName::JsLiteralMemberName(js_literal_member_name(
SyntaxToken::new_detached(
JsSyntaxKind::JS_LITERAL_MEMBER_NAME,
JsReferenceIdentifier::cast_ref(state.reference.syntax())?
.value_token()
.ok()?
.text(),
[],
[],
),
)),
SyntaxToken::new_detached(JsSyntaxKind::COLON, ":", [], []),
JsAnyExpression::JsAnyLiteralExpression(literal),
);

batch.replace_element(node.into_syntax().into(), new_element.into_syntax().into());
} else {
return None;
}

Some(JsRuleAction {
Expand Down
22 changes: 20 additions & 2 deletions crates/rome_js_analyze/tests/specs/style/noShoutyConstants.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,24 @@ const FOO2 = "FOO2", a = "FOO3", FOO4 = "FOO4";
console.log(FOO, FOO4);

let foo = "foo";
const B = "B";
export default B;
export const A = "A";

const A = "A";
export default A;
const BAR = "BAR";

export const bar = {
foo: BAR,
bar: BAR,
};

const C = "C";

export const d = {
foo: C
};

const D ="D";
export const e = {
D
};
145 changes: 97 additions & 48 deletions crates/rome_js_analyze/tests/specs/style/noShoutyConstants.js.snap
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
---
source: crates/rome_js_analyze/tests/spec_tests.rs
assertion_line: 73
expression: noShoutyConstants.js
---
# Input
Expand All @@ -12,60 +13,30 @@ const FOO2 = "FOO2", a = "FOO3", FOO4 = "FOO4";
console.log(FOO, FOO4);

let foo = "foo";
const B = "B";
export default B;
export const A = "A";

const A = "A";
export default A;
```
const BAR = "BAR";

# Diagnostics
```
noShoutyConstants.js:1:7 lint/style/noShoutyConstants FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
export const bar = {
foo: BAR,
bar: BAR,
};

! Redundant constant declaration.
> 1 │ const FOO = "FOO";
│ ^^^^^^^^^^^
2 │ console.log(FOO, FOO2);
3 │
i Used here.
1 │ const FOO = "FOO";
> 2 │ console.log(FOO, FOO2);
│ ^^^
3 │
4 │ const FOO2 = "FOO2", a = "FOO3", FOO4 = "FOO4";
i Used here.
4 │ const FOO2 = "FOO2", a = "FOO3", FOO4 = "FOO4";
5 │
> 6 │ console.log(FOO, FOO4);
│ ^^^
7 │
8 │ let foo = "foo";
i You should avoid declaring constants with a string that's the same
value as the variable name. It introduces a level of unnecessary
indirection when it's only two additional characters to inline.
i Suggested fix: Use the constant value directly
1 │ - const·FOO·=·"FOO";
2 │ - console.log(FOO,·FOO2);
1 │ +
2 │ + console.log("FOO",·FOO2);
3 3 │
4 4 │ const FOO2 = "FOO2", a = "FOO3", FOO4 = "FOO4";
5 5 │
6 │ - console.log(FOO,·FOO4);
6 │ + console.log("FOO",·FOO4);
7 7 │
8 8 │ let foo = "foo";
const C = "C";

export const d = {
foo: C
};

const D ="D";
export const e = {
D
};
```

# Diagnostics
```
noShoutyConstants.js:4:7 lint/style/noShoutyConstants FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
Expand Down Expand Up @@ -144,4 +115,82 @@ noShoutyConstants.js:4:34 lint/style/noShoutyConstants FIXABLE ━━━━━
```

```
noShoutyConstants.js:20:7 lint/style/noShoutyConstants FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
! Redundant constant declaration.
18 │ };
19 │
> 20 │ const C = "C";
│ ^^^^^^^
21 │
22 │ export const d = {
i Used here.
22 │ export const d = {
> 23 │ foo: C
│ ^
24 │ };
25 │
i You should avoid declaring constants with a string that's the same
value as the variable name. It introduces a level of unnecessary
indirection when it's only two additional characters to inline.
i Suggested fix: Use the constant value directly
18 18 │ };
19 19 │
20 │ - const·C·=·"C";
21 │ -
22 20 │ export const d = {
23 │ - → foo:·C
21 │ + → foo:·"C"
24 22 │ };
25 23 │
```

```
noShoutyConstants.js:26:7 lint/style/noShoutyConstants FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
! Redundant constant declaration.
24 │ };
25 │
> 26 │ const D ="D";
│ ^^^^^^
27 │ export const e = {
28 │ D
i Used here.
26 │ const D ="D";
27 │ export const e = {
> 28 │ D
│ ^
29 │ };
i You should avoid declaring constants with a string that's the same
value as the variable name. It introduces a level of unnecessary
indirection when it's only two additional characters to inline.
i Suggested fix: Use the constant value directly
23 23 │ foo: C
24 24 │ };
25 │ -
26 │ - const·D·="D";
27 25 │ export const e = {
28 │ - → D
26 │ + →
27 │ + → D:"D"
29 28 │ };
```


0 comments on commit f1df39c

Please sign in to comment.