Skip to content

Commit

Permalink
Allow hooks in the expression of a match statement and if statement (#…
Browse files Browse the repository at this point in the history 
…2902)

* Allow hooks in the expression of a match statement and if statement

* Don't allow hooks in spawn and detect hooks in initialization closures

* Point to DX check when hooks are called conditionally
  • Loading branch information
@ealmloff
ealmloff authored Sep 7, 2024
1 parent 7bb53fe commit c11f2ed
Show file tree
Hide file tree
Showing 4 changed files with 232 additions and 7 deletions.
213 changes: 208 additions & 5 deletions packages/check/src/check.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@ use syn::{spanned::Spanned, visit::Visit, Pat};
use crate::{
issues::{Issue, IssueReport},
metadata::{
AnyLoopInfo, ClosureInfo, ComponentInfo, ConditionalInfo, FnInfo, ForInfo, HookInfo,
IfInfo, LoopInfo, MatchInfo, Span, WhileInfo,
AnyLoopInfo, AsyncInfo, ClosureInfo, ComponentInfo, ConditionalInfo, FnInfo, ForInfo,
HookInfo, IfInfo, LoopInfo, MatchInfo, Span, WhileInfo,
},
};

Expand Down Expand Up @@ -46,6 +46,7 @@ enum Node {
While(WhileInfo),
Loop(LoopInfo),
Closure(ClosureInfo),
Async(AsyncInfo),
ComponentFn(ComponentInfo),
HookFn(HookInfo),
OtherFn(FnInfo),
Expand Down Expand Up @@ -107,7 +108,7 @@ impl<'ast> syn::visit::Visit<'ast> for VisitHooks {
);
let mut container_fn: Option<Node> = None;
for node in self.context.iter().rev() {
match node {
match &node {
Node::If(if_info) => {
let issue = Issue::HookInsideConditional(
hook_info.clone(),
Expand Down Expand Up @@ -150,6 +151,11 @@ impl<'ast> syn::visit::Visit<'ast> for VisitHooks {
);
self.issues.push(issue);
}
Node::Async(async_info) => {
let issue =
Issue::HookInsideAsync(hook_info.clone(), async_info.clone());
self.issues.push(issue);
}
Node::ComponentFn(_) | Node::HookFn(_) | Node::OtherFn(_) => {
container_fn = Some(node.clone());
break;
Expand All @@ -164,6 +170,7 @@ impl<'ast> syn::visit::Visit<'ast> for VisitHooks {
}
}
}
syn::visit::visit_expr_call(self, i);
}

fn visit_item_fn(&mut self, i: &'ast syn::ItemFn) {
Expand Down Expand Up @@ -208,7 +215,11 @@ impl<'ast> syn::visit::Visit<'ast> for VisitHooks {
.unwrap_or_else(|| i.span())
.into(),
)));
syn::visit::visit_expr_if(self, i);
// only visit the body and else branch, calling hooks inside the expression is not conditional
self.visit_block(&i.then_branch);
if let Some(it) = &i.else_branch {
self.visit_expr(&(it).1);
}
self.context.pop();
}

Expand All @@ -221,7 +232,10 @@ impl<'ast> syn::visit::Visit<'ast> for VisitHooks {
.unwrap_or_else(|| i.span())
.into(),
)));
syn::visit::visit_expr_match(self, i);
// only visit the arms, calling hooks inside the expression is not conditional
for it in &i.arms {
self.visit_arm(it);
}
self.context.pop();
}

Expand Down Expand Up @@ -264,6 +278,13 @@ impl<'ast> syn::visit::Visit<'ast> for VisitHooks {
syn::visit::visit_expr_closure(self, i);
self.context.pop();
}

fn visit_expr_async(&mut self, i: &'ast syn::ExprAsync) {
self.context
.push(Node::Async(AsyncInfo::new(i.span().into())));
syn::visit::visit_expr_async(self, i);
self.context.pop();
}
}

#[cfg(test)]
Expand Down Expand Up @@ -416,6 +437,22 @@ mod tests {
);
}

