-
Notifications
You must be signed in to change notification settings - Fork 172
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(control-flow): mark exprs as may_throw = true
even when scope.end is already set
#721
Conversation
#[test] | ||
fn issue_716() { | ||
let src = r#" | ||
function foo() { | ||
if (bool) {} else {} | ||
try { | ||
bar(); | ||
return 42; | ||
} catch (err) { | ||
console.error(err); | ||
} | ||
} | ||
"#; | ||
let flow = analyze_flow(src); | ||
assert_flow!(flow, 1, false, Some(End::Continue)); // function | ||
assert_flow!(flow, 16, false, Some(End::Continue)); // BlockStmt of `foo` | ||
assert_flow!(flow, 20, false, Some(End::Continue)); // if stmt | ||
assert_flow!(flow, 30, false, Some(End::Continue)); // BlockStmt of if | ||
assert_flow!(flow, 38, false, Some(End::Continue)); // BlockStmt of else | ||
assert_flow!(flow, 43, false, Some(End::Continue)); // try stmt | ||
assert_flow!(flow, 47, false, Some(End::forced_return())); // BlockStmt of try | ||
assert_flow!(flow, 53, false, None); // `bar();` | ||
assert_flow!(flow, 64, false, Some(End::forced_return())); // return stmt | ||
assert_flow!(flow, 79, false, Some(End::Continue)); // catch clause | ||
assert_flow!(flow, 91, false, Some(End::Continue)); // BlockStmt of catch | ||
assert_flow!(flow, 97, false, None); // `console.error(err);` | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a test to ensure the control-flow analyzer works as expected.
// https://github.com/denoland/deno_lint/issues/716 | ||
r#" | ||
function foo(trueOrFalse: boolean) { | ||
if (trueOrFalse) { | ||
// noop | ||
} else { | ||
// noop | ||
} | ||
try { | ||
bar(); | ||
return 42; | ||
} catch (err) { | ||
console.error(err); | ||
} | ||
} | ||
"#, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also added a direct regression test.
fn visit_expr(&mut self, n: &Expr, _: &dyn Node) { | ||
n.visit_children_with(self); | ||
|
||
if matches!(self.scope.end, None | Some(End::Continue)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only change for the implementation is here.
Previously this line looked like if self.scope.end.is_none() {
. Actually we should set self.scope.may_throw
to true
in the case of Some(End::Continue)
too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, no comments to implementation, provided tests are self-explanatory 👍
This PR fixes a false positive that occurs in situations where a try-catch block follows a if-else block.
In addition, I separated
control_flow.rs
into two files because there are so many test cases, making it less easy for us to navigate the implementation and the tests in the editor.This refactor makes the essential diff hard to find, so I'm going to add some comments to spot where it is.
Fixes #716