From 8a32a1adc708df02e61aaa76ae607ee9bd10520f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Elias=20Sj=C3=B6green?= Date: Fri, 19 Sep 2025 00:36:07 +0200 Subject: [PATCH 1/9] feat(linter): Implement `immediately_bind_scoped` rule --- nova_lint/Cargo.toml | 4 + nova_lint/src/immediately_bind_scoped.rs | 103 ++++++++++++++++++++ nova_lint/src/lib.rs | 2 + nova_lint/src/utils.rs | 58 ++++++++++- nova_lint/ui/immediately_bind_scoped.rs | 63 ++++++++++++ nova_lint/ui/immediately_bind_scoped.stderr | 19 ++++ 6 files changed, 248 insertions(+), 1 deletion(-) create mode 100644 nova_lint/src/immediately_bind_scoped.rs create mode 100644 nova_lint/ui/immediately_bind_scoped.rs create mode 100644 nova_lint/ui/immediately_bind_scoped.stderr diff --git a/nova_lint/Cargo.toml b/nova_lint/Cargo.toml index 17649db46..a49513034 100644 --- a/nova_lint/Cargo.toml +++ b/nova_lint/Cargo.toml @@ -24,6 +24,10 @@ 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" + [[example]] name = "no_it_performs_the_following" path = "ui/no_it_performs_the_following.rs" diff --git a/nova_lint/src/immediately_bind_scoped.rs b/nova_lint/src/immediately_bind_scoped.rs new file mode 100644 index 000000000..48a70b8a9 --- /dev/null +++ b/nova_lint/src/immediately_bind_scoped.rs @@ -0,0 +1,103 @@ +use clippy_utils::paths::{PathLookup, PathNS, lookup_path_str}; +use clippy_utils::sym::Symbol; +use clippy_utils::ty::implements_trait; +use clippy_utils::usage::local_used_after_expr; +use clippy_utils::{diagnostics::span_lint_and_help, is_self}; +use clippy_utils::{ + get_expr_use_or_unification_node, get_parent_expr, is_expr_final_block_expr, is_trait_method, + path_def_id, peel_blocks, potential_return_of_enclosing_body, +}; +use rustc_hir::{Body, FnDecl, def_id::LocalDefId, intravisit::FnKind}; +use rustc_hir::{Expr, ExprKind, LetStmt, Node}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_span::symbol::Symbol; + +use crate::{is_scoped_ty, method_call}; + +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 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) + { + // 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) + && let Some((parent_method, _, _, _, _)) = method_call(parent) + && parent_method == "bind" + && let parent_ty = typeck_results.expr_ty(parent) + && let Some(&trait_def_id) = + lookup_path_str(cx.tcx, PathNS::Type, "nova_vm::engine::context::Bindable") + .first() + && implements_trait(cx, parent_ty, trait_def_id, &[]) + { + return; + } + + // Now we are onto something! If the expression is returned, used + // after the expression or assigned to a variable we might have + // found an issue. + if let Some((usage, hir_id)) = get_expr_use_or_unification_node(cx.tcx, expr) + && (potential_return_of_enclosing_body(cx, expr) + || local_used_after_expr(cx, hir_id, expr) + || matches!( + usage, + Node::LetStmt(LetStmt { + init: Some(hir_id), + .. + }) + )) + { + span_lint_and_help( + cx, + IMMEDIATELY_BIND_SCOPED, + expr.span, + "the result of `Scoped::get` should be immediately bound", + None, + "immediately bind the value", + ); + } + } + } +} diff --git a/nova_lint/src/lib.rs b/nova_lint/src/lib.rs index 3976e9879..ed6b09d84 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 no_it_performs_the_following; mod no_multipage_spec; mod spec_header_level; @@ -37,6 +38,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); no_it_performs_the_following::register_lints(sess, lint_store); no_multipage_spec::register_lints(sess, lint_store); spec_header_level::register_lints(sess, lint_store); diff --git a/nova_lint/src/utils.rs b/nova_lint/src/utils.rs index 073f64a0b..4200c8078 100644 --- a/nova_lint/src/utils.rs +++ b/nova_lint/src/utils.rs @@ -1,6 +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::Span; use rustc_span::symbol::Symbol; // Copyright (c) 2014-2025 The Rust Project Developers @@ -19,6 +20,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 +73,39 @@ 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, + } +} + +// Checks if a given expression is assigned to a variable. +// +// Copyright (c) 2014-2025 The Rust Project Developers +// +// Copied and modified from `clippy_utils`: +// - https://github.com/rust-lang/rust-clippy/blob/8a5dc7c1713a7eb9af28bf9f53dc6b61da7aad90/clippy_utils/src/lib.rs#L1369-L1388 +// pub fn is_inside_let(tcx: TyCtxt<'_>, expr: &Expr<'_>) -> bool { +// let mut child_id = expr.hir_id; +// for (parent_id, node) in tcx.hir_parent_iter(child_id) { +// if let Node::LetStmt(LetStmt { +// init: Some(init), +// els: Some(els), +// .. +// }) = node +// && (init.hir_id == child_id || els.hir_id == child_id) +// { +// return true; +// } + +// child_id = parent_id; +// } + +// 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..fce71c9b3 --- /dev/null +++ b/nova_lint/ui/immediately_bind_scoped.rs @@ -0,0 +1,63 @@ +#![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); +} + +// TODO: These are valid patterns, which are found in certain parts of the +// codebase so should ideally be implemented. +// 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_right_after_and_never_used_again( +// agent: &Agent, +// scoped: Scoped, +// gc: NoGcScope, +// ) { +// let a = scoped.get(agent); +// let b = a.bind(gc); +// a; +// } +// +// fn test_scoped_get_can_be_immediately_passed_on( +// agent: &Agent, +// scoped: Scoped, +// gc: NoGcScope, +// ) { +// let a = scoped.get(agent); +// test_consumes_unbound_value(value); +// } +// +// 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..53ff97d5c --- /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:16: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:24:14 + | +LL | let _a = Scoped::new(agent, Value::Undefined, gc).get(agent); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: immediately bind the value + +error: aborting due to 2 previous errors + From 593d4eab642d47e07597a070e200659ca90afa25 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Elias=20Sj=C3=B6green?= Date: Fri, 19 Sep 2025 00:52:50 +0200 Subject: [PATCH 2/9] fix: Test expectations --- nova_lint/ui/immediately_bind_scoped.stderr | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/nova_lint/ui/immediately_bind_scoped.stderr b/nova_lint/ui/immediately_bind_scoped.stderr index 53ff97d5c..feeb517dd 100644 --- a/nova_lint/ui/immediately_bind_scoped.stderr +++ b/nova_lint/ui/immediately_bind_scoped.stderr @@ -1,5 +1,5 @@ error: the result of `Scoped::get` should be immediately bound - --> $DIR/immediately_bind_scoped.rs:16:14 + --> $DIR/immediately_bind_scoped.rs:46:14 | LL | let _a = scoped.get(agent); | ^^^^^^^^^^^^^^^^^ @@ -8,7 +8,7 @@ LL | let _a = scoped.get(agent); = note: `#[deny(immediately_bind_scoped)]` on by default error: the result of `Scoped::get` should be immediately bound - --> $DIR/immediately_bind_scoped.rs:24:14 + --> $DIR/immediately_bind_scoped.rs:58:14 | LL | let _a = Scoped::new(agent, Value::Undefined, gc).get(agent); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ From d1699b2f183cc700500df3698aaab69681f868ce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Elias=20Sj=C3=B6green?= Date: Fri, 19 Sep 2025 00:59:44 +0200 Subject: [PATCH 3/9] fix: Clippy --- nova_lint/src/immediately_bind_scoped.rs | 20 +++++--------------- 1 file changed, 5 insertions(+), 15 deletions(-) diff --git a/nova_lint/src/immediately_bind_scoped.rs b/nova_lint/src/immediately_bind_scoped.rs index 48a70b8a9..1b99043e4 100644 --- a/nova_lint/src/immediately_bind_scoped.rs +++ b/nova_lint/src/immediately_bind_scoped.rs @@ -1,16 +1,12 @@ -use clippy_utils::paths::{PathLookup, PathNS, lookup_path_str}; -use clippy_utils::sym::Symbol; +use clippy_utils::paths::{PathNS, lookup_path_str}; use clippy_utils::ty::implements_trait; use clippy_utils::usage::local_used_after_expr; -use clippy_utils::{diagnostics::span_lint_and_help, is_self}; +use clippy_utils::diagnostics::span_lint_and_help; use clippy_utils::{ - get_expr_use_or_unification_node, get_parent_expr, is_expr_final_block_expr, is_trait_method, - path_def_id, peel_blocks, potential_return_of_enclosing_body, + get_expr_use_or_unification_node, get_parent_expr, potential_return_of_enclosing_body, }; -use rustc_hir::{Body, FnDecl, def_id::LocalDefId, intravisit::FnKind}; -use rustc_hir::{Expr, ExprKind, LetStmt, Node}; +use rustc_hir::{Expr, Node}; use rustc_lint::{LateContext, LateLintPass}; -use rustc_span::symbol::Symbol; use crate::{is_scoped_ty, method_call}; @@ -81,13 +77,7 @@ impl<'tcx> LateLintPass<'tcx> for ImmediatelyBindScoped { if let Some((usage, hir_id)) = get_expr_use_or_unification_node(cx.tcx, expr) && (potential_return_of_enclosing_body(cx, expr) || local_used_after_expr(cx, hir_id, expr) - || matches!( - usage, - Node::LetStmt(LetStmt { - init: Some(hir_id), - .. - }) - )) + || matches!(usage, Node::LetStmt(_))) { span_lint_and_help( cx, From edc15fba0ff679c1ebfaf1a8c2241ae6f6392903 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Elias=20Sj=C3=B6green?= Date: Fri, 19 Sep 2025 11:35:14 +0200 Subject: [PATCH 4/9] chore: Clean up, organize, think, and start work on the edge-cases --- nova_lint/src/agent_comes_first.rs | 2 +- nova_lint/src/gc_scope_comes_last.rs | 2 +- .../src/gc_scope_is_only_passed_by_value.rs | 2 +- nova_lint/src/immediately_bind_scoped.rs | 84 ++++++++++++++----- nova_lint/src/utils.rs | 28 +------ nova_lint/ui/immediately_bind_scoped.rs | 56 ++++++------- 6 files changed, 95 insertions(+), 79 deletions(-) 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 index 1b99043e4..b37485bd2 100644 --- a/nova_lint/src/immediately_bind_scoped.rs +++ b/nova_lint/src/immediately_bind_scoped.rs @@ -1,14 +1,15 @@ -use clippy_utils::paths::{PathNS, lookup_path_str}; -use clippy_utils::ty::implements_trait; -use clippy_utils::usage::local_used_after_expr; -use clippy_utils::diagnostics::span_lint_and_help; +use crate::{is_scoped_ty, method_call}; use clippy_utils::{ - get_expr_use_or_unification_node, get_parent_expr, potential_return_of_enclosing_body, + diagnostics::span_lint_and_help, + get_expr_use_or_unification_node, get_parent_expr, + paths::{PathNS, lookup_path_str}, + potential_return_of_enclosing_body, + ty::implements_trait, + usage::local_used_after_expr, }; use rustc_hir::{Expr, Node}; use rustc_lint::{LateContext, LateLintPass}; - -use crate::{is_scoped_ty, method_call}; +use rustc_middle::ty::Ty; dylint_linting::declare_late_lint! { /// ### What it does @@ -51,26 +52,26 @@ dylint_linting::declare_late_lint! { 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 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) - { + 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) - && let Some((parent_method, _, _, _, _)) = method_call(parent) - && parent_method == "bind" - && let parent_ty = typeck_results.expr_ty(parent) - && let Some(&trait_def_id) = - lookup_path_str(cx.tcx, PathNS::Type, "nova_vm::engine::context::Bindable") - .first() - && implements_trait(cx, parent_ty, trait_def_id, &[]) + && is_bindable_bind_method_call(cx, parent) { return; } + // If the `Scoped::get` call is never used or unified we can + // safely exit out of the rule, otherwise we need to look into how + // it's used. + let Some((usage, hir_id)) = get_expr_use_or_unification_node(cx.tcx, expr) else { + return; + }; + + if !local_used_after_expr(cx, hir_id, expr) { + return; + } + // Now we are onto something! If the expression is returned, used // after the expression or assigned to a variable we might have // found an issue. @@ -91,3 +92,46 @@ impl<'tcx> LateLintPass<'tcx> for ImmediatelyBindScoped { } } } + +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 is_bindable_unbind_method_call(cx: &LateContext<'_>, expr: &Expr) -> bool { + if let Some((method, _, _, _, _)) = method_call(expr) + && method == "unbind" + && 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/utils.rs b/nova_lint/src/utils.rs index 4200c8078..b52548082 100644 --- a/nova_lint/src/utils.rs +++ b/nova_lint/src/utils.rs @@ -1,8 +1,7 @@ use rustc_hir::{Expr, ExprKind, def_id::DefId}; use rustc_lint::LateContext; use rustc_middle::ty::{Ty, TyKind}; -use rustc_span::Span; -use rustc_span::symbol::Symbol; +use rustc_span::{Span, symbol::Symbol}; // Copyright (c) 2014-2025 The Rust Project Developers // @@ -84,28 +83,3 @@ pub fn is_scoped_ty(cx: &LateContext<'_>, ty: &Ty) -> bool { _ => false, } } - -// Checks if a given expression is assigned to a variable. -// -// Copyright (c) 2014-2025 The Rust Project Developers -// -// Copied and modified from `clippy_utils`: -// - https://github.com/rust-lang/rust-clippy/blob/8a5dc7c1713a7eb9af28bf9f53dc6b61da7aad90/clippy_utils/src/lib.rs#L1369-L1388 -// pub fn is_inside_let(tcx: TyCtxt<'_>, expr: &Expr<'_>) -> bool { -// let mut child_id = expr.hir_id; -// for (parent_id, node) in tcx.hir_parent_iter(child_id) { -// if let Node::LetStmt(LetStmt { -// init: Some(init), -// els: Some(els), -// .. -// }) = node -// && (init.hir_id == child_id || els.hir_id == child_id) -// { -// return true; -// } - -// child_id = parent_id; -// } - -// false -// } diff --git a/nova_lint/ui/immediately_bind_scoped.rs b/nova_lint/ui/immediately_bind_scoped.rs index fce71c9b3..73a05d2f6 100644 --- a/nova_lint/ui/immediately_bind_scoped.rs +++ b/nova_lint/ui/immediately_bind_scoped.rs @@ -12,35 +12,33 @@ fn test_scoped_get_is_immediately_bound(agent: &Agent, scoped: Scoped, gc let _a = scoped.get(agent).bind(gc); } -// TODO: These are valid patterns, which are found in certain parts of the -// codebase so should ideally be implemented. -// 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_right_after_and_never_used_again( -// agent: &Agent, -// scoped: Scoped, -// gc: NoGcScope, -// ) { -// let a = scoped.get(agent); -// let b = a.bind(gc); -// a; -// } -// -// fn test_scoped_get_can_be_immediately_passed_on( -// agent: &Agent, -// scoped: Scoped, -// gc: NoGcScope, -// ) { -// let a = scoped.get(agent); -// test_consumes_unbound_value(value); -// } -// -// fn test_consumes_unbound_value(value: Value) { -// unimplemented!() -// } +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_right_after_but_never_used_again( + agent: &Agent, + scoped: Scoped, + gc: NoGcScope, +) { + let a = scoped.get(agent); + let b = a.bind(gc); + a.is_undefined(); +} + +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.unbind()); +} + +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); From fa1a9026eae95b4b58c75c03b0c88ec313752a56 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Elias=20Sj=C3=B6green?= Date: Fri, 19 Sep 2025 11:41:08 +0200 Subject: [PATCH 5/9] fix: Update test expectations --- nova_lint/ui/immediately_bind_scoped.rs | 2 +- nova_lint/ui/immediately_bind_scoped.stderr | 16 +++++++++++----- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/nova_lint/ui/immediately_bind_scoped.rs b/nova_lint/ui/immediately_bind_scoped.rs index 73a05d2f6..66027a63c 100644 --- a/nova_lint/ui/immediately_bind_scoped.rs +++ b/nova_lint/ui/immediately_bind_scoped.rs @@ -33,7 +33,7 @@ fn test_scoped_get_can_be_immediately_passed_on( gc: NoGcScope, ) { let a = scoped.get(agent); - test_consumes_unbound_value(a.unbind()); + test_consumes_unbound_value(a); } fn test_consumes_unbound_value(value: Value) { diff --git a/nova_lint/ui/immediately_bind_scoped.stderr b/nova_lint/ui/immediately_bind_scoped.stderr index feeb517dd..91b65f9df 100644 --- a/nova_lint/ui/immediately_bind_scoped.stderr +++ b/nova_lint/ui/immediately_bind_scoped.stderr @@ -1,19 +1,25 @@ error: the result of `Scoped::get` should be immediately bound - --> $DIR/immediately_bind_scoped.rs:46:14 + --> $DIR/immediately_bind_scoped.rs:25:13 + | +LL | let a = scoped.get(agent); + | ^^^^^^^^^^^^^^^^^ + | + = help: immediately bind the value + +error: the result of `Scoped::get` should be immediately bound + --> $DIR/immediately_bind_scoped.rs:44: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:58:14 + --> $DIR/immediately_bind_scoped.rs:56:14 | LL | let _a = Scoped::new(agent, Value::Undefined, gc).get(agent); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = help: immediately bind the value -error: aborting due to 2 previous errors - +error: aborting due to 3 previous errors From 145923cd40d1036c3585fa1d7ce97a96cd6c3dc7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Elias=20Sj=C3=B6green?= Date: Sun, 21 Sep 2025 00:07:11 +0200 Subject: [PATCH 6/9] fix(linter): Make sure the immediate next use of the scoped variable is either as a binding or as a parameter to a method call --- nova_lint/src/immediately_bind_scoped.rs | 169 +++++++++++++++----- nova_lint/ui/immediately_bind_scoped.rs | 5 +- nova_lint/ui/immediately_bind_scoped.stderr | 16 +- 3 files changed, 136 insertions(+), 54 deletions(-) diff --git a/nova_lint/src/immediately_bind_scoped.rs b/nova_lint/src/immediately_bind_scoped.rs index b37485bd2..010f14ad9 100644 --- a/nova_lint/src/immediately_bind_scoped.rs +++ b/nova_lint/src/immediately_bind_scoped.rs @@ -1,13 +1,15 @@ +use std::ops::ControlFlow; + use crate::{is_scoped_ty, method_call}; use clippy_utils::{ diagnostics::span_lint_and_help, - get_expr_use_or_unification_node, get_parent_expr, + get_enclosing_block, get_parent_expr, path_to_local_id, paths::{PathNS, lookup_path_str}, - potential_return_of_enclosing_body, ty::implements_trait, - usage::local_used_after_expr, + visitors::for_each_expr, }; -use rustc_hir::{Expr, Node}; + +use rustc_hir::{Expr, ExprKind, HirId, Node, PatKind, StmtKind}; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::ty::Ty; @@ -54,45 +56,144 @@ impl<'tcx> LateLintPass<'tcx> for ImmediatelyBindScoped { // 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 + // 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; } - // If the `Scoped::get` call is never used or unified we can - // safely exit out of the rule, otherwise we need to look into how - // it's used. - let Some((usage, hir_id)) = get_expr_use_or_unification_node(cx.tcx, expr) else { - return; - }; - - if !local_used_after_expr(cx, hir_id, expr) { + // 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; } - // Now we are onto something! If the expression is returned, used - // after the expression or assigned to a variable we might have - // found an issue. - if let Some((usage, hir_id)) = get_expr_use_or_unification_node(cx.tcx, expr) - && (potential_return_of_enclosing_body(cx, expr) - || local_used_after_expr(cx, hir_id, expr) - || matches!(usage, Node::LetStmt(_))) + // 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) { - span_lint_and_help( - cx, - IMMEDIATELY_BIND_SCOPED, - expr.span, - "the result of `Scoped::get` should be immediately bound", - None, - "immediately bind the value", - ); + 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" @@ -118,18 +219,6 @@ fn is_bindable_bind_method_call(cx: &LateContext<'_>, expr: &Expr) -> bool { } } -fn is_bindable_unbind_method_call(cx: &LateContext<'_>, expr: &Expr) -> bool { - if let Some((method, _, _, _, _)) = method_call(expr) - && method == "unbind" - && 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() diff --git a/nova_lint/ui/immediately_bind_scoped.rs b/nova_lint/ui/immediately_bind_scoped.rs index 66027a63c..372991f24 100644 --- a/nova_lint/ui/immediately_bind_scoped.rs +++ b/nova_lint/ui/immediately_bind_scoped.rs @@ -17,14 +17,13 @@ fn test_scoped_get_can_get_bound_right_after(agent: &Agent, scoped: Scoped, gc: NoGcScope, ) { let a = scoped.get(agent); - let b = a.bind(gc); - a.is_undefined(); + (a.bind(gc), ()); } fn test_scoped_get_can_be_immediately_passed_on( diff --git a/nova_lint/ui/immediately_bind_scoped.stderr b/nova_lint/ui/immediately_bind_scoped.stderr index 91b65f9df..f38b2e398 100644 --- a/nova_lint/ui/immediately_bind_scoped.stderr +++ b/nova_lint/ui/immediately_bind_scoped.stderr @@ -1,25 +1,19 @@ error: the result of `Scoped::get` should be immediately bound - --> $DIR/immediately_bind_scoped.rs:25:13 - | -LL | let a = scoped.get(agent); - | ^^^^^^^^^^^^^^^^^ - | - = help: immediately bind the value - -error: the result of `Scoped::get` should be immediately bound - --> $DIR/immediately_bind_scoped.rs:44:14 + --> $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:56:14 + --> $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 3 previous errors +error: aborting due to 2 previous errors + From ca6ef18d87dd05c89f7f3ee7725a89f74e511330 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Elias=20Sj=C3=B6green?= Date: Tue, 18 Nov 2025 20:36:12 +0100 Subject: [PATCH 7/9] fix: Allow use as self in method call after unbinding --- nova_lint/src/immediately_bind_scoped.rs | 26 ++++++++++++++ nova_lint/ui/immediately_bind_scoped.rs | 39 ++++++++++++++++++++- nova_lint/ui/immediately_bind_scoped.stderr | 4 +-- 3 files changed, 66 insertions(+), 3 deletions(-) diff --git a/nova_lint/src/immediately_bind_scoped.rs b/nova_lint/src/immediately_bind_scoped.rs index 010f14ad9..bfd32f937 100644 --- a/nova_lint/src/immediately_bind_scoped.rs +++ b/nova_lint/src/immediately_bind_scoped.rs @@ -161,6 +161,32 @@ fn is_valid_use_of_unbound_value(cx: &LateContext<'_>, expr: &Expr, hir_id: HirI return true; } + // If this is the self value of a method call, it's valid + if path_to_local_id(expr, hir_id) && is_in_self_position(cx, expr) { + return true; + } + + false +} + +fn is_in_self_position(cx: &LateContext<'_>, expr: &Expr) -> bool { + let mut current_expr = expr; + + // Walk up the parent chain to see if we're in a method call + while let Some(parent) = get_parent_expr(cx, current_expr) { + match parent.kind { + // If we find a method call where our expression is in the receiver position + ExprKind::MethodCall(_, receiver, args, _) => { + if receiver.hir_id == current_expr.hir_id { + return true; + } + } + // Continue walking up for other expression types + _ => {} + } + current_expr = parent; + } + false } diff --git a/nova_lint/ui/immediately_bind_scoped.rs b/nova_lint/ui/immediately_bind_scoped.rs index 372991f24..3f9d8285c 100644 --- a/nova_lint/ui/immediately_bind_scoped.rs +++ b/nova_lint/ui/immediately_bind_scoped.rs @@ -3,8 +3,8 @@ use nova_vm::{ ecmascript::{execution::Agent, types::Value}, engine::{ + context::{Bindable, GcScope, NoGcScope}, Scoped, - context::{Bindable, NoGcScope}, }, }; @@ -35,6 +35,19 @@ fn test_scoped_get_can_be_immediately_passed_on( test_consumes_unbound_value(a); } +fn test_scoped_get_can_be_used_as_self(agent: &Agent, scoped: Scoped) { + scoped.get(agent).is_undefined(); +} + +fn test_scoped_get_can_be_used_as_self_immediately_after( + agent: &Agent, + scoped: Scoped, + gc: NoGcScope, +) { + let a = scoped.get(agent); + a.is_undefined(); +} + fn test_consumes_unbound_value(value: Value) { unimplemented!() } @@ -55,6 +68,30 @@ fn test_improbable_but_technically_bad_situation( let _a = Scoped::new(agent, Value::Undefined, gc).get(agent); } +// fn take_and_return_value_with_gc<'gc>( +// agent: &mut Agent, +// value: Value, +// mut gc: GcScope<'gc, '_>, +// ) -> Value<'gc> { +// Value::Undefined +// } + +// fn test_scoped_used_twice_right_after<'gc>( +// agent: &mut Agent, +// value: Scoped, +// mut gc: GcScope<'gc, '_>, +// ) { +// let value = value.get(agent); +// let something = if value.is_undefined() { +// true +// } else { +// take_and_return_value_with_gc(agent, value, gc.reborrow()) +// .unbind() +// .bind(gc.nogc()) +// .is_undefined() +// }; +// } + fn main() { unimplemented!() } diff --git a/nova_lint/ui/immediately_bind_scoped.stderr b/nova_lint/ui/immediately_bind_scoped.stderr index f38b2e398..49dfc28ff 100644 --- a/nova_lint/ui/immediately_bind_scoped.stderr +++ b/nova_lint/ui/immediately_bind_scoped.stderr @@ -1,5 +1,5 @@ error: the result of `Scoped::get` should be immediately bound - --> $DIR/immediately_bind_scoped.rs:43:14 + --> $DIR/immediately_bind_scoped.rs:56:14 | LL | let _a = scoped.get(agent); | ^^^^^^^^^^^^^^^^^ @@ -8,7 +8,7 @@ LL | let _a = scoped.get(agent); = 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 + --> $DIR/immediately_bind_scoped.rs:68:14 | LL | let _a = Scoped::new(agent, Value::Undefined, gc).get(agent); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ From eafee715355098ac12bb58de1e4b616990368654 Mon Sep 17 00:00:00 2001 From: Aapo Alasuutari Date: Sat, 13 Dec 2025 21:43:09 +0200 Subject: [PATCH 8/9] hopefully fix lints --- .../testing_and_comparison.rs | 40 +++++++++++++------ .../array_objects/array_prototype.rs | 10 +++-- .../typed_array_intrinsic_object.rs | 4 +- nova_vm/src/ecmascript/builtins/ordinary.rs | 26 ++++++------ .../environments/global_environment.rs | 9 +++-- 5 files changed, 53 insertions(+), 36 deletions(-) diff --git a/nova_vm/src/ecmascript/abstract_operations/testing_and_comparison.rs b/nova_vm/src/ecmascript/abstract_operations/testing_and_comparison.rs index d582a46e7..c8f7f79bf 100644 --- a/nova_vm/src/ecmascript/abstract_operations/testing_and_comparison.rs +++ b/nova_vm/src/ecmascript/abstract_operations/testing_and_comparison.rs @@ -365,23 +365,28 @@ pub(crate) fn is_less_than<'a, const LEFT_FIRST: bool>( let (px, py, gc) = match (Primitive::try_from(x.into()), Primitive::try_from(y.into())) { (Ok(px), Ok(py)) => { let gc = gc.into_nogc(); - (px.bind(gc), py.bind(gc), gc) + let px = px.bind(gc); + let py = py.bind(gc); + (px, py, gc) } (Ok(px), Err(_)) => { let px = px.scope(agent, gc.nogc()); let py = to_primitive(agent, y.into(), Some(PreferredType::Number), gc.reborrow()) .unbind()?; let gc = gc.into_nogc(); - let px = px.get(agent); - (px.bind(gc), py.bind(gc), gc) + let px = px.get(agent).bind(gc); + let py = py.bind(gc); + (px, py, gc) } (Err(_), Ok(py)) => { let py = py.scope(agent, gc.nogc()); let px = to_primitive(agent, x.into(), Some(PreferredType::Number), gc.reborrow()) .unbind()?; let gc = gc.into_nogc(); - let py = py.get(agent); - (px.bind(gc), py.bind(gc), gc) + let px = px.bind(gc); + // SAFETY: not shared. + let py = unsafe { py.take(agent) }.bind(gc); + (px, py, gc) } (Err(_), Err(_)) => { if LEFT_FIRST { @@ -392,17 +397,22 @@ pub(crate) fn is_less_than<'a, const LEFT_FIRST: bool>( let y = y.scope(agent, gc.nogc()); let px = to_primitive(agent, x.into(), Some(PreferredType::Number), gc.reborrow()) .unbind()? - .scope(agent, gc.nogc()); + .bind(gc.nogc()); + let py = y.get(agent).bind(gc.nogc()); + // SAFETY: not shared. + let px = unsafe { y.replace_self(agent, px.unbind()) }; let py = to_primitive( agent, - y.get(agent), + py.unbind(), Some(PreferredType::Number), gc.reborrow(), ) .unbind()?; let gc = gc.into_nogc(); - let px = px.get(agent); - (px.bind(gc), py.bind(gc), gc) + // SAFETY: not shared. + let px = unsafe { px.take(agent) }.bind(gc); + let py = py.bind(gc); + (px, py, gc) } else { // 2. Else, // a. NOTE: The order of evaluation needs to be reversed to preserve left to right evaluation. @@ -412,17 +422,21 @@ pub(crate) fn is_less_than<'a, const LEFT_FIRST: bool>( let x = x.scope(agent, gc.nogc()); let py = to_primitive(agent, y.into(), Some(PreferredType::Number), gc.reborrow()) .unbind()? - .scope(agent, gc.nogc()); + .bind(gc.nogc()); + let px = x.get(agent).bind(gc.nogc()); + // SAFETY: not shared. + let py = unsafe { x.replace_self(agent, py.unbind()) }; let px = to_primitive( agent, - x.get(agent), + px.unbind(), Some(PreferredType::Number), gc.reborrow(), ) .unbind()?; let gc = gc.into_nogc(); - let py = py.get(agent); - (px.bind(gc), py.bind(gc), gc) + let px = px.bind(gc); + let py = unsafe { py.take(agent) }.bind(gc); + (px, py, gc) } } }; diff --git a/nova_vm/src/ecmascript/builtins/indexed_collections/array_objects/array_prototype.rs b/nova_vm/src/ecmascript/builtins/indexed_collections/array_objects/array_prototype.rs index c6dbf75be..9703369b6 100644 --- a/nova_vm/src/ecmascript/builtins/indexed_collections/array_objects/array_prototype.rs +++ b/nova_vm/src/ecmascript/builtins/indexed_collections/array_objects/array_prototype.rs @@ -3793,6 +3793,7 @@ impl ArrayPrototype { // d. Set i to i + 1. i += 1; } + let local_a = scoped_a.get(agent).bind(gc.nogc()); // 17. For each element E of items, do for e in items { // a. Let Pi be ! ToString(𝔽(i)). @@ -3800,9 +3801,10 @@ impl ArrayPrototype { // b. Perform ! CreateDataPropertyOrThrow(A, Pi, E). unwrap_try(try_create_data_property_or_throw( agent, - scoped_a.get(agent), + local_a, pi, - e.get(agent).unbind(), + // SAFETY: not shared. + unsafe { e.take(agent) }, None, gc.nogc(), )); @@ -3833,9 +3835,9 @@ impl ArrayPrototype { // f. Set r to r + 1. r += 1; } - let a = scoped_a.get(agent); // 19. Return A. - Ok(a.into_value()) + // SAFETY: not shared. + Ok(unsafe { scoped_a.take(agent) }.into_value()) } /// ### [23.1.3.36 Array.prototype.toString ( )](https://tc39.es/ecma262/#sec-array.prototype.tostring) diff --git a/nova_vm/src/ecmascript/builtins/indexed_collections/typed_array_objects/typed_array_intrinsic_object.rs b/nova_vm/src/ecmascript/builtins/indexed_collections/typed_array_objects/typed_array_intrinsic_object.rs index 5f9c5c2ad..aa2442bb2 100644 --- a/nova_vm/src/ecmascript/builtins/indexed_collections/typed_array_objects/typed_array_intrinsic_object.rs +++ b/nova_vm/src/ecmascript/builtins/indexed_collections/typed_array_objects/typed_array_intrinsic_object.rs @@ -301,8 +301,8 @@ impl TypedArrayIntrinsicObject { // f. Set k to k + 1. k += 1; } - let target_obj = scoped_target_obj.get(agent); - Ok(target_obj.into_value()) + // SAFETY: not shared. + Ok(unsafe { scoped_target_obj.take(agent) }.into_value()) } /// ### [23.2.2.2 %TypedArray%.of ( ...items )](https://tc39.es/ecma262/#sec-properties-of-the-%typedarray%-intrinsic-object) diff --git a/nova_vm/src/ecmascript/builtins/ordinary.rs b/nova_vm/src/ecmascript/builtins/ordinary.rs index f8ed9bf06..723bc2e68 100644 --- a/nova_vm/src/ecmascript/builtins/ordinary.rs +++ b/nova_vm/src/ecmascript/builtins/ordinary.rs @@ -917,8 +917,8 @@ pub(crate) fn ordinary_get<'gc>( let object = object.bind(gc.nogc()); let property_key = property_key.bind(gc.nogc()); // Note: We scope here because it's likely we've already tried. - let scoped_object = object.scope(agent, gc.nogc()); let scoped_property_key = property_key.scope(agent, gc.nogc()); + let scoped_object = object.scope(agent, gc.nogc()); // 1. Let desc be ? O.[[GetOwnProperty]](P). let Some(descriptor) = object .unbind() @@ -929,17 +929,16 @@ pub(crate) fn ordinary_get<'gc>( // 2. If desc is undefined, then // a. Let parent be ? O.[[GetPrototypeOf]](). - let object = scoped_object.get(agent).bind(gc.nogc()); + // SAFETY: not shared. + let object = unsafe { scoped_object.take(agent) }.bind(gc.nogc()); let (parent, property_key, receiver) = if let TryResult::Continue(parent) = object.try_get_prototype_of(agent, gc.nogc()) { let Some(parent) = parent else { return Ok(Value::Undefined); }; - ( - parent, - scoped_property_key.get(agent).bind(gc.nogc()), - receiver, - ) + // SAFETY: not shared. + let property_key = unsafe { scoped_property_key.take(agent) }.bind(gc.nogc()); + (parent, property_key, receiver) } else { // Note: We should root property_key and receiver here. let receiver = receiver.scope(agent, gc.nogc()); @@ -952,18 +951,17 @@ pub(crate) fn ordinary_get<'gc>( return Ok(Value::Undefined); }; let parent = parent.unbind().bind(gc.nogc()); - let receiver = receiver.get(agent); - ( - parent, - scoped_property_key.get(agent).bind(gc.nogc()), - receiver, - ) + // SAFETY: not shared. + let receiver = unsafe { receiver.take(agent) }.bind(gc.nogc()); + // SAFETY: not shared. + let property_key = unsafe { scoped_property_key.take(agent) }.bind(gc.nogc()); + (parent, property_key, receiver) }; // c. Return ? parent.[[Get]](P, Receiver). return parent .unbind() - .internal_get(agent, property_key.unbind(), receiver, gc); + .internal_get(agent, property_key.unbind(), receiver.unbind(), gc); }; // 3. If IsDataDescriptor(desc) is true, return desc.[[Value]]. diff --git a/nova_vm/src/ecmascript/execution/environments/global_environment.rs b/nova_vm/src/ecmascript/execution/environments/global_environment.rs index 9aa8c65ba..cedb90c39 100644 --- a/nova_vm/src/ecmascript/execution/environments/global_environment.rs +++ b/nova_vm/src/ecmascript/execution/environments/global_environment.rs @@ -607,14 +607,17 @@ impl<'e> GlobalEnvironment<'e> { .unbind() .delete_binding(agent, scoped_name.get(agent), gc.reborrow()) .unbind()?; - let env = unsafe { env.take(agent) }.bind(gc.nogc()); + let gc = gc.into_nogc(); + // SAFETY: not shared. + let env = unsafe { env.take(agent) }.bind(gc); // b. If status is true and envRec.[[VarNames]] contains N, then if status { - let name = scoped_name.get(agent); + // SAFETY: not shared. + let name = unsafe { scoped_name.take(agent) }.bind(gc); let env_rec = &mut agent[env]; if env_rec.var_names.contains(&name) { // i. Remove N from envRec.[[VarNames]]. - env_rec.var_names.remove(&name); + env_rec.var_names.remove(&name.unbind()); } } // c. Return status. From 7ea0a0ed2b5aefe751bd049553190bc5db81c462 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Elias=20Sj=C3=B6green?= Date: Sun, 14 Dec 2025 17:21:29 +0100 Subject: [PATCH 9/9] fix: Aapos suggestions --- nova_lint/src/immediately_bind_scoped.rs | 71 +++++++++++++++--------- nova_lint/ui/immediately_bind_scoped.rs | 2 +- 2 files changed, 47 insertions(+), 26 deletions(-) diff --git a/nova_lint/src/immediately_bind_scoped.rs b/nova_lint/src/immediately_bind_scoped.rs index bfd32f937..c37a78661 100644 --- a/nova_lint/src/immediately_bind_scoped.rs +++ b/nova_lint/src/immediately_bind_scoped.rs @@ -1,10 +1,11 @@ -use std::ops::ControlFlow; +use std::{fmt::Display, 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, + get_enclosing_block, get_parent_expr, paths::{PathNS, lookup_path_str}, + res::MaybeResPath, ty::implements_trait, visitors::for_each_expr, }; @@ -16,11 +17,14 @@ use rustc_middle::ty::Ty; dylint_linting::declare_late_lint! { /// ### What it does /// - /// Makes sure that the user immediately binds `Scoped::get` results. + /// Makes sure that the user immediately binds `Scoped::get` and + /// `Scoped::take` results. /// /// ### Why is this bad? /// - /// TODO: Write an explanation of why this is bad. + /// To avoid odd bugs with the garbage collector when dealing with scoped + /// values, it is important that `Scoped::get` and + /// `Scoped::take` results are immediately bound. /// /// ### Example /// @@ -48,13 +52,13 @@ dylint_linting::declare_late_lint! { /// `Value`s immediately. pub IMMEDIATELY_BIND_SCOPED, Deny, - "the result of `Scoped::get` should be immediately bound" + "the result of `Scoped::get` or `Scoped::take` 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) { + // First we check if we have found a `Scoped::get` or `Scoped::take` call + if let Some(scoped_method) = is_scoped_get_or_take_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) @@ -97,7 +101,7 @@ impl<'tcx> LateLintPass<'tcx> for ImmediatelyBindScoped { // 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 expr_in_stmt.res_local_id() == Some(local_hir_id) { if is_valid_use_of_unbound_value(cx, expr_in_stmt, local_hir_id) { found_valid_next_use = true; } @@ -117,7 +121,10 @@ impl<'tcx> LateLintPass<'tcx> for ImmediatelyBindScoped { cx, IMMEDIATELY_BIND_SCOPED, expr.span, - "the result of `Scoped::get` should be immediately bound", + format!( + "the result of `Scoped::{}` should be immediately bound", + scoped_method + ), None, "immediately bind the value", ); @@ -142,7 +149,8 @@ fn get_assigned_local(cx: &LateContext<'_>, expr: &Expr) -> Option { } } -/// Check if a use of an unbound value is valid (binding or function argument) +/// Check if a use of an unbound value is valid, this is either by being a +/// binding or function argument, or in the return position of a function. 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) @@ -157,12 +165,12 @@ fn is_valid_use_of_unbound_value(cx: &LateContext<'_>, expr: &Expr, hir_id: HirI } // 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) { + if expr.res_local_id() == Some(hir_id) && is_in_argument_position(cx, expr) { return true; } // If this is the self value of a method call, it's valid - if path_to_local_id(expr, hir_id) && is_in_self_position(cx, expr) { + if expr.res_local_id() == Some(hir_id) && is_in_self_position(cx, expr) { return true; } @@ -174,16 +182,12 @@ fn is_in_self_position(cx: &LateContext<'_>, expr: &Expr) -> bool { // Walk up the parent chain to see if we're in a method call while let Some(parent) = get_parent_expr(cx, current_expr) { - match parent.kind { - // If we find a method call where our expression is in the receiver position - ExprKind::MethodCall(_, receiver, args, _) => { - if receiver.hir_id == current_expr.hir_id { - return true; - } + // If we find a method call where our expression is in the receiver position + if let ExprKind::MethodCall(_, receiver, _, _) = parent.kind + && receiver.hir_id == current_expr.hir_id { + return true; } - // Continue walking up for other expression types - _ => {} - } + // Else continue walking up for other expression types current_expr = parent; } @@ -220,16 +224,33 @@ fn is_in_argument_position(cx: &LateContext<'_>, expr: &Expr) -> bool { false } -fn is_scoped_get_method_call(cx: &LateContext<'_>, expr: &Expr) -> bool { +enum ScopedMethod { + Get, + Take, +} + +impl Display for ScopedMethod { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + ScopedMethod::Get => write!(f, "get"), + ScopedMethod::Take => write!(f, "take"), + } + } +} + +fn is_scoped_get_or_take_method_call(cx: &LateContext<'_>, expr: &Expr) -> Option { 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 + match method { + "get" => Some(ScopedMethod::Get), + "take" => Some(ScopedMethod::Take), + _ => None, + } } else { - false + None } } diff --git a/nova_lint/ui/immediately_bind_scoped.rs b/nova_lint/ui/immediately_bind_scoped.rs index 3f9d8285c..6d8022cfb 100644 --- a/nova_lint/ui/immediately_bind_scoped.rs +++ b/nova_lint/ui/immediately_bind_scoped.rs @@ -3,8 +3,8 @@ use nova_vm::{ ecmascript::{execution::Agent, types::Value}, engine::{ - context::{Bindable, GcScope, NoGcScope}, Scoped, + context::{Bindable, NoGcScope}, }, };