From df91db597b6bcf4ff03d8d067a813c5489c66c63 Mon Sep 17 00:00:00 2001 From: Victorien Elvinger Date: Tue, 8 Aug 2023 17:05:38 +0200 Subject: [PATCH] feat(rome_js_analyze): useGetterReturn --- CHANGELOG.md | 14 + .../src/categories.rs | 1 + .../rome_js_analyze/src/analyzers/nursery.rs | 2 + .../analyzers/nursery/use_getter_return.rs | 162 +++++++ .../specs/nursery/useGetterReturn/invalid.js | 129 ++++++ .../nursery/useGetterReturn/invalid.js.snap | 407 ++++++++++++++++++ .../specs/nursery/useGetterReturn/valid.js | 191 ++++++++ .../nursery/useGetterReturn/valid.js.snap | 201 +++++++++ .../src/configuration/linter/rules.rs | 57 ++- .../src/configuration/parse/json/rules.rs | 24 ++ editors/vscode/configuration_schema.json | 7 + npm/backend-jsonrpc/src/workspace.ts | 5 + npm/rome/configuration_schema.json | 7 + .../components/generated/NumberOfRules.astro | 2 +- website/src/pages/lint/rules/index.mdx | 6 + .../src/pages/lint/rules/useGetterReturn.md | 79 ++++ 16 files changed, 1274 insertions(+), 20 deletions(-) create mode 100644 crates/rome_js_analyze/src/analyzers/nursery/use_getter_return.rs create mode 100644 crates/rome_js_analyze/tests/specs/nursery/useGetterReturn/invalid.js create mode 100644 crates/rome_js_analyze/tests/specs/nursery/useGetterReturn/invalid.js.snap create mode 100644 crates/rome_js_analyze/tests/specs/nursery/useGetterReturn/valid.js create mode 100644 crates/rome_js_analyze/tests/specs/nursery/useGetterReturn/valid.js.snap create mode 100644 website/src/pages/lint/rules/useGetterReturn.md diff --git a/CHANGELOG.md b/CHANGELOG.md index cdbb49df511..3f76465e25f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,20 @@ ### Formatter ### JavaScript APIs ### Linter + +### New rules + +- Add [`useGetterReturn`](https://docs.rome.tools/lint/rules/useGetterReturn/) + + This rule enforces the presence of non-empty return statements in getters. + This makes the following code incorrect: + + ```js + class Person { + get firstName() {} + } + ``` + ### Parser ### VSCode diff --git a/crates/rome_diagnostics_categories/src/categories.rs b/crates/rome_diagnostics_categories/src/categories.rs index 934cf261daa..74ce6f3cfcb 100644 --- a/crates/rome_diagnostics_categories/src/categories.rs +++ b/crates/rome_diagnostics_categories/src/categories.rs @@ -102,6 +102,7 @@ define_categories! { "lint/nursery/useAriaPropTypes": "https://docs.rome.tools/lint/rules/useAriaPropTypes", "lint/nursery/useArrowFunction": "https://docs.rome.tools/lint/rules/useArrowFunction", "lint/nursery/useExhaustiveDependencies": "https://docs.rome.tools/lint/rules/useExhaustiveDependencies", + "lint/nursery/useGetterReturn": "https://docs.rome.tools/lint/rules/useGetterReturn", "lint/nursery/useGroupedTypeImport": "https://docs.rome.tools/lint/rules/useGroupedTypeImport", "lint/nursery/useHookAtTopLevel": "https://docs.rome.tools/lint/rules/useHookAtTopLevel", "lint/nursery/useImportRestrictions": "https://docs.rome.tools/lint/rules/useImportRestrictions", diff --git a/crates/rome_js_analyze/src/analyzers/nursery.rs b/crates/rome_js_analyze/src/analyzers/nursery.rs index 5733a2e0d42..338087fd4ae 100644 --- a/crates/rome_js_analyze/src/analyzers/nursery.rs +++ b/crates/rome_js_analyze/src/analyzers/nursery.rs @@ -12,6 +12,7 @@ pub(crate) mod no_static_only_class; pub(crate) mod no_useless_empty_export; pub(crate) mod no_void; pub(crate) mod use_arrow_function; +pub(crate) mod use_getter_return; pub(crate) mod use_grouped_type_import; pub(crate) mod use_import_restrictions; pub(crate) mod use_literal_enum_members; @@ -30,6 +31,7 @@ declare_group! { self :: no_useless_empty_export :: NoUselessEmptyExport , self :: no_void :: NoVoid , self :: use_arrow_function :: UseArrowFunction , + self :: use_getter_return :: UseGetterReturn , self :: use_grouped_type_import :: UseGroupedTypeImport , self :: use_import_restrictions :: UseImportRestrictions , self :: use_literal_enum_members :: UseLiteralEnumMembers , diff --git a/crates/rome_js_analyze/src/analyzers/nursery/use_getter_return.rs b/crates/rome_js_analyze/src/analyzers/nursery/use_getter_return.rs new file mode 100644 index 00000000000..670ae02bfe6 --- /dev/null +++ b/crates/rome_js_analyze/src/analyzers/nursery/use_getter_return.rs @@ -0,0 +1,162 @@ +use crate::ControlFlowGraph; +use rome_analyze::{context::RuleContext, declare_rule, Rule, RuleDiagnostic}; +use rome_console::markup; +use rome_control_flow::{ExceptionHandlerKind, InstructionKind}; +use rome_js_syntax::{JsGetterClassMember, JsGetterObjectMember, JsReturnStatement}; +use rome_rowan::{AstNode, NodeOrToken, TextRange}; +use rustc_hash::FxHashSet; + +declare_rule! { + /// Enforces the presence of non-empty `return` statements in getters. + /// + /// A _getter_ allows defining a property which is dynamically computed. + /// Thus, it is desirable that a _getter_ returns a value. + /// + /// Source: https://eslint.org/docs/latest/rules/getter-return + /// + /// ## Examples + /// + /// ### Invalid + /// + /// ```js,expect_diagnostic + /// class Person { + /// get firstName() {} + /// } + /// ``` + /// + /// ```js,expect_diagnostic + /// const obj = { + /// get firstName() { + /// return; + /// }, + /// } + /// ``` + /// + /// ## Valid + /// + /// ```js + /// class Person { + /// get firstName() { + /// return this.fullname.split(" ")[0]; + /// } + /// } + /// ``` + /// + /// ```js + /// const obj = { + /// get firstName() { + /// return this.fullname.split(" ")[0]; + /// }, + /// } + /// ``` + /// + pub(crate) UseGetterReturn { + version: "next", + name: "useGetterReturn", + recommended: true, + } +} + +impl Rule for UseGetterReturn { + type Query = ControlFlowGraph; + type State = InvalidGetterReturn; + type Signals = Vec; + type Options = (); + + fn run(ctx: &RuleContext) -> Self::Signals { + let cfg = ctx.query(); + let node_kind = cfg.node.kind(); + let mut invalid_returns = Vec::new(); + if !JsGetterClassMember::can_cast(node_kind) && !JsGetterObjectMember::can_cast(node_kind) { + // The node is not a getter. + return invalid_returns; + } + // stack of blocks to process + let mut block_stack = Vec::new(); + let mut visited_blocks = FxHashSet::default(); + block_stack.push(0u32); + visited_blocks.insert(0u32); + while let Some(block_index) = block_stack.pop() { + // SAFETY: this is a safe conversion because it is already an index for `cfg.blocks`. + let block_index = block_index as usize; + let Some(block) = cfg.blocks.get(block_index) else { + continue; + }; + for exception_handler in block.exception_handlers.iter() { + // Ignore finally handler: they are already in the Control Flow Graph. + if matches!(exception_handler.kind, ExceptionHandlerKind::Catch) { + // Avoid cycles and redundant checks. + if visited_blocks.insert(exception_handler.target) { + block_stack.push(exception_handler.target); + } + } + } + for instruction in block.instructions.iter() { + match instruction.kind { + InstructionKind::Statement => {} + InstructionKind::Jump { + block, conditional, .. + } => { + let jump_block_index = block.index(); + // Avoid cycles and redundant checks. + if visited_blocks.insert(jump_block_index) { + block_stack.push(jump_block_index); + } + if !conditional { + // The next instructions are unreachable. + break; + } + } + InstructionKind::Return => { + if let Some(NodeOrToken::Node(node)) = instruction.node.clone() { + if let Some(return_stmt) = JsReturnStatement::cast(node) { + if return_stmt.argument().is_none() { + invalid_returns.push(InvalidGetterReturn::EmptyReturn( + return_stmt.range(), + )); + } + } + } else { + invalid_returns.push(InvalidGetterReturn::MissingReturn); + } + // The next instructions are unreachable. + break; + } + } + } + } + invalid_returns + } + + fn diagnostic(ctx: &RuleContext, invalid_return: &Self::State) -> Option { + let cfg = ctx.query(); + let diagnostic = match invalid_return { + InvalidGetterReturn::MissingReturn => { + let getter_range = cfg.node.text_trimmed_range(); + RuleDiagnostic::new( + rule_category!(), + getter_range, + markup! { + "This ""getter"" should ""return"" a value." + }, + ) + } + InvalidGetterReturn::EmptyReturn(return_stmt_range) => RuleDiagnostic::new( + rule_category!(), + return_stmt_range, + markup! { + "This ""return"" should return a value because it is located in a ""return""." + }, + ), + }; + Some(diagnostic) + } +} + +#[derive(Debug)] +pub(crate) enum InvalidGetterReturn { + /// No `return` statement. + MissingReturn, + // A `return` statement without argument. + EmptyReturn(TextRange), +} diff --git a/crates/rome_js_analyze/tests/specs/nursery/useGetterReturn/invalid.js b/crates/rome_js_analyze/tests/specs/nursery/useGetterReturn/invalid.js new file mode 100644 index 00000000000..30bf7fab361 --- /dev/null +++ b/crates/rome_js_analyze/tests/specs/nursery/useGetterReturn/invalid.js @@ -0,0 +1,129 @@ +var foo = { + get bar() {} +}; + +var foo = { + get bar(){ + if(baz) { + return true; + } + } +}; + +var foo = { + get bar() { + ~function () { + return true; + } + } +}; + +var foo = { + get bar() { + return; + } +}; + +class Foo { + get bar() {} +} + +class Foo { + get bar(){ + if(baz) { + return true; + } + } +} + +class Foo { + get bar() { + ~function () { + return true; + } + } +} + +class Foo { + get bar() { + return; + } +} + +class Foo { + get bar(){ + try { + return foo(); + } catch {} finally {} + } +} + +class Foo { + get bar(){ + switch (this.n) { + case 0: + return 0; + case 1: + case 2: + break; + } + } +} + +var foo = { + get bar(){ + if(baz) { + return true; + } else { + false; + } + } +}; + +var foo = { + get bar(){ + if(baz) { + true; + } else { + return false; + } + } +}; + +var foo = { + get bar(){ + for (;;) { + break; + } + while (false) { + return true; + } + } +}; + +var foo = { + get bar(){ + do { + if (bar()) { + return 0; + } + } while(foo()); + for (x in [1, 2]) { + if (x == 0) { + return 0; + } + } + } +}; + +var foo = { + get bar(){ + if(baz) { + if (foo) { + return true; + } + } else { + return true; + } + } +}; diff --git a/crates/rome_js_analyze/tests/specs/nursery/useGetterReturn/invalid.js.snap b/crates/rome_js_analyze/tests/specs/nursery/useGetterReturn/invalid.js.snap new file mode 100644 index 00000000000..05f7c9a2d21 --- /dev/null +++ b/crates/rome_js_analyze/tests/specs/nursery/useGetterReturn/invalid.js.snap @@ -0,0 +1,407 @@ +--- +source: crates/rome_js_analyze/tests/spec_tests.rs +expression: invalid.js +--- +# Input +```js +var foo = { + get bar() {} +}; + +var foo = { + get bar(){ + if(baz) { + return true; + } + } +}; + +var foo = { + get bar() { + ~function () { + return true; + } + } +}; + +var foo = { + get bar() { + return; + } +}; + +class Foo { + get bar() {} +} + +class Foo { + get bar(){ + if(baz) { + return true; + } + } +} + +class Foo { + get bar() { + ~function () { + return true; + } + } +} + +class Foo { + get bar() { + return; + } +} + +class Foo { + get bar(){ + try { + return foo(); + } catch {} finally {} + } +} + +class Foo { + get bar(){ + switch (this.n) { + case 0: + return 0; + case 1: + case 2: + break; + } + } +} + +var foo = { + get bar(){ + if(baz) { + return true; + } else { + false; + } + } +}; + +var foo = { + get bar(){ + if(baz) { + true; + } else { + return false; + } + } +}; + +var foo = { + get bar(){ + for (;;) { + break; + } + while (false) { + return true; + } + } +}; + +var foo = { + get bar(){ + do { + if (bar()) { + return 0; + } + } while(foo()); + for (x in [1, 2]) { + if (x == 0) { + return 0; + } + } + } +}; + +var foo = { + get bar(){ + if(baz) { + if (foo) { + return true; + } + } else { + return true; + } + } +}; + +``` + +# Diagnostics +``` +invalid.js:2:5 lint/nursery/useGetterReturn ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! This getter should return a value. + + 1 │ var foo = { + > 2 │ get bar() {} + │ ^^^^^^^^^^^^ + 3 │ }; + 4 │ + + +``` + +``` +invalid.js:6:5 lint/nursery/useGetterReturn ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! This getter should return a value. + + 5 │ var foo = { + > 6 │ get bar(){ + │ ^^^^^^^^^^ + > 7 │ if(baz) { + > 8 │ return true; + > 9 │ } + > 10 │ } + │ ^ + 11 │ }; + 12 │ + + +``` + +``` +invalid.js:14:5 lint/nursery/useGetterReturn ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! This getter should return a value. + + 13 │ var foo = { + > 14 │ get bar() { + │ ^^^^^^^^^^^ + > 15 │ ~function () { + > 16 │ return true; + > 17 │ } + > 18 │ } + │ ^ + 19 │ }; + 20 │ + + +``` + +``` +invalid.js:23:9 lint/nursery/useGetterReturn ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! This return should return a value because it is located in a return. + + 21 │ var foo = { + 22 │ get bar() { + > 23 │ return; + │ ^^^^^^^ + 24 │ } + 25 │ }; + + +``` + +``` +invalid.js:28:5 lint/nursery/useGetterReturn ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! This getter should return a value. + + 27 │ class Foo { + > 28 │ get bar() {} + │ ^^^^^^^^^^^^ + 29 │ } + 30 │ + + +``` + +``` +invalid.js:32:5 lint/nursery/useGetterReturn ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! This getter should return a value. + + 31 │ class Foo { + > 32 │ get bar(){ + │ ^^^^^^^^^^ + > 33 │ if(baz) { + > 34 │ return true; + > 35 │ } + > 36 │ } + │ ^ + 37 │ } + 38 │ + + +``` + +``` +invalid.js:40:5 lint/nursery/useGetterReturn ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! This getter should return a value. + + 39 │ class Foo { + > 40 │ get bar() { + │ ^^^^^^^^^^^ + > 41 │ ~function () { + > 42 │ return true; + > 43 │ } + > 44 │ } + │ ^ + 45 │ } + 46 │ + + +``` + +``` +invalid.js:49:9 lint/nursery/useGetterReturn ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! This return should return a value because it is located in a return. + + 47 │ class Foo { + 48 │ get bar() { + > 49 │ return; + │ ^^^^^^^ + 50 │ } + 51 │ } + + +``` + +``` +invalid.js:54:5 lint/nursery/useGetterReturn ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! This getter should return a value. + + 53 │ class Foo { + > 54 │ get bar(){ + │ ^^^^^^^^^^ + > 55 │ try { + > 56 │ return foo(); + > 57 │ } catch {} finally {} + > 58 │ } + │ ^ + 59 │ } + 60 │ + + +``` + +``` +invalid.js:62:5 lint/nursery/useGetterReturn ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! This getter should return a value. + + 61 │ class Foo { + > 62 │ get bar(){ + │ ^^^^^^^^^^ + > 63 │ switch (this.n) { + ... + > 69 │ } + > 70 │ } + │ ^ + 71 │ } + 72 │ + + +``` + +``` +invalid.js:74:5 lint/nursery/useGetterReturn ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! This getter should return a value. + + 73 │ var foo = { + > 74 │ get bar(){ + │ ^^^^^^^^^^ + > 75 │ if(baz) { + ... + > 79 │ } + > 80 │ } + │ ^ + 81 │ }; + 82 │ + + +``` + +``` +invalid.js:84:5 lint/nursery/useGetterReturn ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! This getter should return a value. + + 83 │ var foo = { + > 84 │ get bar(){ + │ ^^^^^^^^^^ + > 85 │ if(baz) { + ... + > 89 │ } + > 90 │ } + │ ^ + 91 │ }; + 92 │ + + +``` + +``` +invalid.js:94:5 lint/nursery/useGetterReturn ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! This getter should return a value. + + 93 │ var foo = { + > 94 │ get bar(){ + │ ^^^^^^^^^^ + > 95 │ for (;;) { + ... + > 100 │ } + > 101 │ } + │ ^ + 102 │ }; + 103 │ + + +``` + +``` +invalid.js:105:5 lint/nursery/useGetterReturn ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! This getter should return a value. + + 104 │ var foo = { + > 105 │ get bar(){ + │ ^^^^^^^^^^ + > 106 │ do { + ... + > 115 │ } + > 116 │ } + │ ^ + 117 │ }; + 118 │ + + +``` + +``` +invalid.js:120:5 lint/nursery/useGetterReturn ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + ! This getter should return a value. + + 119 │ var foo = { + > 120 │ get bar(){ + │ ^^^^^^^^^^ + > 121 │ if(baz) { + ... + > 127 │ } + > 128 │ } + │ ^ + 129 │ }; + 130 │ + + +``` + + diff --git a/crates/rome_js_analyze/tests/specs/nursery/useGetterReturn/valid.js b/crates/rome_js_analyze/tests/specs/nursery/useGetterReturn/valid.js new file mode 100644 index 00000000000..24a9914bd18 --- /dev/null +++ b/crates/rome_js_analyze/tests/specs/nursery/useGetterReturn/valid.js @@ -0,0 +1,191 @@ +var foo = { + get bar(){ + return true; + } +}; + +var foo = { + get bar(){ + throw new Error("no value"); + } +}; + +var foo = { + get bar(){ + if(baz) { + if (foo) { + return true; + } else { + return true; + } + } else { + return true; + } + } +}; + +class Foo { + get bar(){ + return true; + } +} + +class Foo { + get bar(){ + if (baz) { + let x = 0; + while (cond) { + x++; + } + return x; + } else { + return 0; + } + } +} + +class Foo { + get bar(){ + try { + return foo(); + } catch {} finally { + return 0; + } + } +} + +class Foo { + get bar(){ + try { + return foo(); + } catch { + return 0; + } finally {} + } +} + +class Foo { + get bar(){ + try { + foo(); + } catch {} finally { + return 0; + } + } +} + +class Foo { + get bar(){ + try { + foo(); + } finally { + return 0; + } + } +} + +class Foo { + get(){ + switch (this.n) { + case 1: + case 2: + return 2; + default: + return 0; + } + } +} + +Object.defineProperty(foo, "bar", { + get: function () { + return true; + } +}); + +Object.defineProperty(foo, "bar", { + get: function () { + ~function (){ return true; }(); + return true; + } +}); + +Object.defineProperties(foo, { + bar: { + get: function () { + return true; + } + } +}); + +Object.defineProperties(foo, { + bar: { + get: function () { + ~function (){ + return true; + }(); + return true; + } + } +}); + +Reflect.defineProperty(foo, "bar", { + get: function () { + return true; + } +}); + +Reflect.defineProperty(foo, "bar", { + get: function () { + ~function (){ + return true; + }(); + return true; + } +}) + +Object.create(foo, { + bar: { + get() { + return true; + } + } +}); + +Object.create(foo, { bar: { + get: function () { + return true;} + } +}); + +Object.create(foo, { bar: { + get: () => { + return true;} + } +}); + +// not getter. +var get = function(){}; + +var get = function(){ return true; }; + +var foo = { bar(){} }; + +var foo = { bar(){ return true; } }; + +var foo = { bar: function(){} }; + +var foo = { bar: function(){return;} }; + +var foo = { bar: function(){return true;} }; + +var foo = { get: function () {} } + +var foo = { get: () => {}}; + +class C { get; foo() {} } + +foo.defineProperty(null, { get() {} }); + +foo.defineProperties(null, { bar: { get() {} } }); + +foo.create(null, { bar: { get() {} } }) diff --git a/crates/rome_js_analyze/tests/specs/nursery/useGetterReturn/valid.js.snap b/crates/rome_js_analyze/tests/specs/nursery/useGetterReturn/valid.js.snap new file mode 100644 index 00000000000..6b5d579d829 --- /dev/null +++ b/crates/rome_js_analyze/tests/specs/nursery/useGetterReturn/valid.js.snap @@ -0,0 +1,201 @@ +--- +source: crates/rome_js_analyze/tests/spec_tests.rs +expression: valid.js +--- +# Input +```js +var foo = { + get bar(){ + return true; + } +}; + +var foo = { + get bar(){ + throw new Error("no value"); + } +}; + +var foo = { + get bar(){ + if(baz) { + if (foo) { + return true; + } else { + return true; + } + } else { + return true; + } + } +}; + +class Foo { + get bar(){ + return true; + } +} + +class Foo { + get bar(){ + if (baz) { + let x = 0; + while (cond) { + x++; + } + return x; + } else { + return 0; + } + } +} + +class Foo { + get bar(){ + try { + return foo(); + } catch {} finally { + return 0; + } + } +} + +class Foo { + get bar(){ + try { + return foo(); + } catch { + return 0; + } finally {} + } +} + +class Foo { + get bar(){ + try { + foo(); + } catch {} finally { + return 0; + } + } +} + +class Foo { + get bar(){ + try { + foo(); + } finally { + return 0; + } + } +} + +class Foo { + get(){ + switch (this.n) { + case 1: + case 2: + return 2; + default: + return 0; + } + } +} + +Object.defineProperty(foo, "bar", { + get: function () { + return true; + } +}); + +Object.defineProperty(foo, "bar", { + get: function () { + ~function (){ return true; }(); + return true; + } +}); + +Object.defineProperties(foo, { + bar: { + get: function () { + return true; + } + } +}); + +Object.defineProperties(foo, { + bar: { + get: function () { + ~function (){ + return true; + }(); + return true; + } + } +}); + +Reflect.defineProperty(foo, "bar", { + get: function () { + return true; + } +}); + +Reflect.defineProperty(foo, "bar", { + get: function () { + ~function (){ + return true; + }(); + return true; + } +}) + +Object.create(foo, { + bar: { + get() { + return true; + } + } +}); + +Object.create(foo, { bar: { + get: function () { + return true;} + } +}); + +Object.create(foo, { bar: { + get: () => { + return true;} + } +}); + +// not getter. +var get = function(){}; + +var get = function(){ return true; }; + +var foo = { bar(){} }; + +var foo = { bar(){ return true; } }; + +var foo = { bar: function(){} }; + +var foo = { bar: function(){return;} }; + +var foo = { bar: function(){return true;} }; + +var foo = { get: function () {} } + +var foo = { get: () => {}}; + +class C { get; foo() {} } + +foo.defineProperty(null, { get() {} }); + +foo.defineProperties(null, { bar: { get() {} } }); + +foo.create(null, { bar: { get() {} } }) + +``` + + diff --git a/crates/rome_service/src/configuration/linter/rules.rs b/crates/rome_service/src/configuration/linter/rules.rs index 31123c79f90..7fb09abd79c 100644 --- a/crates/rome_service/src/configuration/linter/rules.rs +++ b/crates/rome_service/src/configuration/linter/rules.rs @@ -2026,6 +2026,10 @@ pub struct Nursery { )] #[serde(skip_serializing_if = "Option::is_none")] pub use_exhaustive_dependencies: Option, + #[doc = "Enforces the presence of non-empty return statements in getters."] + #[bpaf(long("use-getter-return"), argument("on|off|warn"), optional, hide)] + #[serde(skip_serializing_if = "Option::is_none")] + pub use_getter_return: Option, #[doc = "Enforce the use of import type when an import only has specifiers with type qualifier."] #[bpaf( long("use-grouped-type-import"), @@ -2068,7 +2072,7 @@ pub struct Nursery { } impl Nursery { const GROUP_NAME: &'static str = "nursery"; - pub(crate) const GROUP_RULES: [&'static str; 28] = [ + pub(crate) const GROUP_RULES: [&'static str; 29] = [ "noAccumulatingSpread", "noAriaUnsupportedElements", "noBannedTypes", @@ -2091,6 +2095,7 @@ impl Nursery { "useAriaPropTypes", "useArrowFunction", "useExhaustiveDependencies", + "useGetterReturn", "useGroupedTypeImport", "useHookAtTopLevel", "useImportRestrictions", @@ -2098,7 +2103,7 @@ impl Nursery { "useLiteralEnumMembers", "useNamingConvention", ]; - const RECOMMENDED_RULES: [&'static str; 18] = [ + const RECOMMENDED_RULES: [&'static str; 19] = [ "noAriaUnsupportedElements", "noBannedTypes", "noConstantCondition", @@ -2114,11 +2119,12 @@ impl Nursery { "noUselessEmptyExport", "useArrowFunction", "useExhaustiveDependencies", + "useGetterReturn", "useGroupedTypeImport", "useIsArray", "useLiteralEnumMembers", ]; - const RECOMMENDED_RULES_AS_FILTERS: [RuleFilter<'static>; 18] = [ + const RECOMMENDED_RULES_AS_FILTERS: [RuleFilter<'static>; 19] = [ RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[1]), RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[2]), RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[4]), @@ -2135,10 +2141,11 @@ impl Nursery { RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[20]), RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[21]), RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[22]), - RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[25]), + RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[23]), RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[26]), + RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[27]), ]; - const ALL_RULES_AS_FILTERS: [RuleFilter<'static>; 28] = [ + const ALL_RULES_AS_FILTERS: [RuleFilter<'static>; 29] = [ RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[0]), RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[1]), RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[2]), @@ -2167,6 +2174,7 @@ impl Nursery { RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[25]), RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[26]), RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[27]), + RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[28]), ]; #[doc = r" Retrieves the recommended rules"] pub(crate) fn is_recommended(&self) -> bool { matches!(self.recommended, Some(true)) } @@ -2287,36 +2295,41 @@ impl Nursery { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[21])); } } - if let Some(rule) = self.use_grouped_type_import.as_ref() { + if let Some(rule) = self.use_getter_return.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[22])); } } - if let Some(rule) = self.use_hook_at_top_level.as_ref() { + if let Some(rule) = self.use_grouped_type_import.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[23])); } } - if let Some(rule) = self.use_import_restrictions.as_ref() { + if let Some(rule) = self.use_hook_at_top_level.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[24])); } } - if let Some(rule) = self.use_is_array.as_ref() { + if let Some(rule) = self.use_import_restrictions.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[25])); } } - if let Some(rule) = self.use_literal_enum_members.as_ref() { + if let Some(rule) = self.use_is_array.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[26])); } } - if let Some(rule) = self.use_naming_convention.as_ref() { + if let Some(rule) = self.use_literal_enum_members.as_ref() { if rule.is_enabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[27])); } } + if let Some(rule) = self.use_naming_convention.as_ref() { + if rule.is_enabled() { + index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[28])); + } + } index_set } pub(crate) fn get_disabled_rules(&self) -> IndexSet { @@ -2431,36 +2444,41 @@ impl Nursery { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[21])); } } - if let Some(rule) = self.use_grouped_type_import.as_ref() { + if let Some(rule) = self.use_getter_return.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[22])); } } - if let Some(rule) = self.use_hook_at_top_level.as_ref() { + if let Some(rule) = self.use_grouped_type_import.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[23])); } } - if let Some(rule) = self.use_import_restrictions.as_ref() { + if let Some(rule) = self.use_hook_at_top_level.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[24])); } } - if let Some(rule) = self.use_is_array.as_ref() { + if let Some(rule) = self.use_import_restrictions.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[25])); } } - if let Some(rule) = self.use_literal_enum_members.as_ref() { + if let Some(rule) = self.use_is_array.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[26])); } } - if let Some(rule) = self.use_naming_convention.as_ref() { + if let Some(rule) = self.use_literal_enum_members.as_ref() { if rule.is_disabled() { index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[27])); } } + if let Some(rule) = self.use_naming_convention.as_ref() { + if rule.is_disabled() { + index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[28])); + } + } index_set } #[doc = r" Checks if, given a rule name, matches one of the rules contained in this category"] @@ -2469,10 +2487,10 @@ impl Nursery { pub(crate) fn is_recommended_rule(rule_name: &str) -> bool { Self::RECOMMENDED_RULES.contains(&rule_name) } - pub(crate) fn recommended_rules_as_filters() -> [RuleFilter<'static>; 18] { + pub(crate) fn recommended_rules_as_filters() -> [RuleFilter<'static>; 19] { Self::RECOMMENDED_RULES_AS_FILTERS } - pub(crate) fn all_rules_as_filters() -> [RuleFilter<'static>; 28] { Self::ALL_RULES_AS_FILTERS } + pub(crate) fn all_rules_as_filters() -> [RuleFilter<'static>; 29] { Self::ALL_RULES_AS_FILTERS } #[doc = r" Select preset rules"] pub(crate) fn collect_preset_rules( &self, @@ -2515,6 +2533,7 @@ impl Nursery { "useAriaPropTypes" => self.use_aria_prop_types.as_ref(), "useArrowFunction" => self.use_arrow_function.as_ref(), "useExhaustiveDependencies" => self.use_exhaustive_dependencies.as_ref(), + "useGetterReturn" => self.use_getter_return.as_ref(), "useGroupedTypeImport" => self.use_grouped_type_import.as_ref(), "useHookAtTopLevel" => self.use_hook_at_top_level.as_ref(), "useImportRestrictions" => self.use_import_restrictions.as_ref(), diff --git a/crates/rome_service/src/configuration/parse/json/rules.rs b/crates/rome_service/src/configuration/parse/json/rules.rs index cf2ba76062e..b2a5a352df4 100644 --- a/crates/rome_service/src/configuration/parse/json/rules.rs +++ b/crates/rome_service/src/configuration/parse/json/rules.rs @@ -1786,6 +1786,7 @@ impl VisitNode for Nursery { "useAriaPropTypes", "useArrowFunction", "useExhaustiveDependencies", + "useGetterReturn", "useGroupedTypeImport", "useHookAtTopLevel", "useImportRestrictions", @@ -2317,6 +2318,29 @@ impl VisitNode for Nursery { )); } }, + "useGetterReturn" => match value { + AnyJsonValue::JsonStringValue(_) => { + let mut configuration = RuleConfiguration::default(); + self.map_to_known_string(&value, name_text, &mut configuration, diagnostics)?; + self.use_getter_return = Some(configuration); + } + AnyJsonValue::JsonObjectValue(_) => { + let mut rule_configuration = RuleConfiguration::default(); + rule_configuration.map_rule_configuration( + &value, + name_text, + "useGetterReturn", + diagnostics, + )?; + self.use_getter_return = Some(rule_configuration); + } + _ => { + diagnostics.push(DeserializationDiagnostic::new_incorrect_type( + "object or string", + value.range(), + )); + } + }, "useGroupedTypeImport" => match value { AnyJsonValue::JsonStringValue(_) => { let mut configuration = RuleConfiguration::default(); diff --git a/editors/vscode/configuration_schema.json b/editors/vscode/configuration_schema.json index e59b4a8b83b..f353f371dbe 100644 --- a/editors/vscode/configuration_schema.json +++ b/editors/vscode/configuration_schema.json @@ -989,6 +989,13 @@ { "type": "null" } ] }, + "useGetterReturn": { + "description": "Enforces the presence of non-empty return statements in getters.", + "anyOf": [ + { "$ref": "#/definitions/RuleConfiguration" }, + { "type": "null" } + ] + }, "useGroupedTypeImport": { "description": "Enforce the use of import type when an import only has specifiers with type qualifier.", "anyOf": [ diff --git a/npm/backend-jsonrpc/src/workspace.ts b/npm/backend-jsonrpc/src/workspace.ts index b7151409409..074ae116422 100644 --- a/npm/backend-jsonrpc/src/workspace.ts +++ b/npm/backend-jsonrpc/src/workspace.ts @@ -659,6 +659,10 @@ export interface Nursery { * Enforce all dependencies are correctly specified. */ useExhaustiveDependencies?: RuleConfiguration; + /** + * Enforces the presence of non-empty return statements in getters. + */ + useGetterReturn?: RuleConfiguration; /** * Enforce the use of import type when an import only has specifiers with type qualifier. */ @@ -1224,6 +1228,7 @@ export type Category = | "lint/nursery/useAriaPropTypes" | "lint/nursery/useArrowFunction" | "lint/nursery/useExhaustiveDependencies" + | "lint/nursery/useGetterReturn" | "lint/nursery/useGroupedTypeImport" | "lint/nursery/useHookAtTopLevel" | "lint/nursery/useImportRestrictions" diff --git a/npm/rome/configuration_schema.json b/npm/rome/configuration_schema.json index e59b4a8b83b..f353f371dbe 100644 --- a/npm/rome/configuration_schema.json +++ b/npm/rome/configuration_schema.json @@ -989,6 +989,13 @@ { "type": "null" } ] }, + "useGetterReturn": { + "description": "Enforces the presence of non-empty return statements in getters.", + "anyOf": [ + { "$ref": "#/definitions/RuleConfiguration" }, + { "type": "null" } + ] + }, "useGroupedTypeImport": { "description": "Enforce the use of import type when an import only has specifiers with type qualifier.", "anyOf": [ diff --git a/website/src/components/generated/NumberOfRules.astro b/website/src/components/generated/NumberOfRules.astro index ea5c582e0f8..07715a7f29d 100644 --- a/website/src/components/generated/NumberOfRules.astro +++ b/website/src/components/generated/NumberOfRules.astro @@ -1,2 +1,2 @@ -

