Skip to content

Commit

Permalink
Revert "fix(linter): fix getter return rule false positives in TypeSc…
Browse files Browse the repository at this point in the history 
…ript (oxc-project#2543)"

This reverts commit 5d3e42e.
  • Loading branch information
@a-rustacean
a-rustacean authored Mar 4, 2024
1 parent 62fe057 commit 9ba1920
Show file tree
Hide file tree
Showing 2 changed files with 20 additions and 94 deletions.
11 changes: 0 additions & 11 deletions crates/oxc_ast/src/ast/ts.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;`
Expand Down
103 changes: 20 additions & 83 deletions crates/oxc_linter/src/rules/eslint/getter_return.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down Expand Up @@ -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.
Expand All @@ -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 {
Expand Down Expand Up @@ -254,52 +230,48 @@ 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.
//
// 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)
},
);

// Deciding whether we definitely return or throw in all
// 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 {
Expand All @@ -309,7 +281,7 @@ impl GetterReturn {
}

#[derive(Default, Copy, Clone, Debug)]
enum DefinitelyReturnsOrThrowsOrUnreachable {
enum DefinitelyReturnsOrThrows {
#[default]
No,
Yes,
Expand Down Expand Up @@ -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![
Expand Down

0 comments on commit 9ba1920

Please sign in to comment.