From 1bf5c6ac3911c4503309e7dcfe4f836ba07e640c Mon Sep 17 00:00:00 2001 From: Tim Seckinger Date: Thu, 3 Nov 2022 21:00:26 +0100 Subject: [PATCH] WIP feat(rome_js_analyze): lint/correctness/noDupeKeys --- .../src/categories.rs | 9 +- .../rome_js_analyze/src/analyzers/nursery.rs | 3 +- .../src/analyzers/nursery/no_dupe_keys.rs | 254 ++++++++++++++++++ .../tests/specs/nursery/noDupeKeys.js | 36 +++ .../src/configuration/linter/rules.rs | 4 +- editors/vscode/configuration_schema.json | 10 + npm/backend-jsonrpc/src/workspace.ts | 10 +- website/src/docs/lint/rules/index.md | 7 + website/src/docs/lint/rules/noDupeKeys.md | 96 +++++++ 9 files changed, 419 insertions(+), 10 deletions(-) create mode 100644 crates/rome_js_analyze/src/analyzers/nursery/no_dupe_keys.rs create mode 100644 crates/rome_js_analyze/tests/specs/nursery/noDupeKeys.js create mode 100644 website/src/docs/lint/rules/noDupeKeys.md diff --git a/crates/rome_diagnostics_categories/src/categories.rs b/crates/rome_diagnostics_categories/src/categories.rs index 1bd3b50c4f66..7d794d902013 100644 --- a/crates/rome_diagnostics_categories/src/categories.rs +++ b/crates/rome_diagnostics_categories/src/categories.rs @@ -75,14 +75,15 @@ define_dategories! { // nursery - "lint/nursery/useFlatMap": "https://rome.tools/docs/lint/rules/useFlatMap", + "lint/nursery/noBannedTypes":"https://rome.tools/docs/lint/rules/noBannedTypes", "lint/nursery/noConstAssign": "https://rome.tools/docs/lint/rules/noConstAssign", + "lint/nursery/noDupeKeys": "https://rome.tools/docs/lint/rules/noDupeKeys", "lint/nursery/noExplicitAny": "https://rome.tools/docs/lint/rules/noExplicitAny", - "lint/nursery/useValidForDirection": "https://rome.tools/docs/lint/rules/useValidForDirection", "lint/nursery/noInvalidConstructorSuper": "https://rome.tools/docs/lint/rules/noInvalidConstructorSuper", - "lint/nursery/useExhaustiveDependencies": "https://rome.tools/docs/lint/rules/useExhaustiveDependencies", "lint/nursery/useCamelCase": "https://rome.tools/docs/lint/rules/useCamelCase", - "lint/nursery/noBannedTypes":"https://rome.tools/docs/lint/rules/noBannedTypes", + "lint/nursery/useExhaustiveDependencies": "https://rome.tools/docs/lint/rules/useExhaustiveDependencies", + "lint/nursery/useFlatMap": "https://rome.tools/docs/lint/rules/useFlatMap", + "lint/nursery/useValidForDirection": "https://rome.tools/docs/lint/rules/useValidForDirection", ; diff --git a/crates/rome_js_analyze/src/analyzers/nursery.rs b/crates/rome_js_analyze/src/analyzers/nursery.rs index f74417a3e03e..5a117ca581b0 100644 --- a/crates/rome_js_analyze/src/analyzers/nursery.rs +++ b/crates/rome_js_analyze/src/analyzers/nursery.rs @@ -2,8 +2,9 @@ use rome_analyze::declare_group; mod no_banned_types; +mod no_dupe_keys; mod no_explicit_any; mod no_invalid_constructor_super; mod use_flat_map; mod use_valid_for_direction; -declare_group! { pub (crate) Nursery { name : "nursery" , rules : [self :: no_banned_types :: NoBannedTypes , self :: no_explicit_any :: NoExplicitAny , self :: no_invalid_constructor_super :: NoInvalidConstructorSuper , self :: use_flat_map :: UseFlatMap , self :: use_valid_for_direction :: UseValidForDirection ,] } } +declare_group! { pub (crate) Nursery { name : "nursery" , rules : [self :: no_banned_types :: NoBannedTypes , self :: no_dupe_keys :: NoDupeKeys , self :: no_explicit_any :: NoExplicitAny , self :: no_invalid_constructor_super :: NoInvalidConstructorSuper , self :: use_flat_map :: UseFlatMap , self :: use_valid_for_direction :: UseValidForDirection ,] } } diff --git a/crates/rome_js_analyze/src/analyzers/nursery/no_dupe_keys.rs b/crates/rome_js_analyze/src/analyzers/nursery/no_dupe_keys.rs new file mode 100644 index 000000000000..968a2a10529a --- /dev/null +++ b/crates/rome_js_analyze/src/analyzers/nursery/no_dupe_keys.rs @@ -0,0 +1,254 @@ +use rome_analyze::context::RuleContext; +use rome_analyze::{declare_rule, Ast, Rule, RuleDiagnostic}; +use rome_console::markup; +use rome_js_syntax::{JsAnyObjectMember, JsObjectExpression}; +use rome_rowan::{AstNode, BatchMutationExt}; +use std::collections::HashMap; +use std::fmt::Display; + +use crate::JsRuleAction; + +declare_rule! { + /// Prevents object literals having more than one property declaration for the same key. + /// If the same key is specified multiple times, only the last property declaration will prevail and previous will be lost, which is likely a mistake. + /// + /// ## Examples + /// + /// ### Invalid + /// + /// ```js,expect_diagnostic + /// const obj = { + /// a: 1, + /// a: 2, + /// } + /// ``` + /// + /// ```js,expect_diagnostic + /// const obj = { + /// set a(v) {}, + /// a: 2, + /// } + /// ``` + /// + /// ### Valid + /// + /// ```js + /// const obj = { + /// a: 1, + /// b: 2, + /// } + /// ``` + /// + /// ```js + /// const obj = { + /// get a() { return 1; }, + /// set a(v) {}, + /// } + /// ``` + /// + pub(crate) NoDupeKeys { + version: "10.0.0", + name: "noDupeKeys", + recommended: false, // should be once out of nursery + } +} + +enum PropertyType { + Getter, + Setter, + Value, +} +impl Display for PropertyType { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str(match self { + Self::Getter => "getter", + Self::Setter => "setter", + Self::Value => "value", + }) + } +} +struct PropertyDefinition(PropertyType, JsAnyObjectMember); + +#[derive(Clone)] +enum DefinedProperty { + Getter(JsAnyObjectMember), + Setter(JsAnyObjectMember), + GetterSetter(JsAnyObjectMember, JsAnyObjectMember), + Value(JsAnyObjectMember), +} +impl From for DefinedProperty { + fn from(PropertyDefinition(property_type, range): PropertyDefinition) -> Self { + match property_type { + PropertyType::Getter => DefinedProperty::Getter(range), + PropertyType::Setter => DefinedProperty::Setter(range), + PropertyType::Value => DefinedProperty::Value(range), + } + } +} + +pub(crate) struct PropertyConflict(DefinedProperty, PropertyDefinition); + +impl Rule for NoDupeKeys { + type Query = Ast; + type State = PropertyConflict; + type Signals = Vec; + type Options = (); + + fn run(ctx: &RuleContext) -> Self::Signals { + let node = ctx.query(); + + let mut defined_properties: HashMap = HashMap::new(); + let mut signals: Self::Signals = Vec::new(); + + for member in node + .members() + .into_iter() + // Note that we iterate from last to first property, so that we highlight properties being overwritten as problems and not those that take effect. + .rev() + .filter_map(|result| result.ok()) + { + let property_name = get_property_name(&member); + if let Some(property_name) = property_name { + let property_definition = PropertyDefinition( + match member { + JsAnyObjectMember::JsGetterObjectMember(_) => PropertyType::Getter, + JsAnyObjectMember::JsSetterObjectMember(_) => PropertyType::Setter, + _ => PropertyType::Value, + }, + member, + ); + let defined_property = defined_properties.remove(&property_name); + match (defined_property, property_definition) { + // Property not seen before + (None, property_definition) => { + // Put a new definition. + defined_properties + .insert(property_name, DefinedProperty::from(property_definition)); + } + // Only get/set counterpart seen before + ( + Some(DefinedProperty::Setter(set_range)), + PropertyDefinition(PropertyType::Getter, get_range), + ) + | ( + Some(DefinedProperty::Getter(get_range)), + PropertyDefinition(PropertyType::Setter, set_range), + ) => { + // Put definition back with the missing get or set filled. + defined_properties.insert( + property_name, + DefinedProperty::GetterSetter(get_range, set_range), + ); + } + // Trying to define something that was already defined + ( + Some(defined_property @ DefinedProperty::Getter(_)), + property_definition @ (PropertyDefinition(PropertyType::Getter, _) + | PropertyDefinition(PropertyType::Value, _)), + ) + | ( + Some(defined_property @ DefinedProperty::Setter(_)), + property_definition @ (PropertyDefinition(PropertyType::Setter, _) + | PropertyDefinition(PropertyType::Value, _)), + ) + | ( + Some( + defined_property @ (DefinedProperty::Value(_) + | DefinedProperty::GetterSetter(..)), + ), + property_definition @ _, + ) => { + // Register the conflict. + signals.push(PropertyConflict( + defined_property.clone(), + property_definition, + )); + // Put definition back unchanged. + defined_properties.insert(property_name, defined_property); + } + } + } + } + + signals + } + + fn diagnostic( + _ctx: &RuleContext, + PropertyConflict(defined_property, PropertyDefinition(property_type, node)): &Self::State, + ) -> Option { + let mut diagnostic = RuleDiagnostic::new( + rule_category!(), + node.range(), + format!( + "This property {} is later overwritten by a property with the same name.", + property_type + ), + ); + diagnostic = match defined_property { + DefinedProperty::Getter(node) => { + diagnostic.detail(node.range(), "Overwritten by this getter.") + } + DefinedProperty::Setter(node) => { + diagnostic.detail(node.range(), "Overwritten by this setter.") + } + DefinedProperty::Value(node) => { + diagnostic.detail(node.range(), "Overwritten with this value.") + } + DefinedProperty::GetterSetter(getter_node, setter_node) => match property_type { + PropertyType::Getter => { + diagnostic.detail(getter_node.range(), "Overwritten by this getter.") + } + PropertyType::Setter => { + diagnostic.detail(setter_node.range(), "Overwritten by this setter.") + } + PropertyType::Value => { + let diagnostic = + diagnostic.detail(getter_node.range(), "Overwritten by this getter."); + let diagnostic = + diagnostic.detail(setter_node.range(), "Overwritten by this setter."); + diagnostic + } + }, + }; + diagnostic = diagnostic.note("If an object property or with the same key is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored."); + + Some(diagnostic) + } + + fn action( + ctx: &RuleContext, + PropertyConflict(_, PropertyDefinition(property_type, node)): &Self::State, + ) -> Option { + let mut batch = ctx.root().begin(); + batch.remove_node(node.clone()); + Some(JsRuleAction { + category: rome_analyze::ActionCategory::QuickFix, + // The property initialization could contain side effects + applicability: rome_diagnostics::Applicability::MaybeIncorrect, + message: markup!("Remove this property " {property_type.to_string()}).to_owned(), + mutation: batch, + }) + } +} + +fn get_property_name(member: &JsAnyObjectMember) -> Option { + match member { + JsAnyObjectMember::JsGetterObjectMember(member) => { + member.name().ok()?.as_js_literal_member_name()?.name().ok() + } + JsAnyObjectMember::JsMethodObjectMember(member) => { + member.name().ok()?.as_js_literal_member_name()?.name().ok() + } + JsAnyObjectMember::JsPropertyObjectMember(member) => { + member.name().ok()?.as_js_literal_member_name()?.name().ok() + } + JsAnyObjectMember::JsSetterObjectMember(member) => { + member.name().ok()?.as_js_literal_member_name()?.name().ok() + } + JsAnyObjectMember::JsShorthandPropertyObjectMember(member) => { + Some(member.name().ok()?.text()) + } + JsAnyObjectMember::JsSpread(_) | JsAnyObjectMember::JsUnknownMember(_) => None, + } +} diff --git a/crates/rome_js_analyze/tests/specs/nursery/noDupeKeys.js b/crates/rome_js_analyze/tests/specs/nursery/noDupeKeys.js new file mode 100644 index 000000000000..c7dd042bf7e5 --- /dev/null +++ b/crates/rome_js_analyze/tests/specs/nursery/noDupeKeys.js @@ -0,0 +1,36 @@ +// invalid + +({ a: 1, a: 2 }); +({ "": 1, "": 2 }); +({ 0x1: 1, 1: 2 }); +({ 012: 1, 10: 2 }); +({ 0b1: 1, 1: 2 }); +({ 0o1: 1, 1: 2 }); +({ 1n: 1, 1: 2 }); +({ 1_0: 1, 10: 2 }); +({ "z": 1, z: 2 }); +({ get a() {}, get a() {} }); +({ set a(v) {}, set a(v) {} }); +({ a: 1, get a() {} }); +({ a: 1, set a(v) {} }); +({ get a() {}, a: 1 }); +({ set a(v) {}, a: 1 }); +({ get a() {}, a: 1, set a(v) {} }); +({ get a() {}, set a(v) {}, a: 1 }); + +// valid + +({ a: 1, b: 1 }); +({ "": 1, " ": 1 }); +({ 012: 1, 12: 1 }); +({ 1_0: 1, 1: 1 }); +// This particular simple computed property case with just a string literal would be easy to catch, +// but we don't want to open Pandora's static analysis box so we have to draw a line somewhere +({ a: 1, ["a"]: 1 }); +({ a: 1, [a]: 1 }); +({ [a]: 1, [a]: 1 }); +({ get a() {}, set a(v) {} }); +({ a: 1, ...a }); +({ a: 1, b: { a: 1, b: 1 } }); +// Not object keys, so out of scope for this rule +var { a, a } = obj; diff --git a/crates/rome_service/src/configuration/linter/rules.rs b/crates/rome_service/src/configuration/linter/rules.rs index b028325a1abd..1d493cf81fe5 100644 --- a/crates/rome_service/src/configuration/linter/rules.rs +++ b/crates/rome_service/src/configuration/linter/rules.rs @@ -689,6 +689,7 @@ pub struct Nursery { struct NurserySchema { no_banned_types: Option, no_const_assign: Option, + no_dupe_keys: Option, no_explicit_any: Option, no_invalid_constructor_super: Option, use_camel_case: Option, @@ -698,9 +699,10 @@ struct NurserySchema { } impl Nursery { const CATEGORY_NAME: &'static str = "nursery"; - pub(crate) const CATEGORY_RULES: [&'static str; 8] = [ + pub(crate) const CATEGORY_RULES: [&'static str; 9] = [ "noBannedTypes", "noConstAssign", + "noDupeKeys", "noExplicitAny", "noInvalidConstructorSuper", "useCamelCase", diff --git a/editors/vscode/configuration_schema.json b/editors/vscode/configuration_schema.json index 2dbf7e2a7067..b0760bb999e2 100644 --- a/editors/vscode/configuration_schema.json +++ b/editors/vscode/configuration_schema.json @@ -704,6 +704,16 @@ } ] }, + "noDupeKeys": { + "anyOf": [ + { + "$ref": "#/definitions/RuleConfiguration" + }, + { + "type": "null" + } + ] + }, "noExplicitAny": { "anyOf": [ { diff --git a/npm/backend-jsonrpc/src/workspace.ts b/npm/backend-jsonrpc/src/workspace.ts index cb979ec8e65e..834d7b745b26 100644 --- a/npm/backend-jsonrpc/src/workspace.ts +++ b/npm/backend-jsonrpc/src/workspace.ts @@ -210,6 +210,7 @@ export interface Correctness { export interface Nursery { noBannedTypes?: RuleConfiguration; noConstAssign?: RuleConfiguration; + noDupeKeys?: RuleConfiguration; noExplicitAny?: RuleConfiguration; noInvalidConstructorSuper?: RuleConfiguration; /** @@ -385,14 +386,15 @@ export type Category = | "lint/a11y/useAltText" | "lint/security/noDangerouslySetInnerHtml" | "lint/security/noDangerouslySetInnerHtmlWithChildren" - | "lint/nursery/useFlatMap" + | "lint/nursery/noBannedTypes" | "lint/nursery/noConstAssign" + | "lint/nursery/noDupeKeys" | "lint/nursery/noExplicitAny" - | "lint/nursery/useValidForDirection" | "lint/nursery/noInvalidConstructorSuper" - | "lint/nursery/useExhaustiveDependencies" | "lint/nursery/useCamelCase" - | "lint/nursery/noBannedTypes" + | "lint/nursery/useExhaustiveDependencies" + | "lint/nursery/useFlatMap" + | "lint/nursery/useValidForDirection" | "files/missingHandler" | "format" | "internalError/io" diff --git a/website/src/docs/lint/rules/index.md b/website/src/docs/lint/rules/index.md index 3b1de408b1b7..2b969fb835ff 100644 --- a/website/src/docs/lint/rules/index.md +++ b/website/src/docs/lint/rules/index.md @@ -440,6 +440,13 @@ Disallow certain types. Prevents from having const variables being re-assigned.
+

+ noDupeKeys +

+Prevents object literals having more than one property declaration for the same key. +If the same key is specified multiple times, only the last property declaration will prevail and previous will be lost, which is likely a mistake. +
+

noExplicitAny

diff --git a/website/src/docs/lint/rules/noDupeKeys.md b/website/src/docs/lint/rules/noDupeKeys.md new file mode 100644 index 000000000000..29abd3400b13 --- /dev/null +++ b/website/src/docs/lint/rules/noDupeKeys.md @@ -0,0 +1,96 @@ +--- +title: Lint Rule noDupeKeys +layout: layouts/rule.liquid +--- + +# noDupeKeys (since v10.0.0) + +Prevents object literals having more than one property declaration for the same key. +If the same key is specified multiple times, only the last property declaration will prevail and previous will be lost, which is likely a mistake. + +## Examples + +### Invalid + +```jsx +const obj = { + a: 1, + a: 2, +} +``` + +{% raw %}
nursery/noDupeKeys.js:2:5 lint/nursery/noDupeKeys  FIXABLE  ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
+
+  ⚠ This property value is later overwritten by a property with the same name.
+  
+    1 │ const obj = {
+  > 2 │    	a: 1,
+      │    	^^^^
+    3 │    	a: 2,
+    4 │ }
+  
+  ℹ Overwritten with this value.
+  
+    1 │ const obj = {
+    2 │    	a: 1,
+  > 3 │    	a: 2,
+      │    	^^^^
+    4 │ }
+    5 │ 
+  
+  ℹ If an object property or with the same key is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
+  
+  ℹ Suggested fix: Remove this property value
+  
+  
+
{% endraw %} + +```jsx +const obj = { + set a(v) {}, + a: 2, +} +``` + +{% raw %}
nursery/noDupeKeys.js:2:5 lint/nursery/noDupeKeys  FIXABLE  ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
+
+  ⚠ This property setter is later overwritten by a property with the same name.
+  
+    1 │ const obj = {
+  > 2 │    	set a(v) {},
+      │    	^^^^^^^^^^^
+    3 │    	a: 2,
+    4 │ }
+  
+  ℹ Overwritten with this value.
+  
+    1 │ const obj = {
+    2 │    	set a(v) {},
+  > 3 │    	a: 2,
+      │    	^^^^
+    4 │ }
+    5 │ 
+  
+  ℹ If an object property or with the same key is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
+  
+  ℹ Suggested fix: Remove this property setter
+  
+  
+
{% endraw %} + +### Valid + +```jsx +const obj = { + a: 1, + b: 2, +} +``` + +```jsx +const obj = { + get a() { return 1; }, + set a(v) {}, +} +``` +