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.

Sign up for GitHub

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

Improve unused_unsafe lint #93678

Merged
merged 1 commit into from
Feb 20, 2022
Merged

Improve unused_unsafe lint #93678

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 5 additions & 7 deletions compiler/rustc_codegen_llvm/src/abi.rs
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It isn't entirely clear to me that lower HirId necessarily means that its an eariler use (within a block). Could you add a note to the comment here that this relationship holds?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay

Copy link
Member Author

@steffahn steffahn Feb 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kind-of derived that by reading this part in the original implementation

    // The unused unsafe blocks might not be in source order; sort them so that the unused unsafe
    // error messages are properly aligned and the issue-45107 and lint-unused-unsafe tests pass.
    unsafe_unused.sort_by_cached_key(|hir_id| tcx.hir().span(*hir_id));

I don’t know how true it is that HirId really corresponds in this way to source order. In any case note that the only usage of this HirId here is for providing one instance of where the unsafe_op_in_unsafe_fn lint level was defined for one of the unsafe operations in the block. (Typically, it’s going to be the same source for all usages anyways.) There is no strict need for it to be the first one, it’s more about being consistent and not providing more information than necessary, so arbitrarily choosing the first one seems like a decent approach. (In particular, changing the unsafe_op_in_unsafe_fn lint level for any of the operations would already make the unused_unsafe warning disappear).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, just for sanity, it might make sense to somehow determine (i.e. ask somebody) whether or not HirId order does actually correspond (to some degree) to order of appearance in the source code. Or to order of appearance in some other immediate representation, e.g. of the AST, in case that makes a difference.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, the code above

unsafe_unused.sort_by_cached_key(|hir_id| tcx.hir().span(*hir_id));

is sorting the spans of the HIR node, not the HirIds themselves, and those do have direct relationship with the appearance order in the source code.

It might be better if in the code below, where the comparison and logic for this invariant is implemented (that is *entry = cmp::min(*entry, new_usage);), the spans were compared as well?

Though my understanding of your explanation that it doesn't actually matter that this HirId actually refers to the first unsafe use in the source-order, is that correct? I think its okay too and the adjusted comment is clear enough, I guess, what the “first” here really means.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorting the spans of the HIR node, not the HirIds themselves

Oh… I don’t know how I missed that. Perhaps misreading it once, or even misremembering and then never double-checking.

Maybe the more systematic approach would then be to just take the first one appearing in MIR (i.e. just the first one we come across in the MIR traversal)? 🤔

Indeed it doesn’t really matter, I would appreciate reproducible compilation results (in terms of the set of warnings) though, even in the rare edge cases where this logic actually matters, so some sort of systematic approach is good.

Right, I think I’ll change it to first appearance in MIR then, this is particularly straightforward as it’s just going to be keeping the first operation that’s encountered, making it the cheapest option, too.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’ve changed it now

}

#[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
Open in desktop
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