Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add diagnostics for unsafe_op_in_unsafe_fn #18135

Merged
merged 1 commit into from
Sep 18, 2024
Merged
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
10 changes: 5 additions & 5 deletions crates/hir-ty/src/diagnostics/unsafe_check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,10 @@ use crate::{
db::HirDatabase, utils::is_fn_unsafe_to_call, InferenceResult, Interner, TyExt, TyKind,
};

pub fn missing_unsafe(db: &dyn HirDatabase, def: DefWithBodyId) -> Vec<ExprId> {
/// Returns `(unsafe_exprs, fn_is_unsafe)`.
///
/// If `fn_is_unsafe` is false, `unsafe_exprs` are hard errors. If true, they're `unsafe_op_in_unsafe_fn`.
pub fn missing_unsafe(db: &dyn HirDatabase, def: DefWithBodyId) -> (Vec<ExprId>, bool) {
let _p = tracing::info_span!("missing_unsafe").entered();

let mut res = Vec::new();
Expand All @@ -24,9 +27,6 @@ pub fn missing_unsafe(db: &dyn HirDatabase, def: DefWithBodyId) -> Vec<ExprId> {
| DefWithBodyId::VariantId(_)
| DefWithBodyId::InTypeConstId(_) => false,
};
if is_unsafe {
return res;
}

let body = db.body(def);
let infer = db.infer(def);
Expand All @@ -36,7 +36,7 @@ pub fn missing_unsafe(db: &dyn HirDatabase, def: DefWithBodyId) -> Vec<ExprId> {
}
});

res
(res, is_unsafe)
}

pub struct UnsafeExpr {
Expand Down
2 changes: 2 additions & 0 deletions crates/hir/src/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,8 @@ pub struct PrivateField {
#[derive(Debug)]
pub struct MissingUnsafe {
pub expr: InFile<AstPtr<ast::Expr>>,
/// If true, the diagnostics is an `unsafe_op_in_unsafe_fn` lint instead of a hard error.
pub only_lint: bool,
}

#[derive(Debug)]
Expand Down
5 changes: 3 additions & 2 deletions crates/hir/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1884,9 +1884,10 @@ impl DefWithBody {
);
}

for expr in hir_ty::diagnostics::missing_unsafe(db, self.into()) {
let (unafe_exprs, only_lint) = hir_ty::diagnostics::missing_unsafe(db, self.into());
for expr in unafe_exprs {
match source_map.expr_syntax(expr) {
Ok(expr) => acc.push(MissingUnsafe { expr }.into()),
Ok(expr) => acc.push(MissingUnsafe { expr, only_lint }.into()),
Err(SyntheticSyntax) => {
// FIXME: Here and elsewhere in this file, the `expr` was
// desugared, report or assert that this doesn't happen.
Expand Down
31 changes: 30 additions & 1 deletion crates/ide-diagnostics/src/handlers/missing_unsafe.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,14 @@ use crate::{fix, Diagnostic, DiagnosticCode, DiagnosticsContext};
//
// This diagnostic is triggered if an operation marked as `unsafe` is used outside of an `unsafe` function or block.
pub(crate) fn missing_unsafe(ctx: &DiagnosticsContext<'_>, d: &hir::MissingUnsafe) -> Diagnostic {
let code = if d.only_lint {
DiagnosticCode::RustcLint("unsafe_op_in_unsafe_fn")
} else {
DiagnosticCode::RustcHardError("E0133")
};
Diagnostic::new_with_syntax_node_ptr(
ctx,
DiagnosticCode::RustcHardError("E0133"),
code,
"this operation is unsafe and requires an unsafe function or block",
d.expr.map(|it| it.into()),
)
Expand Down Expand Up @@ -562,6 +567,30 @@ fn main() {
ed2021::safe();
ed2024::not_safe();
//^^^^^^^^^^^^^^^^^^💡 error: this operation is unsafe and requires an unsafe function or block
}
"#,
)
}

#[test]
fn unsafe_op_in_unsafe_fn_allowed_by_default() {
check_diagnostics(
r#"
unsafe fn foo(p: *mut i32) {
*p = 123;
}
"#,
)
}

#[test]
fn unsafe_op_in_unsafe_fn() {
check_diagnostics(
r#"
#![warn(unsafe_op_in_unsafe_fn)]
unsafe fn foo(p: *mut i32) {
*p = 123;
//^^💡 warn: this operation is unsafe and requires an unsafe function or block
}
"#,
)
Expand Down
17 changes: 15 additions & 2 deletions crates/ide-diagnostics/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -491,7 +491,7 @@ pub fn semantic_diagnostics(

// The edition isn't accurate (each diagnostics may have its own edition due to macros),
// but it's okay as it's only being used for error recovery.
handle_lint_attributes(
handle_lints(
&ctx.sema,
&mut FxHashMap::default(),
&mut lints,
Expand Down Expand Up @@ -551,14 +551,27 @@ fn build_group_dict(
map_with_prefixes.into_iter().map(|(k, v)| (k.strip_prefix(prefix).unwrap(), v)).collect()
}

fn handle_lint_attributes(
/// Thd default severity for lints that are not warn by default.
// FIXME: Autogenerate this instead of write manually.
static LINTS_DEFAULT_SEVERITY: LazyLock<FxHashMap<&str, Severity>> =
LazyLock::new(|| FxHashMap::from_iter([("unsafe_op_in_unsafe_fn", Severity::Allow)]));

fn handle_lints(
sema: &Semantics<'_, RootDatabase>,
cache: &mut FxHashMap<HirFileId, FxHashMap<SmolStr, SeverityAttr>>,
diagnostics: &mut [(InFile<SyntaxNode>, &mut Diagnostic)],
cache_stack: &mut Vec<HirFileId>,
edition: Edition,
) {
for (node, diag) in diagnostics {
let lint = match diag.code {
DiagnosticCode::RustcLint(lint) | DiagnosticCode::Clippy(lint) => lint,
_ => panic!("non-lint passed to `handle_lints()`"),
};
if let Some(&default_severity) = LINTS_DEFAULT_SEVERITY.get(lint) {
diag.severity = default_severity;
}

let mut diag_severity = fill_lint_attrs(sema, node, cache, cache_stack, diag, edition);

if let outline_diag_severity @ Some(_) =
Expand Down