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.

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(linter): fix no_dupe_keys false postive on similar key names #2291

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
1 change: 0 additions & 1 deletion crates/oxc_ast/src/ast/js.rs
Original file line number Diff line number Diff line change
Expand Up @@ -445,7 +445,6 @@ pub enum PropertyKey<'a> {
}

impl<'a> PropertyKey<'a> {
// FIXME: this would ideally return Option<&'a Atom> or a Cow
pub fn static_name(&self) -> Option<Atom> {
match self {
Self::Identifier(ident) => Some(ident.name.clone()),
Expand Down
24 changes: 1 addition & 23 deletions crates/oxc_linter/src/ast_util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use std::hash::{Hash, Hasher};

use oxc_ast::AstKind;
use oxc_semantic::AstNode;
use oxc_span::{Atom, GetSpan, Span};
use oxc_span::{GetSpan, Span};
use oxc_syntax::operator::{AssignmentOperator, BinaryOperator, LogicalOperator, UnaryOperator};
use rustc_hash::FxHasher;

Expand Down Expand Up @@ -264,28 +264,6 @@ pub fn outermost_paren_parent<'a, 'b>(
ctx.nodes().parent_node(node.id())
}

pub fn get_name_from_property_key(key: &PropertyKey<'_>) -> Option<Atom> {
match key {
PropertyKey::Identifier(ident) => Some(ident.name.clone()),
PropertyKey::PrivateIdentifier(ident) => {
let name = ident.name.clone();

Some(Atom::from(format!("#{name}")))
}
PropertyKey::Expression(expr) => match expr {
Expression::StringLiteral(lit) => Some(lit.value.clone()),
Expression::RegExpLiteral(lit) => Some(Atom::from(format!("{0}", lit.regex))),
Expression::NumberLiteral(lit) => Some(Atom::from(lit.raw)),
Expression::BigintLiteral(lit) => Some(Atom::from(format!("{0}", lit.value))),
Expression::NullLiteral(_) => Some("null".into()),
Expression::TemplateLiteral(lit) => {
lit.expressions.is_empty().then(|| lit.quasi()).flatten().cloned()
}
_ => None,
},
}
}

pub fn get_declaration_of_variable<'a, 'b>(
ident: &IdentifierReference,
ctx: &'b LintContext<'a>,
Expand Down
36 changes: 4 additions & 32 deletions crates/oxc_linter/src/rules/eslint/no_dupe_keys.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
use lazy_static::lazy_static;
use oxc_ast::{
ast::{Expression, ObjectPropertyKind, PropertyKey, PropertyKind},
ast::{ObjectPropertyKind, PropertyKind},
AstKind,
};
use oxc_diagnostics::{
Expand All @@ -11,7 +10,7 @@ use oxc_macros::declare_oxc_lint;
use oxc_span::{GetSpan, Span};
use rustc_hash::FxHashMap;

use crate::{ast_util::calculate_hash, context::LintContext, rule::Rule, AstNode};
use crate::{context::LintContext, rule::Rule, AstNode};

#[derive(Debug, Error, Diagnostic)]
#[error("eslint(no-dupe-keys): Disallow duplicate keys in object literals")]
Expand Down Expand Up @@ -47,8 +46,8 @@ impl Rule for NoDupeKeys {
let mut map = FxHashMap::default();
for prop in &obj_expr.properties {
let ObjectPropertyKind::ObjectProperty(prop) = prop else { continue };
let Some(hash) = calculate_property_kind_hash(&prop.key) else { continue };
if let Some((prev_kind, prev_span)) = map.insert(hash, (prop.kind, prop.key.span())) {
let Some(name) = prop.key.static_name() else { return };
if let Some((prev_kind, prev_span)) = map.insert(name, (prop.kind, prop.key.span())) {
if prev_kind == PropertyKind::Init
|| prop.kind == PropertyKind::Init
|| prev_kind == prop.kind
Expand All @@ -60,33 +59,6 @@ impl Rule for NoDupeKeys {
}
}

// todo: should this be located within oxc_ast?
fn calculate_property_kind_hash(key: &PropertyKey) -> Option<u64> {
lazy_static! {
static ref NULL_HASH: u64 = calculate_hash(&"null");
}

match key {
PropertyKey::Identifier(ident) => Some(calculate_hash(&ident)),
PropertyKey::PrivateIdentifier(_) => None,
PropertyKey::Expression(expr) => match expr {
Expression::StringLiteral(lit) => Some(calculate_hash(&lit.value)),
// note: hashes won't work as expected if these aren't strings. Save
// NumberLiteral I don't think this should be too much of a problem
// b/c most people don't use `null`, regexes, etc. as object
// property keys when writing real code.
Expression::RegExpLiteral(lit) => Some(calculate_hash(&lit.regex.to_string())),
Expression::NumberLiteral(lit) => Some(calculate_hash(&lit.value.to_string())),
Expression::BigintLiteral(lit) => Some(calculate_hash(&lit.value.to_string())),
Expression::NullLiteral(_) => Some(*NULL_HASH),
Expression::TemplateLiteral(lit) => {
lit.expressions.is_empty().then(|| lit.quasi()).flatten().map(calculate_hash)
}
_ => None,
},
}
}

#[test]
fn test() {
use crate::tester::Tester;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use oxc_diagnostics::{
use oxc_macros::declare_oxc_lint;
use oxc_span::{Atom, GetSpan, Span};

use crate::{ast_util::get_name_from_property_key, context::LintContext, rule::Rule, AstNode};
use crate::{context::LintContext, rule::Rule, AstNode};

#[derive(Debug, Error, Diagnostic)]
#[error(
Expand Down Expand Up @@ -125,15 +125,13 @@ trait GetMethod {
impl GetMethod for ClassElement<'_> {
fn get_method(&self) -> Option<Method> {
match self {
ClassElement::MethodDefinition(def) => {
get_name_from_property_key(&def.key).map(|name| Method {
name,
r#static: def.r#static,
call_signature: false,
kind: get_kind_from_key(&def.key),
span: Span::new(def.span.start, def.key.span().end),
})
}
ClassElement::MethodDefinition(def) => def.key.static_name().map(|name| Method {
name,
r#static: def.r#static,
call_signature: false,
kind: get_kind_from_key(&def.key),
span: Span::new(def.span.start, def.key.span().end),
}),
_ => None,
}
}
Expand All @@ -142,15 +140,13 @@ impl GetMethod for ClassElement<'_> {
impl GetMethod for TSSignature<'_> {
fn get_method(&self) -> Option<Method> {
match self {
TSSignature::TSMethodSignature(sig) => {
get_name_from_property_key(&sig.key).map(|name| Method {
name,
r#static: false,
call_signature: false,
kind: get_kind_from_key(&sig.key),
span: sig.key.span(),
})
}
TSSignature::TSMethodSignature(sig) => sig.key.static_name().map(|name| Method {
name,
r#static: false,
call_signature: false,
kind: get_kind_from_key(&sig.key),
span: sig.key.span(),
}),
TSSignature::TSCallSignatureDeclaration(sig) => Some(Method {
name: Atom::from("call"),
r#static: false,
Expand Down
11 changes: 0 additions & 11 deletions crates/oxc_linter/src/snapshots/adjacent_overload_signatures.snap
Original file line number Diff line number Diff line change
Expand Up @@ -359,17 +359,6 @@ expression: adjacent_overload_signatures
5 │ static foo(sn: string | number): void {}
╰────

⚠ typescript-eslint(adjacent-overload-signatures): All "#private" signatures should be adjacent.
╭─[adjacent_overload_signatures.tsx:2:9]
1 │ class Test {
2 │ #private(): void;
· ────────
3 │ '#private'(): void;
4 │ #private(arg: number): void {}
· ────────
5 │ '#private'(arg: number): void {}
╰────

⚠ typescript-eslint(adjacent-overload-signatures): All "#private" signatures should be adjacent.
╭─[adjacent_overload_signatures.tsx:3:9]
2 │ #private(): void;
Expand Down