Skip to content

Commit

Permalink
Auto merge of #93678 - steffahn:better_unsafe_diagnostics, r=nagisa
Browse files Browse the repository at this point in the history 
Improve `unused_unsafe` lint

I’m going to add some motivation and explanation below, particularly pointing the changes in behavior from this PR.

_Edit:_ Looking for existing issues, looks like this PR fixes #88260.

_Edit2:_ Now also contains code that closes #90776.
  • Loading branch information
@bors
bors committed Feb 20, 2022
2 parents 523a1b1 + 8f8689f commit 45e2c28
Show file tree
Hide file tree
Showing 11 changed files with 3,209 additions and 244 deletions.
12 changes: 5 additions & 7 deletions compiler/rustc_codegen_llvm/src/abi.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -599,13 +599,11 @@ impl<'ll, 'tcx> FnAbiLlvmExt<'ll, 'tcx> for FnAbi<'tcx, Ty<'tcx>> {
if self.conv == Conv::CCmseNonSecureCall {
// This will probably get ignored on all targets but those supporting the TrustZone-M
// extension (thumbv8m targets).
unsafe {
llvm::AddCallSiteAttrString(
callsite,
llvm::AttributePlace::Function,
cstr::cstr!("cmse_nonsecure_call"),
);
}
llvm::AddCallSiteAttrString(
callsite,
llvm::AttributePlace::Function,
cstr::cstr!("cmse_nonsecure_call"),
);
}
}
}
Expand Down
4 changes: 4 additions & 0 deletions compiler/rustc_middle/src/hir/nested_filter.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,10 @@ use rustc_hir::intravisit::nested_filter::NestedFilter;
/// Do not visit nested item-like things, but visit nested things
/// that are inside of an item-like.
///
/// Notably, possible occurrences of bodies in non-item-like things
/// include: closures/generators, inline `const {}` blocks, and
/// constant arguments of types, e.g. in `let _: [(); /* HERE */];`.
///
/// **This is the most common choice.** A very common pattern is
/// to use `visit_all_item_likes()` as an outer loop,
/// and to have the visitor that visits the contents of each item
Expand Down
135 changes: 73 additions & 62 deletions compiler/rustc_middle/src/lint.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,77 @@ impl<'a> LintDiagnosticBuilder<'a> {
}
}

pub fn explain_lint_level_source<'s>(
sess: &'s Session,
lint: &'static Lint,
level: Level,
src: LintLevelSource,
err: &mut DiagnosticBuilder<'s>,
) {
let name = lint.name_lower();
match src {
LintLevelSource::Default => {
sess.diag_note_once(
err,
DiagnosticMessageId::from(lint),
&format!("`#[{}({})]` on by default", level.as_str(), name),
);
}
LintLevelSource::CommandLine(lint_flag_val, orig_level) => {
let flag = match orig_level {
Level::Warn => "-W",
Level::Deny => "-D",
Level::Forbid => "-F",
Level::Allow => "-A",
Level::ForceWarn => "--force-warn",
};
let hyphen_case_lint_name = name.replace('_', "-");
if lint_flag_val.as_str() == name {
sess.diag_note_once(
err,
DiagnosticMessageId::from(lint),
&format!(
"requested on the command line with `{} {}`",
flag, hyphen_case_lint_name
),
);
} else {
let hyphen_case_flag_val = lint_flag_val.as_str().replace('_', "-");
sess.diag_note_once(
err,
DiagnosticMessageId::from(lint),
&format!(
"`{} {}` implied by `{} {}`",
flag, hyphen_case_lint_name, flag, hyphen_case_flag_val
),
);
}
}
LintLevelSource::Node(lint_attr_name, src, reason) => {
if let Some(rationale) = reason {
err.note(rationale.as_str());
}
sess.diag_span_note_once(
err,
DiagnosticMessageId::from(lint),
src,
"the lint level is defined here",
);
if lint_attr_name.as_str() != name {
let level_str = level.as_str();
sess.diag_note_once(
err,
DiagnosticMessageId::from(lint),
&format!(
"`#[{}({})]` implied by `#[{}({})]`",
level_str, name, level_str, lint_attr_name
),
);
}
}
}
}

pub fn struct_lint_level<'s, 'd>(
sess: &'s Session,
lint: &'static Lint,
Expand Down Expand Up @@ -277,69 +348,9 @@ pub fn struct_lint_level<'s, 'd>(
}
}

let name = lint.name_lower();
match src {
LintLevelSource::Default => {
sess.diag_note_once(
&mut err,
DiagnosticMessageId::from(lint),
&format!("`#[{}({})]` on by default", level.as_str(), name),
);
}
LintLevelSource::CommandLine(lint_flag_val, orig_level) => {
let flag = match orig_level {
Level::Warn => "-W",
Level::Deny => "-D",
Level::Forbid => "-F",
Level::Allow => "-A",
Level::ForceWarn => "--force-warn",
};
let hyphen_case_lint_name = name.replace('_', "-");
if lint_flag_val.as_str() == name {
sess.diag_note_once(
&mut err,
DiagnosticMessageId::from(lint),
&format!(
"requested on the command line with `{} {}`",
flag, hyphen_case_lint_name
),
);
} else {
let hyphen_case_flag_val = lint_flag_val.as_str().replace('_', "-");
sess.diag_note_once(
&mut err,
DiagnosticMessageId::from(lint),
&format!(
"`{} {}` implied by `{} {}`",
flag, hyphen_case_lint_name, flag, hyphen_case_flag_val
),
);
}
}
LintLevelSource::Node(lint_attr_name, src, reason) => {
if let Some(rationale) = reason {
err.note(rationale.as_str());
}
sess.diag_span_note_once(
&mut err,
DiagnosticMessageId::from(lint),
src,
"the lint level is defined here",
);
if lint_attr_name.as_str() != name {
let level_str = level.as_str();
sess.diag_note_once(
&mut err,
DiagnosticMessageId::from(lint),
&format!(
"`#[{}({})]` implied by `#[{}({})]`",
level_str, name, level_str, lint_attr_name
),
);
}
}
}
explain_lint_level_source(sess, lint, level, src, &mut err);

