Skip to content

Commit

Permalink
Apply lost changes
Browse files Browse the repository at this point in the history
  • Loading branch information
@jgcrosta
jgcrosta committed Aug 30, 2024
1 parent 6839388 commit de0613a
Show file tree
Hide file tree
Showing 5 changed files with 106 additions and 55 deletions.
7 changes: 4 additions & 3 deletions detectors/iterators-over-indexing/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ use rustc_lint::{LateContext, LateLintPass};
use rustc_middle::ty::{TyCtxt, TyKind};
use rustc_span::{symbol::Ident, Span};
use rustc_type_ir::Interner;
use std::collections::HashSet;
use utils::get_node_type;

const LINT_MESSAGE: &str =
Expand All @@ -39,7 +40,7 @@ dylint_linting::declare_late_lint! {
}

struct ForLoopVisitor<'a, 'b> {
span_constant: Vec<Span>,
span_constant: HashSet<Span>,
cx: &'b LateContext<'a>,
}
struct VectorAccessVisitor<'a, 'b> {
Expand Down Expand Up @@ -351,7 +352,7 @@ fn handle_expr<'a>(me: &mut ForLoopVisitor<'a, '_>, expr: &'a Expr<'a>) -> Resul
}

if visitor.has_vector_access {
me.span_constant.push(expr.span);
me.span_constant.insert(expr.span);
}

