Merge branch 'main' into 128-improve-unprotected-update-current-contr…

…act-wasm-detector

.github/workflows/general-rust.yml

@@ -6,9 +6,8 @@ on:
       - "detectors/**"
       - "test-cases/**"
       - "scripts/**"
-    paths-ignore:
-      - "detectors/**/*.md"
-      - "test-cases/**/*.md"
+      - "!detectors/**/*.md"
+      - "!test-cases/**/*.md"
 
 env:
   CARGO_TERM_COLOR: always

.github/workflows/test-detectors.yml

@@ -6,9 +6,8 @@ on:
       - "detectors/**"
       - "test-cases/**"
       - "scripts/**"
-    paths-ignore:
-      - "detectors/**/*.md"
-      - "test-cases/**/*.md"
+      - "!detectors/**/*.md"
+      - "!test-cases/**/*.md"
 
 env:
   CARGO_TERM_COLOR: always

detectors/unsafe-expect/src/lib.rs

@@ -1,18 +1,25 @@
 #![feature(rustc_private)]
+#![allow(clippy::enum_variant_names)]
 
 extern crate rustc_hir;
 extern crate rustc_span;
 
+use std::{collections::HashSet, hash::Hash};
+
+use if_chain::if_chain;
+use rustc_hir::BinOpKind;
 use rustc_hir::{
+    def::Res,
     def_id::LocalDefId,
     intravisit::{walk_expr, FnKind, Visitor},
-    Body, Expr, ExprKind, FnDecl,
+    Body, Expr, ExprKind, FnDecl, HirId, Local, QPath, UnOp,
 };
 use rustc_lint::{LateContext, LateLintPass};
-use rustc_span::{Span, Symbol};
-use scout_audit_clippy_utils::diagnostics::span_lint_and_help;
+use rustc_span::{sym, Span, Symbol};
+use scout_audit_clippy_utils::{diagnostics::span_lint_and_help, higher};
 
 const LINT_MESSAGE: &str = "Unsafe usage of `expect`";
+const PANIC_INDUCING_FUNCTIONS: [&str; 2] = ["panic", "bail"];
 
 dylint_linting::declare_late_lint! {
     /// ### What it does
@@ -57,53 +64,263 @@ dylint_linting::declare_late_lint! {
     }
 }
 
