feat(linter): Implement immediately_bind_scoped rule

eliassjogreen · eliassjogreen · commit 4572c1264cdb · 2025-10-14T22:55:01.000+02:00
diff --git 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
@@ -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/immediately_bind_scoped.rs 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<Value>::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<Value>::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<Value>::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<Value>::get` should be immediately bound",
+                    None,
+                    "immediately bind the value",
+                );
+            }
+        }
+    }
+}
diff --git 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
@@ -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
@@ -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<Value>, 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<Value>, 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<Value>,
+//     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<Value>,
+//     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<Value>) {
+    let _a = scoped.get(agent);
+}
+
+fn test_scoped_get_doesnt_need_to_be_bound_if_not_assigned(agent: &Agent, scoped: Scoped<Value>) {
+    scoped.get(agent);
+}
+
+fn test_improbable_but_technically_bad_situation(
+    agent: &Agent,
+    scoped: Scoped<Value>,
+    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
@@ -0,0 +1,19 @@
+error: the result of `Scoped<Value>::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<Value>::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
+
