From 9ba192031a16b0eeafcb0f2747860202e0f89fd3 Mon Sep 17 00:00:00 2001 From: Dilshad Date: Mon, 4 Mar 2024 08:55:34 +0530 Subject: [PATCH] Revert "fix(linter): fix getter return rule false positives in TypeScript (#2543)" This reverts commit 5d3e42e7713545bfec13c8afeb982611fc498c69. --- crates/oxc_ast/src/ast/ts.rs | 11 -- .../src/rules/eslint/getter_return.rs | 103 ++++-------------- 2 files changed, 20 insertions(+), 94 deletions(-) diff --git a/crates/oxc_ast/src/ast/ts.rs b/crates/oxc_ast/src/ast/ts.rs index 87978d359908c..c25414ba401c1 100644 --- a/crates/oxc_ast/src/ast/ts.rs +++ b/crates/oxc_ast/src/ast/ts.rs @@ -137,17 +137,6 @@ impl<'a> TSType<'a> { pub fn is_const_type_reference(&self) -> bool { matches!(self, TSType::TSTypeReference(reference) if reference.type_name.is_const()) } - - /// Check if type maybe `undefined` - pub fn is_maybe_undefined(&self) -> bool { - match self { - TSType::TSUndefinedKeyword(_) => true, - TSType::TSUnionType(un) => { - un.types.iter().any(|t| matches!(t, TSType::TSUndefinedKeyword(_))) - } - _ => false, - } - } } /// `SomeType extends OtherType ? TrueType : FalseType;` diff --git a/crates/oxc_linter/src/rules/eslint/getter_return.rs b/crates/oxc_linter/src/rules/eslint/getter_return.rs index dc27bfc39bbf8..24c6ce800a196 100644 --- a/crates/oxc_linter/src/rules/eslint/getter_return.rs +++ b/crates/oxc_linter/src/rules/eslint/getter_return.rs @@ -108,7 +108,6 @@ impl GetterReturn { } } - /// Checks whether it is necessary to check the node fn is_wanted_node(node: &AstNode, ctx: &LintContext<'_>) -> bool { if let Some(parent) = ctx.nodes().parent_node(node.id()) { match parent.kind() { @@ -193,7 +192,7 @@ impl GetterReturn { // // We stop this path on a ::Yes because if it was a ::No, // we would have already returned early before exploring more edges - => Some(DefinitelyReturnsOrThrowsOrUnreachable::Yes), + => Some(DefinitelyReturnsOrThrows::Yes), }, // We ignore the state going into this rule because we only care whether // or not this path definitely returns or throws. @@ -202,29 +201,6 @@ impl GetterReturn { // or No (when we see this, we immediately stop walking). Other rules that require knowing // previous such as [`no_this_before_super`] we would want to observe this value. &mut |basic_block_id, _state_going_into_this_rule| { - // The expression is the equivalent of return. - // Therefore, if a function is an expression, it always returns its value. - // - // Example expression: - // ```js - // const fn = () => 1; - // ``` - if let AstKind::ArrowFunctionExpression(arrow_expr) = node.kind() { - if arrow_expr.expression { - return (DefinitelyReturnsOrThrowsOrUnreachable::Yes, false); - } - } - - // If the signature of function supports the return of the `undefined` value, - // you do not need to check this rule - if let AstKind::Function(func) = node.kind() { - if let Some(ref ret) = func.return_type { - if ret.type_annotation.is_maybe_undefined() { - return (DefinitelyReturnsOrThrowsOrUnreachable::Yes, false); - } - } - } - // Scan through the values in this basic block. for entry in cfg.basic_block_by_index(*basic_block_id) { match entry { @@ -254,7 +230,7 @@ impl GetterReturn { // Return false as the second argument to signify we should // not continue walking this branch, as we know a return // is the end of this path. - return (DefinitelyReturnsOrThrowsOrUnreachable::No, false); + return (DefinitelyReturnsOrThrows::No, false); } // Otherwise, we definitely returned since we assigned // to the return register. @@ -262,36 +238,32 @@ impl GetterReturn { // Return false as the second argument to signify we should // not continue walking this branch, as we know a return // is the end of this path. - return (DefinitelyReturnsOrThrowsOrUnreachable::Yes, false); + return (DefinitelyReturnsOrThrows::Yes, false); } } - // Throws are classified as returning. - // - // todo: test with catching... - BasicBlockElement::Throw(_) | - // Although the unreachable code is not returned, it will never be executed. - // There is no point in checking it for return. - // - // An example in such cases: - // ```js - // switch (val) { - // default: return 1; - // } - // return -1; - // ``` - // Make return useless. + BasicBlockElement::Throw(_) => { + // Throws are classified as returning. + // + // todo: test with catching... + return (DefinitelyReturnsOrThrows::Yes, false); + } BasicBlockElement::Unreachable => { - - return (DefinitelyReturnsOrThrowsOrUnreachable::Yes, false); + // Unreachable signifies the last element of this basic block and + // this path that will be observed by javascript, therefore if we + // haven't returned yet we won't after this. + // + // Return false as the second argument to signify we should + // not continue walking this branch, as we know a return + // is the end of this path. + return (DefinitelyReturnsOrThrows::No, false); } } } - // Return true as the second argument to signify we should // continue walking this branch, as we haven't seen anything // that will signify to us that this path of the program will // definitely return or throw. - (DefinitelyReturnsOrThrowsOrUnreachable::No, true) + (DefinitelyReturnsOrThrows::No, true) }, ); @@ -299,7 +271,7 @@ impl GetterReturn { // codepaths is as simple as seeing if each individual codepath // definitely returns or throws. let definitely_returns_in_all_codepaths = - output.into_iter().all(|y| matches!(y, DefinitelyReturnsOrThrowsOrUnreachable::Yes)); + output.into_iter().all(|y| matches!(y, DefinitelyReturnsOrThrows::Yes)); // If not, flag it as a diagnostic. if !definitely_returns_in_all_codepaths { @@ -309,7 +281,7 @@ impl GetterReturn { } #[derive(Default, Copy, Clone, Debug)] -enum DefinitelyReturnsOrThrowsOrUnreachable { +enum DefinitelyReturnsOrThrows { #[default] No, Yes, @@ -393,41 +365,6 @@ fn test() { ("foo.create(null, { bar: { get() {} } });", None), ("var foo = { get willThrowSoValid() { throw MyException() } };", None), ("export abstract class Foo { protected abstract get foobar(): number; }", None), - ("class T { - theme: number; - get type(): number { - switch (theme) { - case 1: return 1; - case 2: return 2; - default: return 3; - } - throw new Error('test') - } - }", None), - ("class T { - theme: number; - get type(): number { - switch (theme) { - case 1: return 1; - case 2: return 2; - default: return 3; - } - } - }", None), - ("const originalClearTimeout = targetWindow.clearTimeout; - Object.defineProperty(targetWindow, 'vscodeOriginalClearTimeout', { get: () => originalClearTimeout }); - ", None), - ("class T { - get width(): number | undefined { - const val = undefined - if (!val) { - return; - } - - return val * val; - } - }", None), - ("function fn(): void { console.log('test') }", None) ]; let fail = vec![