-impl<'tcx> LateLintPass<'tcx> for UnsafeExpect {
-    fn check_fn(
-        &mut self,
-        cx: &LateContext<'tcx>,
-        _: FnKind<'tcx>,
-        _: &'tcx FnDecl<'tcx>,
-        body: &'tcx Body<'tcx>,
-        _: Span,
-        _: LocalDefId,
-    ) {
-        struct UnsafeExpectVisitor {
-            has_expect: bool,
-            has_expect_span: Vec<Option<Span>>,
+/// Represents the type of check performed on method call expressions to determine their safety or behavior.
+#[derive(Clone, Copy, Hash, Eq, PartialEq)]
+enum CheckType {
+    IsSome,
+    IsNone,
+    IsOk,
+    IsErr,
+}
+
+impl CheckType {
+    fn from_method_name(name: Symbol) -> Option<Self> {
+        match name.as_str() {
+            "is_some" => Some(Self::IsSome),
+            "is_none" => Some(Self::IsNone),
+            "is_ok" => Some(Self::IsOk),
+            "is_err" => Some(Self::IsErr),
+            _ => None,
+        }
+    }
+
+    fn inverse(self) -> Self {
+        match self {
+            Self::IsSome => Self::IsNone,
+            Self::IsNone => Self::IsSome,
+            Self::IsOk => Self::IsErr,
+            Self::IsErr => Self::IsOk,
+        }
+    }
+
+    /// Determines if the check type implies execution should halt, such as in error conditions.
+    fn should_halt_execution(self) -> bool {
+        matches!(self, Self::IsNone | Self::IsErr)
+    }
+
+    /// Determines if it is safe to expect the value without further checks, i.e., the value is present.
+    fn is_safe_to_expect(self) -> bool {
+        matches!(self, Self::IsSome | Self::IsOk)
+    }
+}
+
+/// Represents a conditional checker that is used to analyze `if` or `if let` expressions.
+#[derive(Clone, Copy, Hash, Eq, PartialEq)]
+struct ConditionalChecker {
+    check_type: CheckType,
+    checked_expr_hir_id: HirId,
+}
+
+impl ConditionalChecker {
+    /// Handle te condition of the `if` or `if let` expression.
+    fn handle_condition(condition: &Expr<'_>, inverse: bool) -> HashSet<Self> {
+        if_chain! {
+            if let ExprKind::MethodCall(path_segment, receiver, _, _) = condition.kind;
+            if let Some(check_type) = CheckType::from_method_name(path_segment.ident.name);
+            if let ExprKind::Path(QPath::Resolved(_, checked_expr_path)) = receiver.kind;
+            if let Res::Local(checked_expr_hir_id) = checked_expr_path.res;
+            then {
+                let check_type = if inverse { check_type.inverse() } else { check_type };
+                return std::iter::once(Self { check_type, checked_expr_hir_id }).collect();
+            }
+        }
+        HashSet::new()
+    }
+
+    /// Constructs a ConditionalChecker from an expression if it matches a method call with a valid CheckType.
+    fn from_expression(condition: &Expr<'_>) -> HashSet<Self> {
+        match condition.kind {
+            // Single `not` expressions are supported
+            ExprKind::Unary(op, condition) => Self::handle_condition(condition, op == UnOp::Not),
+            // Multiple `or` expressions are supported
+            ExprKind::Binary(op, left_condition, right_condition) if op.node == BinOpKind::Or => {
+                let mut result = Self::from_expression(left_condition);
+                result.extend(Self::from_expression(right_condition));
+                result
+            }
+            ExprKind::MethodCall(..) => Self::handle_condition(condition, false),
+            _ => HashSet::new(),
+        }
+    }
+}
+
+/// Main unsafe-expect visitor
+struct UnsafeExpectVisitor<'a, 'tcx> {
+    cx: &'a LateContext<'tcx>,
+    conditional_checker: HashSet<ConditionalChecker>,
+    checked_exprs: HashSet<HirId>,
+}
+
+impl UnsafeExpectVisitor<'_, '_> {
+    fn is_panic_inducing_call(&self, func: &Expr<'_>) -> bool {
+        if let ExprKind::Path(QPath::Resolved(_, path)) = &func.kind {
+            return PANIC_INDUCING_FUNCTIONS.iter().any(|&func| {
+                path.segments
+                    .iter()
+                    .any(|segment| segment.ident.name.as_str().contains(func))
+            });
+        }
+        false
+    }
+
+    fn get_expect_info(&self, receiver: &Expr<'_>) -> Option<HirId> {
+        if_chain! {
+            if let ExprKind::Path(QPath::Resolved(_, path)) = &receiver.kind;
+            if let Res::Local(hir_id) = path.res;
+            then {
+                return Some(hir_id);
+            }
+        }
+        None
+    }
+
+    fn set_conditional_checker(&mut self, conditional_checkers: &HashSet<ConditionalChecker>) {
+        for checker in conditional_checkers {
+            self.conditional_checker.insert(*checker);
+            if checker.check_type.is_safe_to_expect() {
+                self.checked_exprs.insert(checker.checked_expr_hir_id);
+            }
+        }
+    }
+
+    fn reset_conditional_checker(&mut self, conditional_checkers: HashSet<ConditionalChecker>) {
+        for checker in conditional_checkers {
+            if checker.check_type.is_safe_to_expect() {
+                self.checked_exprs.remove(&checker.checked_expr_hir_id);
+            }
+            self.conditional_checker.remove(&checker);
         }
+    }
+
+    /// Process conditional expressions to determine if they should halt execution.
+    fn handle_if_expressions(&mut self) {
+        self.conditional_checker.iter().for_each(|checker| {
+            if checker.check_type.should_halt_execution() {
+                println!("Halt: {:?}", checker.checked_expr_hir_id);
+                self.checked_exprs.insert(checker.checked_expr_hir_id);
+            }
+        });
+    }
+
+    fn is_literal_or_composed_of_literals(&self, expr: &Expr<'_>) -> bool {
+        let mut stack = vec![expr];
 
-        impl<'tcx> Visitor<'tcx> for UnsafeExpectVisitor {
-            fn visit_expr(&mut self, expr: &'tcx Expr<'_>) {
-                if let ExprKind::MethodCall(path_segment, _, _, _) = &expr.kind {
-                    if path_segment.ident.name == Symbol::intern("expect") {
-                        self.has_expect = true;
-                        self.has_expect_span.push(Some(expr.span));
+        while let Some(current_expr) = stack.pop() {
+            match current_expr.kind {
+                ExprKind::Lit(_) => continue, // A literal is fine, continue processing.
+                ExprKind::Tup(elements) | ExprKind::Array(elements) => {
+                    stack.extend(elements);
+                }
+                ExprKind::Struct(_, fields, _) => {
+                    for field in fields {
+                        stack.push(field.expr);
                     }
                 }
-                walk_expr(self, expr);
+                ExprKind::Repeat(element, _) => {
+                    stack.push(element);
+                }
+                _ => return false, // If any element is not a literal or a compound of literals, return false.
             }
         }
 
-        let mut visitor = UnsafeExpectVisitor {
-            has_expect: false,
-            has_expect_span: Vec::new(),
-        };
+        true // If the stack is emptied without finding a non-literal, all elements are literals.
+    }
+}
 
-        walk_expr(&mut visitor, body.value);
+impl<'a, 'tcx> Visitor<'tcx> for UnsafeExpectVisitor<'a, 'tcx> {
+    fn visit_local(&mut self, local: &'tcx Local<'tcx>) {
+        if_chain! {
+            if let Some(init) = local.init;
+            if let ExprKind::Call(func, args) = init.kind;
+            if let ExprKind::Path(QPath::Resolved(_, path)) = func.kind;
+            then {
+                let is_some_or_ok = path.segments.iter().any(|segment|
+                    matches!(segment.ident.name, sym::Some | sym::Ok)
+                );
+                let all_literals = args.iter().all(|arg| self.is_literal_or_composed_of_literals(arg));
+                if is_some_or_ok && all_literals {
+                    self.checked_exprs.insert(local.pat.hir_id);
+                }
+            }
+        }
+    }
+
+    fn visit_expr(&mut self, expr: &'tcx Expr<'_>) {
+        // If we are inside an `if` or `if let` expression, we analyze its body
+        if !self.conditional_checker.is_empty() {
+            match &expr.kind {
+                ExprKind::Ret(..) => {
+                    println!("Ret");
+                    self.handle_if_expressions();
+                }
+                ExprKind::Call(func, _) if self.is_panic_inducing_call(func) => {
+                    self.handle_if_expressions()
+                }
+                _ => {}
+            }
+        }
+
+        // Find `if` or `if let` expressions
+        if let Some(higher::IfOrIfLet {
+            cond,
+            then: if_expr,
+            r#else: _,
+        }) = higher::IfOrIfLet::hir(expr)
+        {
+            // If we are interested in the condition (if it is a CheckType) we traverse the body.
+            let conditional_checker = ConditionalChecker::from_expression(cond);
+            self.set_conditional_checker(&conditional_checker);
+            walk_expr(self, if_expr);
+            self.reset_conditional_checker(conditional_checker);
+            return;
+        }
 
-        if visitor.has_expect {
-            visitor.has_expect_span.iter().for_each(|span| {
-                if let Some(span) = span {
+        // If we find an unsafe `expect`, we raise a warning
+        if_chain! {
+            if let ExprKind::MethodCall(path_segment, receiver, _, _) = &expr.kind;
+            if path_segment.ident.name == sym::expect;
+            then {
+                let receiver_hir_id = self.get_expect_info(receiver);
+                // If the receiver is `None`, then we asume that the `expect` is unsafe
+                let is_checked_safe = receiver_hir_id.map_or(false, |id| self.checked_exprs.contains(&id));
+                if !is_checked_safe {
                     span_lint_and_help(
-                        cx,
+                        self.cx,
                         UNSAFE_EXPECT,
-                        *span,
+                        expr.span,
                         LINT_MESSAGE,
                         None,
                         "Please, use a custom error instead of `expect`",
                     );
                 }
-            });
+             }
         }
+
+        walk_expr(self, expr);
+    }
+}
+
+impl<'tcx> LateLintPass<'tcx> for UnsafeExpect {
+    fn check_fn(
+        &mut self,
+        cx: &LateContext<'tcx>,
+        _: FnKind<'tcx>,
+        _: &'tcx FnDecl<'tcx>,
+        body: &'tcx Body<'tcx>,
+        span: Span,
+        _: LocalDefId,
+    ) {
+        // If the function comes from a macro expansion, we don't want to analyze it.
+        if span.from_expansion() {
+            return;
+        }
+
+        let mut visitor = UnsafeExpectVisitor {
+            cx,
+            checked_exprs: HashSet::new(),
+            conditional_checker: HashSet::new(),
+        };
+
+        walk_expr(&mut visitor, body.value);
     }
 }