feat(linter): Rule which warns when you could've used NoGcScope (#913)

Author: eliassjogreen

nova_cli/src/main.rs

@@ -1,6 +1,8 @@
 // This Source Code Form is subject to the terms of the Mozilla Public
 // License, v. 2.0. If a copy of the MPL was not distributed with this
 // file, You can obtain one at https://mozilla.org/MPL/2.0/.
+#![allow(unknown_lints, can_use_no_gc_scope)]
+
 mod helper;
 mod theme;
 

nova_lint/Cargo.toml

@@ -16,6 +16,10 @@ crate-type = ["cdylib"]
 name = "agent_comes_first"
 path = "ui/agent_comes_first.rs"
 
+[[example]]
+name = "can_use_no_gc_scope"
+path = "ui/can_use_no_gc_scope.rs"
+
 [[example]]
 name = "gc_scope_comes_last"
 path = "ui/gc_scope_comes_last.rs"

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;
 

nova_lint/src/can_use_no_gc_scope.rs

@@ -0,0 +1,151 @@
+use std::ops::ControlFlow;
+
+use clippy_utils::{
+    diagnostics::span_lint_and_sugg, fn_def_id, is_trait_impl_item, source::HasSession,
+    visitors::for_each_expr,
+};
+use rustc_errors::Applicability;
+use rustc_hir::{Body, Expr, ExprKind, FnDecl, def::Res, def_id::LocalDefId, intravisit::FnKind};
+use rustc_hir_analysis::lower_ty;
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_span::{BytePos, Span};
+
+use crate::{could_be_builtin_method_def, is_gc_scope_ty, is_no_gc_method, is_trait_item};
+
+dylint_linting::declare_late_lint! {
+    /// ### What it does
+    ///
+    /// Checks that a function which only needs `NoGcScope` uses it instead of
+    /// `GcScope`.
+    ///
+    /// ### Why is this bad?
+    ///
+    /// You usually should use `NoGcScope` instead of `GcScope` if you don't
+    /// need the latter. The reason this is bad is that it forces the caller
+    /// to scope any heap references held past the call site unnecessarily.
+    ///
+    /// ### Example
+    ///
+    /// ```rust
+    /// fn foo(gc: GcScope<'_, '_>) {}
+    /// ```
+    ///
+    /// Use instead:
+    ///
+    /// ```rust
+    /// fn foo(gc: NoGcScope<'_, '_>) {}
+    /// ```
+    pub CAN_USE_NO_GC_SCOPE,
+    Warn,
+    "you should use `NoGcScope` instead of `GcScope` if you don't need the latter"
+}
+
+impl<'tcx> LateLintPass<'tcx> for CanUseNoGcScope {
+    fn check_fn(
+        &mut self,
+        cx: &LateContext<'tcx>,
+        kind: FnKind<'tcx>,
+        _: &'tcx FnDecl<'tcx>,
+        body: &'tcx Body<'tcx>,
+        span: Span,
+        _: LocalDefId,
+    ) {
+        if span.from_expansion() {
+            return;
+        }
+
+        // Skip closures
+        if matches!(kind, FnKind::Closure) {
+            return;
+        }
+
+        // Skip trait definitions and methods
+        let res = {
+            let body_id = cx.tcx.hir_body_owner(body.id());
+            is_trait_impl_item(cx, body_id) || is_trait_item(cx, body_id)
+        };
+        if res {
+            return;
+        }
+
+        // Skip builtin methods
+        if could_be_builtin_method_def(cx, kind) {
+            return;
+        }
+
+        // Skip `GcScope` methods
+        if let FnKind::Method(_, sig) = kind
+            && let Some(maybe_self) = sig.decl.inputs.first()
+            && is_gc_scope_ty(cx, &lower_ty(cx.tcx, maybe_self))
+        {
+            return;
+        }
+
+        let typeck = cx.typeck_results();
+
+        let Some(gc_scope) = body.params.iter().find(|param| {
+            let ty = typeck.pat_ty(param.pat);
+            is_gc_scope_ty(cx, &ty)
+        }) else {
+            // Either the function already takes `NoGcScope` or it doesn't take any
+            // at all in which case we don't need to lint.
+            return;
+        };
+
+        if for_each_expr(cx, body.value, |expr| {
+            // Checks if the expression is function or method call
+            if let Some(did) = fn_def_id(cx, expr)
+                // If we encountered either a `nogc` och `into_nogc` method call
+                // we skip them because they don't count as needing a `GcScope`.
+                && !is_no_gc_method(cx, did)
+            {
+                // Check if the function actually uses `GcScope` in its signature
+                let sig = cx.tcx.fn_sig(did).instantiate_identity().skip_binder();
+                if sig.inputs().iter().any(|input| is_gc_scope_ty(cx, input)) {
+                    return ControlFlow::Break(());
+                }
+            }
+
+            // Calls to closures and other functions may also use `GcScope`,
+            // we need to check those as well.
+            if let ExprKind::Call(
+                Expr {
+                    kind: ExprKind::Path(qpath),
+                    hir_id: path_hir_id,
+                    ..
+                },
+                args,
+            ) = expr.kind
+                && let Res::Local(_) = typeck.qpath_res(qpath, *path_hir_id)
+                && args.iter().any(|arg| {
+                    let ty = typeck.expr_ty(arg);
+                    is_gc_scope_ty(cx, &ty)
+                })
+            {
+                return ControlFlow::Break(());
+            }
+
+            ControlFlow::Continue(())
+        })
+        .is_none()
+        {
+            // We didn't find any calls in the body that would require a `GcScope`
+            // so we can suggest using `NoGcScope` instead.
+            let ty_span = cx
+                .sess()
+                .source_map()
+                .span_until_char(gc_scope.ty_span, '<');
+            // Trim the start of the span to just before the `GcScope` type name
+            let ty_span = ty_span.with_lo(ty_span.hi() - BytePos(7));
+            span_lint_and_sugg(
+                cx,
+                CAN_USE_NO_GC_SCOPE,
+                ty_span,
+                "you can use `NoGcScope` instead of `GcScope` here",
+                "replace with",
+                "NoGcScope".to_owned(),
+                Applicability::MaybeIncorrect,
+            );
+        }
+    }
+}

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;
 

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;

