diff --git a/nova_lint/Cargo.toml b/nova_lint/Cargo.toml index 8826497c1..0ef542acf 100644 --- a/nova_lint/Cargo.toml +++ b/nova_lint/Cargo.toml @@ -24,8 +24,12 @@ path = "ui/gc_scope_comes_last.rs" name = "gc_scope_is_only_passed_by_value" path = "ui/gc_scope_is_only_passed_by_value.rs" +[[example]] +name = "immediately_bind_scoped" +path = "ui/immediately_bind_scoped.rs" + [dependencies] -clippy_utils = { git = "https://github.com/rust-lang/rust-clippy", rev = "0450db33a5d8587f7c1d4b6d233dac963605766b" } +clippy_utils = { git = "https://github.com/rust-lang/rust-clippy", rev = "334fb906aef13d20050987b13448f37391bb97a2" } dylint_linting = { version = "4.1.0", features = ["constituent"] } [dev-dependencies] diff --git a/nova_lint/rust-toolchain.toml b/nova_lint/rust-toolchain.toml index 0f53877b3..07038b722 100644 --- a/nova_lint/rust-toolchain.toml +++ b/nova_lint/rust-toolchain.toml @@ -1,3 +1,3 @@ [toolchain] -channel = "nightly-2025-05-14" +channel = "nightly-2025-08-07" components = ["llvm-tools-preview", "rustc-dev"] diff --git a/nova_lint/src/agent_comes_first.rs b/nova_lint/src/agent_comes_first.rs index c930389ce..5f39d8ecc 100644 --- a/nova_lint/src/agent_comes_first.rs +++ b/nova_lint/src/agent_comes_first.rs @@ -1,5 +1,5 @@ use clippy_utils::{diagnostics::span_lint_and_help, is_self}; -use rustc_hir::{def_id::LocalDefId, intravisit::FnKind, Body, FnDecl}; +use rustc_hir::{Body, FnDecl, def_id::LocalDefId, intravisit::FnKind}; use rustc_lint::{LateContext, LateLintPass}; use rustc_span::Span; diff --git a/nova_lint/src/gc_scope_comes_last.rs b/nova_lint/src/gc_scope_comes_last.rs index ba422a23c..85f799eed 100644 --- a/nova_lint/src/gc_scope_comes_last.rs +++ b/nova_lint/src/gc_scope_comes_last.rs @@ -1,5 +1,5 @@ use clippy_utils::diagnostics::span_lint_and_help; -use rustc_hir::{def_id::LocalDefId, intravisit::FnKind, Body, FnDecl}; +use rustc_hir::{Body, FnDecl, def_id::LocalDefId, intravisit::FnKind}; use rustc_lint::{LateContext, LateLintPass}; use rustc_span::Span; diff --git a/nova_lint/src/gc_scope_is_only_passed_by_value.rs b/nova_lint/src/gc_scope_is_only_passed_by_value.rs index fb59027bd..7078c020e 100644 --- a/nova_lint/src/gc_scope_is_only_passed_by_value.rs +++ b/nova_lint/src/gc_scope_is_only_passed_by_value.rs @@ -1,5 +1,5 @@ use clippy_utils::{diagnostics::span_lint_and_help, is_self}; -use rustc_hir::{def_id::LocalDefId, intravisit::FnKind, Body, FnDecl}; +use rustc_hir::{Body, FnDecl, def_id::LocalDefId, intravisit::FnKind}; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::ty::TyKind; use rustc_span::Span; diff --git a/nova_lint/src/immediately_bind_scoped.rs b/nova_lint/src/immediately_bind_scoped.rs new file mode 100644 index 000000000..010f14ad9 --- /dev/null +++ b/nova_lint/src/immediately_bind_scoped.rs @@ -0,0 +1,226 @@ +use std::ops::ControlFlow; + +use crate::{is_scoped_ty, method_call}; +use clippy_utils::{ + diagnostics::span_lint_and_help, + get_enclosing_block, get_parent_expr, path_to_local_id, + paths::{PathNS, lookup_path_str}, + ty::implements_trait, + visitors::for_each_expr, +}; + +use rustc_hir::{Expr, ExprKind, HirId, Node, PatKind, StmtKind}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::ty::Ty; + +dylint_linting::declare_late_lint! { + /// ### What it does + /// + /// Makes sure that the user immediately binds `Scoped::get` results. + /// + /// ### Why is this bad? + /// + /// TODO: Write an explanation of why this is bad. + /// + /// ### Example + /// + /// ``` + /// let a = scoped_a.get(agent); + /// ``` + /// + /// Use instead: + /// + /// ``` + /// let a = scoped_a.get(agent).bind(gc.nogc()); + /// ``` + /// + /// Which ensures that no odd bugs occur. + /// + /// ### Exception: If the result is immediately used without assigning to a + /// variable, binding can be skipped. + /// + /// ``` + /// scoped_a.get(agent).internal_delete(agent, scoped_b.get(agent), gc.reborrow()); + /// ``` + /// + /// Here it is perfectly okay to skip the binding for both `scoped_a` and + /// `scoped_b` as the borrow checker would force you to again unbind both + /// `Value`s immediately. + pub IMMEDIATELY_BIND_SCOPED, + Deny, + "the result of `Scoped::get` should be immediately bound" +} + +impl<'tcx> LateLintPass<'tcx> for ImmediatelyBindScoped { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) { + // First we check if we have found a `Scoped::get` call + if is_scoped_get_method_call(cx, expr) { + // Which is followed by a trait method call to `bind` in which case + // it is all done properly and we can exit out of the lint. + if let Some(parent) = get_parent_expr(cx, expr) + && is_bindable_bind_method_call(cx, parent) + { + return; + } + + // Check if the unbound value is used in an argument position of a + // method or function call where binding can be safely skipped. + if is_in_argument_position(cx, expr) { + return; + } + + // If the expression is assigned to a local variable, we need to + // check that it's next use is binding or as a function argument. + if let Some(local_hir_id) = get_assigned_local(cx, expr) + && let Some(enclosing_block) = get_enclosing_block(cx, expr.hir_id) + { + let mut found_valid_next_use = false; + + // Look for the next use of this local after the current expression. + // We need to traverse the statements in the block to find proper usage + for stmt in enclosing_block + .stmts + .iter() + .skip_while(|s| s.span.lo() < expr.span.hi()) + { + // Extract relevant expressions from the statement and check + // it for a use valid of the local variable. + let Some(stmt_expr) = (match &stmt.kind { + StmtKind::Expr(expr) | StmtKind::Semi(expr) => Some(*expr), + StmtKind::Let(local) => local.init, + _ => None, + }) else { + continue; + }; + + // Check each expression in the current statement for use + // of the value, breaking when found and optionally marking + // it as valid. + if for_each_expr(cx, stmt_expr, |expr_in_stmt| { + if path_to_local_id(expr_in_stmt, local_hir_id) { + if is_valid_use_of_unbound_value(cx, expr_in_stmt, local_hir_id) { + found_valid_next_use = true; + } + + return ControlFlow::Break(true); + } + ControlFlow::Continue(()) + }) + .unwrap_or(false) + { + break; + } + } + + if !found_valid_next_use { + span_lint_and_help( + cx, + IMMEDIATELY_BIND_SCOPED, + expr.span, + "the result of `Scoped::get` should be immediately bound", + None, + "immediately bind the value", + ); + } + } + } + } +} + +/// Check if an expression is assigned to a local variable and return the local's HirId +fn get_assigned_local(cx: &LateContext<'_>, expr: &Expr) -> Option { + let parent_node = cx.tcx.parent_hir_id(expr.hir_id); + + if let Node::LetStmt(local) = cx.tcx.hir_node(parent_node) + && let Some(init) = local.init + && init.hir_id == expr.hir_id + && let PatKind::Binding(_, hir_id, _, _) = local.pat.kind + { + Some(hir_id) + } else { + None + } +} + +/// Check if a use of an unbound value is valid (binding or function argument) +fn is_valid_use_of_unbound_value(cx: &LateContext<'_>, expr: &Expr, hir_id: HirId) -> bool { + // Check if we're in a method call and if so, check if it's a bind call + if let Some(parent) = get_parent_expr(cx, expr) + && is_bindable_bind_method_call(cx, parent) + { + return true; + } + + // If this is a method call to bind() on our local, it's valid + if is_bindable_bind_method_call(cx, expr) { + return true; + } + + // If this is the local being used as a function argument, it's valid + if path_to_local_id(expr, hir_id) && is_in_argument_position(cx, expr) { + return true; + } + + false +} + +/// Check if an expression is in an argument position where binding can be skipped +fn is_in_argument_position(cx: &LateContext<'_>, expr: &Expr) -> bool { + let mut current_expr = expr; + + // Walk up the parent chain to see if we're in a function call argument + while let Some(parent) = get_parent_expr(cx, current_expr) { + match parent.kind { + // If we find a method call where our expression is an argument (not receiver) + ExprKind::MethodCall(_, receiver, args, _) => { + if receiver.hir_id != current_expr.hir_id + && args.iter().any(|arg| arg.hir_id == current_expr.hir_id) + { + return true; + } + } + // If we find a function call where our expression is an argument + ExprKind::Call(_, args) => { + if args.iter().any(|arg| arg.hir_id == current_expr.hir_id) { + return true; + } + } + // Continue walking up for other expression types + _ => {} + } + current_expr = parent; + } + + false +} + +fn is_scoped_get_method_call(cx: &LateContext<'_>, expr: &Expr) -> bool { + if let Some((method, recv, _, _, _)) = method_call(expr) + && method == "get" + && let typeck_results = cx.typeck_results() + && let recv_ty = typeck_results.expr_ty(recv) + && is_scoped_ty(cx, &recv_ty) + { + true + } else { + false + } +} + +fn is_bindable_bind_method_call(cx: &LateContext<'_>, expr: &Expr) -> bool { + if let Some((method, _, _, _, _)) = method_call(expr) + && method == "bind" + && let expr_ty = cx.typeck_results().expr_ty(expr) + && implements_bindable_trait(cx, &expr_ty) + { + true + } else { + false + } +} + +fn implements_bindable_trait<'tcx>(cx: &LateContext<'tcx>, ty: &Ty<'tcx>) -> bool { + lookup_path_str(cx.tcx, PathNS::Type, "nova_vm::engine::context::Bindable") + .first() + .is_some_and(|&trait_def_id| implements_trait(cx, *ty, trait_def_id, &[])) +} diff --git a/nova_lint/src/lib.rs b/nova_lint/src/lib.rs index 6d40617c3..864448189 100644 --- a/nova_lint/src/lib.rs +++ b/nova_lint/src/lib.rs @@ -25,6 +25,7 @@ extern crate rustc_trait_selection; mod agent_comes_first; mod gc_scope_comes_last; mod gc_scope_is_only_passed_by_value; +mod immediately_bind_scoped; mod utils; pub(crate) use utils::*; @@ -34,6 +35,7 @@ pub fn register_lints(sess: &rustc_session::Session, lint_store: &mut rustc_lint agent_comes_first::register_lints(sess, lint_store); gc_scope_comes_last::register_lints(sess, lint_store); gc_scope_is_only_passed_by_value::register_lints(sess, lint_store); + immediately_bind_scoped::register_lints(sess, lint_store); } #[test] diff --git a/nova_lint/src/utils.rs b/nova_lint/src/utils.rs index 073f64a0b..b52548082 100644 --- a/nova_lint/src/utils.rs +++ b/nova_lint/src/utils.rs @@ -1,7 +1,7 @@ -use rustc_hir::def_id::DefId; +use rustc_hir::{Expr, ExprKind, def_id::DefId}; use rustc_lint::LateContext; use rustc_middle::ty::{Ty, TyKind}; -use rustc_span::symbol::Symbol; +use rustc_span::{Span, symbol::Symbol}; // Copyright (c) 2014-2025 The Rust Project Developers // @@ -19,6 +19,25 @@ pub fn match_def_path(cx: &LateContext<'_>, did: DefId, syms: &[&str]) -> bool { .eq(path.iter().copied()) } +// Copyright (c) 2014-2025 The Rust Project Developers +// +// Originally copied from `dylint` which in turn copied it from `clippy_lints`: +// - https://github.com/trailofbits/dylint/blob/d1be1c42f363ca11f8ebce0ff0797ecbbcc3680b/examples/restriction/collapsible_unwrap/src/lib.rs#L180 +// - https://github.com/rust-lang/rust-clippy/blob/3f015a363020d3811e1f028c9ce4b0705c728289/clippy_lints/src/methods/mod.rs#L3293-L3304 +/// Extracts a method call name, args, and `Span` of the method name. +pub fn method_call<'tcx>( + recv: &'tcx Expr<'tcx>, +) -> Option<(&'tcx str, &'tcx Expr<'tcx>, &'tcx [Expr<'tcx>], Span, Span)> { + if let ExprKind::MethodCall(path, receiver, args, call_span) = recv.kind + && !args.iter().any(|e| e.span.from_expansion()) + && !receiver.span.from_expansion() + { + let name = path.ident.name.as_str(); + return Some((name, receiver, args, path.ident.span, call_span)); + } + None +} + pub fn is_param_ty(ty: &Ty) -> bool { matches!(ty.kind(), TyKind::Param(_)) } @@ -53,3 +72,14 @@ pub fn is_no_gc_scope_ty(cx: &LateContext<'_>, ty: &Ty) -> bool { _ => false, } } + +pub fn is_scoped_ty(cx: &LateContext<'_>, ty: &Ty) -> bool { + match ty.kind() { + TyKind::Adt(def, _) => match_def_path( + cx, + def.did(), + &["nova_vm", "engine", "rootable", "scoped", "Scoped"], + ), + _ => false, + } +} diff --git a/nova_lint/ui/immediately_bind_scoped.rs b/nova_lint/ui/immediately_bind_scoped.rs new file mode 100644 index 000000000..372991f24 --- /dev/null +++ b/nova_lint/ui/immediately_bind_scoped.rs @@ -0,0 +1,60 @@ +#![allow(dead_code, unused_variables, clippy::disallowed_names)] + +use nova_vm::{ + ecmascript::{execution::Agent, types::Value}, + engine::{ + Scoped, + context::{Bindable, NoGcScope}, + }, +}; + +fn test_scoped_get_is_immediately_bound(agent: &Agent, scoped: Scoped, gc: NoGcScope) { + let _a = scoped.get(agent).bind(gc); +} + +fn test_scoped_get_can_get_bound_right_after(agent: &Agent, scoped: Scoped, gc: NoGcScope) { + let a = scoped.get(agent); + a.bind(gc); +} + +fn test_scoped_get_can_get_bound_in_tuple_right_after( + agent: &Agent, + scoped: Scoped, + gc: NoGcScope, +) { + let a = scoped.get(agent); + (a.bind(gc), ()); +} + +fn test_scoped_get_can_be_immediately_passed_on( + agent: &Agent, + scoped: Scoped, + gc: NoGcScope, +) { + let a = scoped.get(agent); + test_consumes_unbound_value(a); +} + +fn test_consumes_unbound_value(value: Value) { + unimplemented!() +} + +fn test_scoped_get_is_not_immediately_bound(agent: &Agent, scoped: Scoped) { + let _a = scoped.get(agent); +} + +fn test_scoped_get_doesnt_need_to_be_bound_if_not_assigned(agent: &Agent, scoped: Scoped) { + scoped.get(agent); +} + +fn test_improbable_but_technically_bad_situation( + agent: &Agent, + scoped: Scoped, + gc: NoGcScope, +) { + let _a = Scoped::new(agent, Value::Undefined, gc).get(agent); +} + +fn main() { + unimplemented!() +} diff --git a/nova_lint/ui/immediately_bind_scoped.stderr b/nova_lint/ui/immediately_bind_scoped.stderr new file mode 100644 index 000000000..f38b2e398 --- /dev/null +++ b/nova_lint/ui/immediately_bind_scoped.stderr @@ -0,0 +1,19 @@ +error: the result of `Scoped::get` should be immediately bound + --> $DIR/immediately_bind_scoped.rs:43:14 + | +LL | let _a = scoped.get(agent); + | ^^^^^^^^^^^^^^^^^ + | + = help: immediately bind the value + = note: `#[deny(immediately_bind_scoped)]` on by default + +error: the result of `Scoped::get` should be immediately bound + --> $DIR/immediately_bind_scoped.rs:55:14 + | +LL | let _a = Scoped::new(agent, Value::Undefined, gc).get(agent); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: immediately bind the value + +error: aborting due to 2 previous errors +