Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions nova_lint/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
2 changes: 1 addition & 1 deletion nova_lint/src/agent_comes_first.rs
Original file line number Diff line number Diff line change
@@ -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;

Expand Down
2 changes: 1 addition & 1 deletion nova_lint/src/gc_scope_comes_last.rs
Original file line number Diff line number Diff line change
@@ -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;

Expand Down
2 changes: 1 addition & 1 deletion nova_lint/src/gc_scope_is_only_passed_by_value.rs
Original file line number Diff line number Diff line change
@@ -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;
Expand Down
273 changes: 273 additions & 0 deletions nova_lint/src/immediately_bind_scoped.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,273 @@
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,
paths::{PathNS, lookup_path_str},
res::MaybeResPath,
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<Value>::get` and
/// `Scoped<Value>::take` results.
///
/// ### Why is this bad?
///
/// To avoid odd bugs with the garbage collector when dealing with scoped
/// values, it is important that `Scoped<Value>::get` and
/// `Scoped<Value>::take` results are immediately bound.
///
/// ### 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` or `Scoped<Value>::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<Value>::get` or `Scoped<Value>::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)
&& 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 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;
}

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,
format!(
"the result of `Scoped<Value>::{}` should be immediately bound",
scoped_method
),
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<HirId> {
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, 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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: Return position would also be valid: return Ok(value.get(agent)); is perfectly safe.

// 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 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 expr.res_local_id() == Some(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) {
// 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;
}
// Else continue walking up for other expression types
current_expr = parent;
}

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
}

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<ScopedMethod> {
if let Some((method, recv, _, _, _)) = method_call(expr)
&& let typeck_results = cx.typeck_results()
&& let recv_ty = typeck_results.expr_ty(recv)
&& is_scoped_ty(cx, &recv_ty)
{
match method {
"get" => Some(ScopedMethod::Get),
"take" => Some(ScopedMethod::Take),
_ => None,
}
} else {
None
}
}

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, &[]))
}
2 changes: 2 additions & 0 deletions nova_lint/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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);
Expand Down
34 changes: 32 additions & 2 deletions nova_lint/src/utils.rs
Original file line number Diff line number Diff line change
@@ -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
//
Expand All @@ -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(_))
}
Expand Down Expand Up @@ -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,
}
}
Loading
Loading