nova_lint/src/lib.rs

@@ -9,7 +9,9 @@ extern crate rustc_ast_pretty;
 extern crate rustc_data_structures;
 extern crate rustc_errors;
 extern crate rustc_hir;
+extern crate rustc_hir_analysis;
 extern crate rustc_hir_pretty;
+extern crate rustc_hir_typeck;
 extern crate rustc_index;
 extern crate rustc_infer;
 extern crate rustc_lexer;
@@ -23,6 +25,7 @@ extern crate rustc_target;
 extern crate rustc_trait_selection;
 
 mod agent_comes_first;
+mod can_use_no_gc_scope;
 mod gc_scope_comes_last;
 mod gc_scope_is_only_passed_by_value;
 mod no_it_performs_the_following;
@@ -35,6 +38,7 @@ pub(crate) use utils::*;
 #[unsafe(no_mangle)]
 pub fn register_lints(sess: &rustc_session::Session, lint_store: &mut rustc_lint::LintStore) {
     agent_comes_first::register_lints(sess, lint_store);
+    can_use_no_gc_scope::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);
     no_it_performs_the_following::register_lints(sess, lint_store);

nova_lint/src/utils.rs

@@ -1,4 +1,6 @@
-use rustc_hir::def_id::DefId;
+use clippy_utils::peel_hir_ty_options;
+use rustc_hir::{FnSig, HirId, ItemKind, Node, def_id::DefId, intravisit::FnKind};
+use rustc_hir_analysis::lower_ty;
 use rustc_lint::LateContext;
 use rustc_middle::ty::{Ty, TyKind};
 use rustc_span::symbol::Symbol;
@@ -19,6 +21,23 @@ pub fn match_def_path(cx: &LateContext<'_>, did: DefId, syms: &[&str]) -> bool {
         .eq(path.iter().copied())
 }
 
+pub fn match_def_paths(cx: &LateContext<'_>, did: DefId, syms: &[&[&str]]) -> bool {
+    let path = cx.get_def_path(did);
+    syms.iter().any(|syms| {
+        syms.iter()
+            .map(|x| Symbol::intern(x))
+            .eq(path.iter().copied())
+    })
+}
+
+pub fn is_trait_item(cx: &LateContext<'_>, hir_id: HirId) -> bool {
+    if let Node::Item(item) = cx.tcx.parent_hir_node(hir_id) {
+        matches!(item.kind, ItemKind::Trait(..))
+    } else {
+        false
+    }
+}
+
 pub fn is_param_ty(ty: &Ty) -> bool {
     matches!(ty.kind(), TyKind::Param(_))
 }