Rome's linter has a total of 154 rules

\ No newline at end of file +

Rome's linter has a total of 155 rules

\ No newline at end of file diff --git a/website/src/pages/lint/rules/index.mdx b/website/src/pages/lint/rules/index.mdx index 1643c51ece8..39d49c43eb3 100644 --- a/website/src/pages/lint/rules/index.mdx +++ b/website/src/pages/lint/rules/index.mdx @@ -1078,6 +1078,12 @@ Use arrow functions over function expressions. Enforce all dependencies are correctly specified.

+

+ useGetterReturn +

+Enforces the presence of non-empty return statements in getters. +
+

useGroupedTypeImport

diff --git a/website/src/pages/lint/rules/useGetterReturn.md b/website/src/pages/lint/rules/useGetterReturn.md new file mode 100644 index 00000000000..77144031194 --- /dev/null +++ b/website/src/pages/lint/rules/useGetterReturn.md @@ -0,0 +1,79 @@ +--- +title: Lint Rule useGetterReturn +parent: lint/rules/index +--- + +# useGetterReturn (since vnext) + +Enforces the presence of non-empty `return` statements in getters. + +A _getter_ allows defining a property which is dynamically computed. +Thus, it is desirable that a _getter_ returns a value. + +Source: https://eslint.org/docs/latest/rules/getter-return + +## Examples + +### Invalid + +```jsx +class Person { + get firstName() {} +} +``` + +
nursery/useGetterReturn.js:2:5 lint/nursery/useGetterReturn ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
+
+  ✖ This getter should return a value.
+  
+    1 │ class Person {
+  > 2 │     get firstName() {}
+      │     ^^^^^^^^^^^^^^^^^^
+    3 │ }
+    4 │ 
+  
+
+ +```jsx +const obj = { + get firstName() { + return; + }, +} +``` + +
nursery/useGetterReturn.js:3:9 lint/nursery/useGetterReturn ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
+
+  ✖ This return should return a value because it is located in a return.
+  
+    1 │ const obj = {
+    2 │     get firstName() {
+  > 3 │         return;
+      │         ^^^^^^^
+    4 │     },
+    5 │ }
+  
+
+ +## Valid + +```jsx +class Person { + get firstName() { + return this.fullname.split(" ")[0]; + } +} +``` + +```jsx +const obj = { + get firstName() { + return this.fullname.split(" ")[0]; + }, +} +``` + +## Related links + +- [Disable a rule](/linter/#disable-a-lint-rule) +- [Rule options](/linter/#rule-options)