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

fix(rome_js_analyze): fix a false positive for noDuplicateCase #4709

Merged
merged 1 commit into from
Jul 19, 2023
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,8 @@ if no error diagnostics are emitted.

- Fix [`noInvalidConstructorSuper`](https://docs.rome.tools/lint/rules/noinvalidconstructorsuper/) rule that erroneously reported generic parents [#4624](https://github.com/rome/tools/issues/4624).

- Fix [`noDuplicateCase`](https://docs.rome.tools/lint/rules/noDuplicateCase/) rule that erroneously reported as equals the strings literals `"'"` and `'"'` [#4706](https://github.com/rome/tools/issues/4706).

- Relax [`noBannedTypes`](https://docs.rome.tools/lint/rules/nobannedtypes/) and improve documentation

The rule no longer reports a user type that reuses a banned type name.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@ use rome_analyze::{declare_rule, ActionCategory, Ast, Rule, RuleDiagnostic};
use rome_console::markup;
use rome_diagnostics::Applicability;
use rome_js_factory::make;
use rome_js_syntax::{AnyJsExpression, AnyJsLiteralExpression, JsSyntaxKind, TsEnumMember};
use rome_js_syntax::{
inner_text, AnyJsExpression, AnyJsLiteralExpression, JsSyntaxKind, TsEnumMember,
};
use rome_rowan::{AstNode, BatchMutationExt, Direction};

declare_rule! {
Expand Down Expand Up @@ -118,8 +120,7 @@ impl Rule for UseEnumInitializers {
}
AnyJsLiteralExpression::JsStringLiteralExpression(expr) => {
let prev_enum_delim_val = expr.value_token().ok()?;
let prev_enum_delim_val = prev_enum_delim_val.text();
let prev_enum_val = &prev_enum_delim_val[1..(prev_enum_delim_val.len() - 1)];
let prev_enum_val = inner_text(&prev_enum_delim_val);
if prev_name.text() == prev_enum_val {
let enum_name = enum_member.name().ok()?.text();
Some(AnyJsLiteralExpression::JsStringLiteralExpression(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ use rome_deserialize::{
use rome_diagnostics::Applicability;
use rome_js_semantic::CanBeImportedExported;
use rome_js_syntax::{
binding_ext::AnyJsBindingDeclaration, AnyJsClassMember, AnyJsObjectMember,
binding_ext::AnyJsBindingDeclaration, inner_text, AnyJsClassMember, AnyJsObjectMember,
AnyJsVariableDeclaration, AnyTsTypeMember, JsIdentifierBinding, JsLiteralExportName,
JsLiteralMemberName, JsPrivateClassMemberName, JsSyntaxKind, JsSyntaxToken,
JsVariableDeclarator, JsVariableKind, TsEnumMember, TsIdentifierBinding, TsTypeParameterName,
Expand Down Expand Up @@ -290,10 +290,7 @@ impl Rule for UseNamingConvention {
return None;
}
let name_token = node.name_token().ok()?;
let mut name = name_token.text_trimmed();
if name_token.kind() == JsSyntaxKind::JS_STRING_LITERAL {
name = &name[1..name.len() - 1];
}
let name = inner_text(&name_token);
if !is_js_ident(name) {
// ignore non-identifier strings
return None;
Expand Down Expand Up @@ -323,10 +320,7 @@ impl Rule for UseNamingConvention {
suggested_name,
} = state;
let name_token = ctx.query().name_token().ok()?;
let mut name = name_token.text_trimmed();
if name_token.kind() == JsSyntaxKind::JS_STRING_LITERAL {
name = &name[1..name.len() - 1];
}
let name = inner_text(&name_token);
let trimmed_name = trim_underscore_dollar(name);
let allowed_cases = element.allowed_cases(ctx.options());
let allowed_case_names = allowed_cases
Expand Down
16 changes: 2 additions & 14 deletions crates/rome_js_analyze/src/utils.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use rome_js_factory::make;
use rome_js_syntax::{
AnyJsStatement, JsLanguage, JsModuleItemList, JsStatementList, JsSyntaxNode, JsSyntaxToken,
inner_text, AnyJsStatement, JsLanguage, JsModuleItemList, JsStatementList, JsSyntaxNode,
JsVariableDeclaration, JsVariableDeclarator, JsVariableDeclaratorList, JsVariableStatement, T,
};
use rome_rowan::{AstNode, AstSeparatedList, BatchMutation, Direction, WalkEvent};
Expand Down Expand Up @@ -225,7 +225,7 @@ pub(crate) fn is_node_equal(a_node: &JsSyntaxNode, b_node: &JsSyntaxNode) -> boo
(None, Some(_)) | (Some(_), None) => return false,
// both are tokens
(Some(a), Some(b)) => {
if !is_token_text_equal(a, b) {
if inner_text(a) != inner_text(b) {
return false;
}
continue;
Expand All @@ -236,18 +236,6 @@ pub(crate) fn is_node_equal(a_node: &JsSyntaxNode, b_node: &JsSyntaxNode) -> boo
true
}

/// Verify that tokens' inner text are equal
fn is_token_text_equal(a: &JsSyntaxToken, b: &JsSyntaxToken) -> bool {
static QUOTES: [char; 2] = ['"', '\''];

a.token_text_trimmed()
.trim_start_matches(QUOTES)
.trim_end_matches(QUOTES)
== b.token_text_trimmed()
.trim_start_matches(QUOTES)
.trim_end_matches(QUOTES)
}

#[test]
fn ok_to_camel_case() {
assert_eq!(to_camel_case("camelCase"), Cow::Borrowed("camelCase"));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -113,3 +113,9 @@ switch (a) {
case toString:
break;
}
switch (a) {
case "'":
return ''';
case '"':
return '"';
}
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,12 @@ switch (a) {
case toString:
break;
}

switch (a) {
case "'":
return ''';
case '"':
return '"';
}
```


27 changes: 27 additions & 0 deletions crates/rome_js_syntax/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -272,3 +272,30 @@ impl OperatorPrecedence {
matches!(self, OperatorPrecedence::Exponential)
}
}

/// Similar to `JsSyntaxToken::text_trimmed` with the difference that delimiters of string literals are trimmed.
///
/// ## Examples
///
/// ```
/// use rome_js_syntax::{JsSyntaxKind, JsSyntaxToken, inner_text};
///
/// let a = JsSyntaxToken::new_detached(JsSyntaxKind::JS_STRING_LITERAL, "'inner_text'", [], []);
/// let b = JsSyntaxToken::new_detached(JsSyntaxKind::JS_STRING_LITERAL, "\"inner_text\"", [], []);
/// assert_eq!(inner_text(&a), inner_text(&b));
///
/// let a = JsSyntaxToken::new_detached(JsSyntaxKind::LET_KW, "let", [], []);
/// let b = JsSyntaxToken::new_detached(JsSyntaxKind::LET_KW, "let", [], []);
/// assert_eq!(inner_text(&a), inner_text(&b));
///
/// let a = JsSyntaxToken::new_detached(JsSyntaxKind::LET_KW, "let", [], []);
/// let b = JsSyntaxToken::new_detached(JsSyntaxKind::CONST_KW, "const", [], []);
/// assert!(inner_text(&a) != inner_text(&b));
/// ```
pub fn inner_text(token: &JsSyntaxToken) -> &str {
Copy link
Contributor

Choose a reason for hiding this comment

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

Two questions:

  • we have some inner_text functions already, weren't they sufficient?
  • why did you implement it as a function not inside as impl for syntax tokens?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is JS-spêcific, while SyntaxToken is more general. It is not possible to write an impl on a type-alias such as JsSyntaxToken?

Which inner_text do you refer to?

Copy link
Contributor

@ematipico ematipico Jul 19, 2023

Choose a reason for hiding this comment

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

We have a bunch of them: https://github.com/search?q=repo%3Arome%2Ftools+%22inner_string_text%22&type=code

So maybe there is some overlap, and if there isn't we should choose e better name to avoid confusion. Their names are very similar

let mut result = token.text_trimmed();
if token.kind() == JsSyntaxKind::JS_STRING_LITERAL {
result = &result[1..result.len() - 1];
}
result
}
Loading