@@ -35,16 +54,27 @@ pub fn is_agent_ty(cx: &LateContext<'_>, ty: &Ty) -> bool {
 }
 
 pub fn is_gc_scope_ty(cx: &LateContext<'_>, ty: &Ty) -> bool {
-    match ty.kind() {
+    match ty.peel_refs().kind() {
         TyKind::Adt(def, _) => {
             match_def_path(cx, def.did(), &["nova_vm", "engine", "context", "GcScope"])
         }
         _ => false,
     }
 }
 
+pub fn is_no_gc_method(cx: &LateContext<'_>, did: DefId) -> bool {
+    match_def_paths(
+        cx,
+        did,
+        &[
+            &["nova_vm", "engine", "context", "GcScope", "nogc"],
+            &["nova_vm", "engine", "context", "GcScope", "into_nogc"],
+        ],
+    )
+}
+
 pub fn is_no_gc_scope_ty(cx: &LateContext<'_>, ty: &Ty) -> bool {
-    match ty.kind() {
+    match ty.peel_refs().kind() {
         TyKind::Adt(def, _) => match_def_path(
             cx,
             def.did(),
@@ -53,3 +83,85 @@ pub fn is_no_gc_scope_ty(cx: &LateContext<'_>, ty: &Ty) -> bool {
         _ => false,
     }
 }
+
+pub fn is_value_ty(cx: &LateContext<'_>, ty: &Ty) -> bool {
+    match ty.peel_refs().kind() {
+        TyKind::Adt(def, _) => match_def_path(
+            cx,
+            def.did(),
+            &[
+                "nova_vm",
+                "ecmascript",
+                "types",
+                "language",
+                "value",
+                "Value",
+            ],
+        ),
+        _ => false,
+    }
+}
+
+pub fn is_object_ty(cx: &LateContext<'_>, ty: &Ty) -> bool {
+    match ty.peel_refs().kind() {
+        TyKind::Adt(def, _) => match_def_path(
+            cx,
+            def.did(),
+            &[
+                "nova_vm",
+                "ecmascript",
+                "types",
+                "language",
+                "object",
+                "Object",
+            ],
+        ),
+        _ => false,
+    }
+}
+
+pub fn is_arguments_list_ty(cx: &LateContext<'_>, ty: &Ty) -> bool {
+    match ty.peel_refs().kind() {
+        TyKind::Adt(def, _) => match_def_path(
+            cx,
+            def.did(),
+            &[
+                "nova_vm",
+                "ecmascript",
+                "builtins",
+                "builtin_function",
+                "ArgumentsList",
+            ],
+        ),
+        _ => false,
+    }
+}
+
+pub fn could_be_builtin_method_sig<'tcx>(cx: &LateContext<'tcx>, sig: &FnSig<'tcx>) -> bool {
+    sig.decl.inputs.len() == 4
+        && is_agent_ty(cx, &lower_ty(cx.tcx, &sig.decl.inputs[0]))
+        && is_value_ty(cx, &lower_ty(cx.tcx, &sig.decl.inputs[1]))
+        && is_arguments_list_ty(cx, &lower_ty(cx.tcx, &sig.decl.inputs[2]))
+        && is_gc_scope_ty(cx, &lower_ty(cx.tcx, &sig.decl.inputs[3]))
+}
+
+pub fn could_be_builtin_constructor_sig<'tcx>(cx: &LateContext<'tcx>, sig: &FnSig<'tcx>) -> bool {
+    sig.decl.inputs.len() == 5
+        && is_agent_ty(cx, &lower_ty(cx.tcx, &sig.decl.inputs[0]))
+        && is_value_ty(cx, &lower_ty(cx.tcx, &sig.decl.inputs[1]))
+        && is_arguments_list_ty(cx, &lower_ty(cx.tcx, &sig.decl.inputs[2]))
+        && is_object_ty(
+            cx,
+            &lower_ty(cx.tcx, peel_hir_ty_options(cx, &sig.decl.inputs[3])),
+        )
+        && is_gc_scope_ty(cx, &lower_ty(cx.tcx, &sig.decl.inputs[4]))
+}
+
+pub fn could_be_builtin_method_def<'tcx>(cx: &LateContext<'tcx>, kind: FnKind<'tcx>) -> bool {
+    match kind {
+        FnKind::Method(_, sig) => {
+            could_be_builtin_method_sig(cx, sig) || could_be_builtin_constructor_sig(cx, sig)
+        }
+        _ => false,
+    }
+}