#[test]
fn test_use_in_match_expr() {
let contents = indoc! {r#"
fn use_thing() {
match use_resource(|| async {}) {
Ok(_) => {}
Err(_) => {}
}
}
"#};

let report = check_file("app.rs".into(), contents);

assert_eq!(report.issues, vec![]);
}

#[test]
fn test_for_loop_hook() {
let contents = indoc! {r#"
Expand Down Expand Up @@ -549,6 +586,21 @@ mod tests {
assert_eq!(report.issues, vec![]);
}

#[test]
fn test_conditional_expr_okay() {
let contents = indoc! {r#"
fn App() -> Element {
if use_signal(|| true) {
println!("clap your {something}")
}
}
"#};

let report = check_file("app.rs".into(), contents);

assert_eq!(report.issues, vec![]);
}

#[test]
fn test_closure_hook() {
let contents = indoc! {r#"
Expand Down Expand Up @@ -637,4 +689,155 @@ mod tests {

assert_eq!(report.issues, vec![]);
}

#[test]
fn test_hook_inside_hook_initialization() {
let contents = indoc! {r#"
fn use_thing() {
let _a = use_signal(|| use_signal(|| 0));
}
"#};

let report = check_file("app.rs".into(), contents);

assert_eq!(
report.issues,
vec![Issue::HookInsideClosure(
HookInfo::new(
Span::new_from_str(
"use_signal(|| 0)",
LineColumn {
line: 2,
column: 27,
},
),
Span::new_from_str(
"use_signal",
LineColumn {
line: 2,
column: 27,
},
),
"use_signal".to_string()
),
ClosureInfo::new(Span::new_from_str(
"|| use_signal(|| 0)",
LineColumn {
line: 2,
column: 24,
},
))
),]
);
}

#[test]
fn test_hook_inside_hook_async_initialization() {
let contents = indoc! {r#"
fn use_thing() {
let _a = use_future(|| async move { use_signal(|| 0) });
}
"#};

let report = check_file("app.rs".into(), contents);

assert_eq!(
report.issues,
vec![
Issue::HookInsideAsync(
HookInfo::new(
Span::new_from_str(
"use_signal(|| 0)",
LineColumn {
line: 2,
column: 40,
},
),
Span::new_from_str(
"use_signal",
LineColumn {
line: 2,
column: 40,
},
),
"use_signal".to_string()
),
AsyncInfo::new(Span::new_from_str(
"async move { use_signal(|| 0) }",
LineColumn {
line: 2,
column: 27,
},
))
),
Issue::HookInsideClosure(
HookInfo::new(
Span::new_from_str(
"use_signal(|| 0)",
LineColumn {
line: 2,
column: 40,
},
),
Span::new_from_str(
"use_signal",
LineColumn {
line: 2,
column: 40,
},
),
"use_signal".to_string()
),
ClosureInfo::new(Span::new_from_str(
"|| async move { use_signal(|| 0) }",
LineColumn {
line: 2,
column: 24,
},
))
),
]
);
}

#[test]
fn test_hook_inside_spawn() {
let contents = indoc! {r#"
fn use_thing() {
let _a = spawn(async move { use_signal(|| 0) });
}
"#};

let report = check_file("app.rs".into(), contents);

assert_eq!(
report.issues,
vec![Issue::HookInsideAsync(
HookInfo::new(
Span::new_from_str(
"use_signal(|| 0)",
LineColumn {
line: 2,
column: 32,
},
),
Span::new_from_str(
"use_signal",
LineColumn {
line: 2,
column: 32,
},
),
"use_signal".to_string()
),
AsyncInfo::new(Span::new_from_str(
"async move { use_signal(|| 0) }",
LineColumn {
line: 2,
column: 19,
},
))
),]
);
}
}
12 changes: 10 additions & 2 deletions packages/check/src/issues.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@ use std::{
};

use crate::metadata::{
AnyLoopInfo, ClosureInfo, ConditionalInfo, ForInfo, HookInfo, IfInfo, MatchInfo, WhileInfo,
AnyLoopInfo, AsyncInfo, ClosureInfo, ConditionalInfo, ForInfo, HookInfo, IfInfo, MatchInfo,
WhileInfo,
};

/// The result of checking a Dioxus file for issues.
Expand Down Expand Up @@ -142,7 +143,9 @@ impl Display for IssueReport {
Issue::HookInsideLoop(_, AnyLoopInfo::Loop(_)) => {
writeln!(f, "{} `loop {{ … }}` is the loop", note_text_prefix,)?;
}
Issue::HookOutsideComponent(_) | Issue::HookInsideClosure(_, _) => {}
Issue::HookOutsideComponent(_)
| Issue::HookInsideClosure(_, _)
| Issue::HookInsideAsync(_, _) => {}
}

if i < self.issues.len() - 1 {
Expand All @@ -165,6 +168,7 @@ pub enum Issue {
HookInsideLoop(HookInfo, AnyLoopInfo),
/// <https://dioxuslabs.com/learn/0.5/reference/hooks#no-hooks-in-closures>
HookInsideClosure(HookInfo, ClosureInfo),
HookInsideAsync(HookInfo, AsyncInfo),
HookOutsideComponent(HookInfo),
}

Expand All @@ -174,6 +178,7 @@ impl Issue {
Issue::HookInsideConditional(hook_info, _)
| Issue::HookInsideLoop(hook_info, _)
| Issue::HookInsideClosure(hook_info, _)
| Issue::HookInsideAsync(hook_info, _)
| Issue::HookOutsideComponent(hook_info) => hook_info.clone(),
}
}
Expand Down Expand Up @@ -208,6 +213,9 @@ impl std::fmt::Display for Issue {
Issue::HookInsideClosure(hook_info, _) => {
write!(f, "hook called in a closure: `{}`", hook_info.name)
}
Issue::HookInsideAsync(hook_info, _) => {
write!(f, "hook called in an async block: `{}`", hook_info.name)
}
Issue::HookOutsideComponent(hook_info) => {
write!(
f,
Expand Down
12 changes: 12 additions & 0 deletions packages/check/src/metadata.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,18 @@ impl ClosureInfo {
}
}

#[derive(Debug, Clone, PartialEq, Eq)]
/// Information about an async block.
pub struct AsyncInfo {
pub span: Span,
}

impl AsyncInfo {
pub const fn new(span: Span) -> Self {
Self { span }
}
}

#[derive(Debug, Clone, PartialEq, Eq)]
/// Information about a component function.
pub struct ComponentInfo {
Expand Down
2 changes: 2 additions & 0 deletions packages/core/src/scope_context.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -424,6 +424,8 @@ impl Scope {
You likely used the hook in a conditional. Hooks rely on consistent ordering between renders.
Functions prefixed with "use" should never be called conditionally.
Help: Run `dx check` to look for check for some common hook errors.
"#,
)
}
Expand Down

0 comments on commit c11f2ed

Please sign in to comment.