let name = lint.name_lower();
let is_force_warn = matches!(level, Level::ForceWarn);
err.code(DiagnosticId::Lint { name, has_future_breakage, is_force_warn });

Expand Down
43 changes: 37 additions & 6 deletions compiler/rustc_middle/src/mir/query.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

use crate::mir::{Body, Promoted};
use crate::ty::{self, Ty, TyCtxt};
use rustc_data_structures::sync::Lrc;
use rustc_data_structures::stable_map::FxHashMap;
use rustc_data_structures::vec_map::VecMap;
use rustc_errors::ErrorReported;
use rustc_hir as hir;
Expand Down Expand Up @@ -114,13 +114,44 @@ pub struct UnsafetyViolation {
pub details: UnsafetyViolationDetails,
}

#[derive(Clone, TyEncodable, TyDecodable, HashStable, Debug)]
#[derive(Copy, Clone, PartialEq, TyEncodable, TyDecodable, HashStable, Debug)]
pub enum UnusedUnsafe {
/// `unsafe` block contains no unsafe operations
/// > ``unnecessary `unsafe` block``
Unused,
/// `unsafe` block nested under another (used) `unsafe` block
/// > ``… because it's nested under this `unsafe` block``
InUnsafeBlock(hir::HirId),
/// `unsafe` block nested under `unsafe fn`
/// > ``… because it's nested under this `unsafe fn` ``
///
/// the second HirId here indicates the first usage of the `unsafe` block,
/// which allows retrival of the LintLevelSource for why that operation would
/// have been permitted without the block
InUnsafeFn(hir::HirId, hir::HirId),
}

#[derive(Copy, Clone, PartialEq, TyEncodable, TyDecodable, HashStable, Debug)]
pub enum UsedUnsafeBlockData {
SomeDisallowedInUnsafeFn,
// the HirId here indicates the first usage of the `unsafe` block
// (i.e. the one that's first encountered in the MIR traversal of the unsafety check)
AllAllowedInUnsafeFn(hir::HirId),
}

#[derive(TyEncodable, TyDecodable, HashStable, Debug)]
pub struct UnsafetyCheckResult {
/// Violations that are propagated *upwards* from this function.
pub violations: Lrc<[UnsafetyViolation]>,
/// `unsafe` blocks in this function, along with whether they are used. This is
/// used for the "unused_unsafe" lint.
pub unsafe_blocks: Lrc<[(hir::HirId, bool)]>,
pub violations: Vec<UnsafetyViolation>,

/// Used `unsafe` blocks in this function. This is used for the "unused_unsafe" lint.
///
/// The keys are the used `unsafe` blocks, the UnusedUnsafeKind indicates whether
/// or not any of the usages happen at a place that doesn't allow `unsafe_op_in_unsafe_fn`.
pub used_unsafe_blocks: FxHashMap<hir::HirId, UsedUnsafeBlockData>,

/// This is `Some` iff the item is not a closure.
pub unused_unsafes: Option<Vec<(hir::HirId, UnusedUnsafe)>>,
}

rustc_index::newtype_index! {
Expand Down
22 changes: 5 additions & 17 deletions compiler/rustc_mir_build/src/build/block.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@ use crate::build::ForGuard::OutsideGuard;
use crate::build::{BlockAnd, BlockAndExtension, BlockFrame, Builder};
use rustc_middle::mir::*;
use rustc_middle::thir::*;
use rustc_session::lint::builtin::UNSAFE_OP_IN_UNSAFE_FN;
use rustc_session::lint::Level;
use rustc_span::Span;

impl<'a, 'tcx> Builder<'a, 'tcx> {
Expand Down Expand Up @@ -209,28 +207,18 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
block.unit()
}

/// If we are changing the safety mode, create a new source scope
/// If we are entering an unsafe block, create a new source scope
fn update_source_scope_for_safety_mode(&mut self, span: Span, safety_mode: BlockSafety) {
debug!("update_source_scope_for({:?}, {:?})", span, safety_mode);
let new_unsafety = match safety_mode {
BlockSafety::Safe => None,
BlockSafety::BuiltinUnsafe => Some(Safety::BuiltinUnsafe),
BlockSafety::Safe => return,
BlockSafety::BuiltinUnsafe => Safety::BuiltinUnsafe,
BlockSafety::ExplicitUnsafe(hir_id) => {
match self.in_scope_unsafe {
Safety::Safe => {}
// no longer treat `unsafe fn`s as `unsafe` contexts (see RFC #2585)
Safety::FnUnsafe
if self.tcx.lint_level_at_node(UNSAFE_OP_IN_UNSAFE_FN, hir_id).0
!= Level::Allow => {}
_ => return,
}
self.in_scope_unsafe = Safety::ExplicitUnsafe(hir_id);
Some(Safety::ExplicitUnsafe(hir_id))
Safety::ExplicitUnsafe(hir_id)
}
};

if let Some(unsafety) = new_unsafety {
self.source_scope = self.new_source_scope(span, LintLevel::Inherited, Some(unsafety));
}
self.source_scope = self.new_source_scope(span, LintLevel::Inherited, Some(new_unsafety));
}
}
Loading

0 comments on commit 45e2c28

Please sign in to comment.