Ok(())
Expand All @@ -377,7 +378,7 @@ impl<'tcx> LateLintPass<'tcx> for IteratorOverIndexing {
if let FnKind::Method(_ident, _sig) = kind {
let span_constant = {
let mut visitor = ForLoopVisitor {
span_constant: vec![],
span_constant: HashSet::new(),
cx,
};
walk_expr(&mut visitor, body.value);
Expand Down
1 change: 1 addition & 0 deletions detectors/unsafe-expect/Cargo.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ clippy_utils = { workspace = true }
clippy_wrappers = { workspace = true }
dylint_linting = { workspace = true }
if_chain = { workspace = true }
utils = { workspace = true }

[package.metadata.rust-analyzer]
rustc_private = true
58 changes: 34 additions & 24 deletions detectors/unsafe-expect/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,7 @@
extern crate rustc_hir;
extern crate rustc_span;

use std::{collections::HashSet, hash::Hash};

use clippy_utils::higher;
use clippy_utils::higher::IfOrIfLet;
use clippy_wrappers::span_lint_and_help;
use if_chain::if_chain;
use rustc_hir::{
Expand All @@ -17,6 +15,8 @@ use rustc_hir::{
};
use rustc_lint::{LateContext, LateLintPass};
use rustc_span::{sym, Span, Symbol};
use std::{collections::HashSet, hash::Hash};
use utils::fn_returns;

const LINT_MESSAGE: &str = "Unsafe usage of `expect`";
const PANIC_INDUCING_FUNCTIONS: [&str; 2] = ["panic", "bail"];
Expand Down Expand Up @@ -149,6 +149,7 @@ struct UnsafeExpectVisitor<'a, 'tcx> {
cx: &'a LateContext<'tcx>,
conditional_checker: HashSet<ConditionalChecker>,
checked_exprs: HashSet<HirId>,
linted_spans: HashSet<Span>,
}

impl UnsafeExpectVisitor<'_, '_> {
Expand All @@ -174,21 +175,23 @@ impl UnsafeExpectVisitor<'_, '_> {
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>) {
fn update_conditional_checker(
&mut self,
conditional_checkers: &HashSet<ConditionalChecker>,
set: bool,
) {
for checker in conditional_checkers {
if checker.check_type.is_safe_to_expect() {
self.checked_exprs.remove(&checker.checked_expr_hir_id);
if set {
self.conditional_checker.insert(*checker);
if checker.check_type.is_safe_to_expect() {
self.checked_exprs.insert(checker.checked_expr_hir_id);
}
} else {
if checker.check_type.is_safe_to_expect() {
self.checked_exprs.remove(&checker.checked_expr_hir_id);
}
self.conditional_checker.remove(checker);
}
self.conditional_checker.remove(&checker);
}
}

Expand Down Expand Up @@ -236,6 +239,7 @@ impl UnsafeExpectVisitor<'_, '_> {
.get_expect_info(receiver)
.map_or(true, |id| !self.checked_exprs.contains(&id));
}

args.iter().any(|arg| self.contains_unsafe_method_call(arg))
|| self.contains_unsafe_method_call(receiver)
}
Expand All @@ -252,7 +256,9 @@ impl UnsafeExpectVisitor<'_, '_> {
fn check_expr_for_unsafe_expect(&mut self, expr: &Expr) {
match &expr.kind {
ExprKind::MethodCall(path_segment, receiver, args, _) => {
if self.is_method_call_unsafe(path_segment, receiver, args) {
if self.is_method_call_unsafe(path_segment, receiver, args)
&& !self.linted_spans.contains(&expr.span)
{
span_lint_and_help(
self.cx,
UNSAFE_EXPECT,
Expand All @@ -261,6 +267,7 @@ impl UnsafeExpectVisitor<'_, '_> {
None,
"Please, use a custom error instead of `expect`",
);
self.linted_spans.insert(expr.span);
}
}
ExprKind::Call(func, args) => {
Expand Down Expand Up @@ -312,17 +319,17 @@ impl<'a, 'tcx> Visitor<'tcx> for UnsafeExpectVisitor<'a, 'tcx> {
}

// Find `if` or `if let` expressions
if let Some(higher::IfOrIfLet {
if let Some(IfOrIfLet {
cond,
then: if_expr,
r#else: _,
}) = higher::IfOrIfLet::hir(expr)
}) = 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);
self.update_conditional_checker(&conditional_checker, true);
walk_expr(self, if_expr);
self.reset_conditional_checker(conditional_checker);
self.update_conditional_checker(&conditional_checker, false);
return;
}

Expand All @@ -338,20 +345,23 @@ impl<'tcx> LateLintPass<'tcx> for UnsafeExpect {
&mut self,
cx: &LateContext<'tcx>,
_: FnKind<'tcx>,
_: &'tcx FnDecl<'tcx>,
fn_decl: &'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() {
// If the function comes from a macro expansion or does not return a Result<(), ()> or Option<()>, we don't want to analyze it.
if span.from_expansion()
|| !fn_returns(fn_decl, sym::Result) && !fn_returns(fn_decl, sym::Option)
{
return;
}

let mut visitor = UnsafeExpectVisitor {
cx,
checked_exprs: HashSet::new(),
conditional_checker: HashSet::new(),
linted_spans: HashSet::new(),
};

walk_expr(&mut visitor, body.value);
Expand Down
1 change: 1 addition & 0 deletions detectors/unsafe-unwrap/Cargo.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ clippy_utils = { workspace = true }
clippy_wrappers = { workspace = true }
dylint_linting = { workspace = true }
if_chain = { workspace = true }
utils = { workspace = true }

[package.metadata.rust-analyzer]
rustc_private = true
94 changes: 66 additions & 28 deletions detectors/unsafe-unwrap/src/lib.rs
View file
Original file line number Diff line number Diff line change
@@ -1,23 +1,22 @@
#![feature(rustc_private)]
#![allow(clippy::enum_variant_names)]

extern crate rustc_ast;
extern crate rustc_hir;
extern crate rustc_span;

use std::{collections::HashSet, hash::Hash};

use clippy_utils::higher;
use clippy_utils::higher::IfOrIfLet;
use clippy_wrappers::span_lint_and_help;
use if_chain::if_chain;
use rustc_hir::{
def::Res,
def_id::LocalDefId,
intravisit::{walk_expr, FnKind, Visitor},
BinOpKind, Body, Expr, ExprKind, FnDecl, HirId, PathSegment, QPath, UnOp,
BinOpKind, Body, Expr, ExprKind, FnDecl, HirId, LetStmt, PathSegment, QPath, UnOp,
};
use rustc_lint::{LateContext, LateLintPass};
use rustc_span::{sym, Span, Symbol};
use std::{collections::HashSet, hash::Hash};
use utils::{fn_returns, get_node_type_opt, match_type_to_str};

const LINT_MESSAGE: &str = "Unsafe usage of `unwrap`";
const PANIC_INDUCING_FUNCTIONS: [&str; 2] = ["panic", "bail"];
Expand Down Expand Up @@ -65,6 +64,13 @@ dylint_linting::declare_late_lint! {
}
}

// Enum to represent the type being unwrapped
#[derive(Clone, Copy, PartialEq, Eq, Debug)]
enum UnwrapType {
Option,
Result,
}

/// Represents the type of check performed on method call expressions to determine their safety or behavior.
#[derive(Clone, Copy, Hash, Eq, PartialEq)]
enum CheckType {
Expand Down Expand Up @@ -132,7 +138,7 @@ impl ConditionalChecker {
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),
ExprKind::Unary(UnOp::Not, condition) => Self::handle_condition(condition, true),
// 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);
Expand All @@ -150,9 +156,33 @@ struct UnsafeUnwrapVisitor<'a, 'tcx> {
cx: &'a LateContext<'tcx>,
conditional_checker: HashSet<ConditionalChecker>,
checked_exprs: HashSet<HirId>,
returns_result_or_option: bool,
}

impl UnsafeUnwrapVisitor<'_, '_> {
fn get_help_message(&self, unwrap_type: UnwrapType) -> &'static str {
match (self.returns_result_or_option, unwrap_type) {
(true, UnwrapType::Option) => "Consider using `ok_or` to convert Option to Result",
(true, UnwrapType::Result) => "Consider using the `?` operator for error propagation",
(false, UnwrapType::Option) => {
"Consider pattern matching or using `if let` instead of `unwrap`"
}
(false, UnwrapType::Result) => {
"Consider handling the error case explicitly or using `if let` instead of `unwrap`"
}
}
}

fn determine_unwrap_type(&self, receiver: &Expr<'_>) -> UnwrapType {
let type_opt = get_node_type_opt(self.cx, &receiver.hir_id);
if let Some(type_) = type_opt {
if match_type_to_str(self.cx, type_, "Result") {
return UnwrapType::Result;
}
}
UnwrapType::Option
}

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| {
Expand All @@ -175,21 +205,23 @@ impl UnsafeUnwrapVisitor<'_, '_> {
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_unwrap() {
self.checked_exprs.insert(checker.checked_expr_hir_id);
}
}
}

fn reset_conditional_checker(&mut self, conditional_checkers: HashSet<ConditionalChecker>) {
fn update_conditional_checker(
&mut self,
conditional_checkers: &HashSet<ConditionalChecker>,
set: bool,
) {
for checker in conditional_checkers {
if checker.check_type.is_safe_to_unwrap() {
self.checked_exprs.remove(&checker.checked_expr_hir_id);
if set {
self.conditional_checker.insert(*checker);
if checker.check_type.is_safe_to_unwrap() {
self.checked_exprs.insert(checker.checked_expr_hir_id);
}
} else {
if checker.check_type.is_safe_to_unwrap() {
self.checked_exprs.remove(&checker.checked_expr_hir_id);
}
self.conditional_checker.remove(checker);
}
self.conditional_checker.remove(&checker);
}
}

Expand Down Expand Up @@ -253,18 +285,20 @@ impl UnsafeUnwrapVisitor<'_, '_> {
}

impl<'a, 'tcx> Visitor<'tcx> for UnsafeUnwrapVisitor<'a, 'tcx> {
fn visit_local(&mut self, local: &'tcx rustc_hir::LetStmt<'tcx>) -> Self::Result {
fn visit_local(&mut self, local: &'tcx LetStmt<'tcx>) -> Self::Result {
if let Some(init) = local.init {
match init.kind {
ExprKind::MethodCall(path_segment, receiver, args, _) => {
if self.is_method_call_unsafe(path_segment, receiver, args) {
let unwrap_type = self.determine_unwrap_type(receiver);
let help_message = self.get_help_message(unwrap_type);
span_lint_and_help(
self.cx,
UNSAFE_UNWRAP,
local.span,
LINT_MESSAGE,
None,
"Please, use a custom error instead of `unwrap`",
help_message,
);
}
}
Expand Down Expand Up @@ -300,17 +334,17 @@ impl<'a, 'tcx> Visitor<'tcx> for UnsafeUnwrapVisitor<'a, 'tcx> {
}

// Find `if` or `if let` expressions
if let Some(higher::IfOrIfLet {
if let Some(IfOrIfLet {
cond,
then: if_expr,
r#else: _,
}) = higher::IfOrIfLet::hir(expr)
}) = 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);
self.update_conditional_checker(&conditional_checker, true);
walk_expr(self, if_expr);
self.reset_conditional_checker(conditional_checker);
self.update_conditional_checker(&conditional_checker, false);
return;
}

Expand All @@ -320,16 +354,18 @@ impl<'a, 'tcx> Visitor<'tcx> for UnsafeUnwrapVisitor<'a, 'tcx> {
if path_segment.ident.name == sym::unwrap;
then {
let receiver_hir_id = self.get_unwrap_info(receiver);
// If the receiver is `None`, then we asume that the `unwrap` is unsafe
// If the receiver is `None` or `Err`, then we assume that the `unwrap` is unsafe
let is_checked_safe = receiver_hir_id.map_or(false, |id| self.checked_exprs.contains(&id));
if !is_checked_safe {
let unwrap_type = self.determine_unwrap_type(receiver);
let help_message = self.get_help_message(unwrap_type);
span_lint_and_help(
self.cx,
UNSAFE_UNWRAP,
expr.span,
LINT_MESSAGE,
None,
"Please, use a custom error instead of `unwrap`",
help_message,
);
}
}
Expand All @@ -344,7 +380,7 @@ impl<'tcx> LateLintPass<'tcx> for UnsafeUnwrap {
&mut self,
cx: &LateContext<'tcx>,
_: FnKind<'tcx>,
_: &'tcx FnDecl<'tcx>,
fn_decl: &'tcx FnDecl<'tcx>,
body: &'tcx Body<'tcx>,
span: Span,
_: LocalDefId,
Expand All @@ -358,6 +394,8 @@ impl<'tcx> LateLintPass<'tcx> for UnsafeUnwrap {
cx,
checked_exprs: HashSet::new(),
conditional_checker: HashSet::new(),
returns_result_or_option: fn_returns(fn_decl, sym::Result)
|| fn_returns(fn_decl, sym::Option),
};

walk_expr(&mut visitor, body.value);
Expand Down

0 comments on commit de0613a

Please sign in to comment.