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

Jump to bottom

Fix target_feature 1.1 should print the list of missing target features #109710

Closed
wants to merge 1 commit into from
Closed

Fix target_feature 1.1 should print the list of missing target features #109710

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
83 changes: 77 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 @@ -4,12 +4,12 @@ use crate::mir::ConstantKind;
use crate::ty::{self, OpaqueHiddenType, Ty, TyCtxt};
use rustc_data_structures::fx::FxIndexMap;
use rustc_data_structures::unord::UnordSet;
use rustc_errors::ErrorGuaranteed;
use rustc_errors::{pluralize, ErrorGuaranteed};
use rustc_hir as hir;
use rustc_hir::def_id::LocalDefId;
use rustc_index::bit_set::BitMatrix;
use rustc_index::{Idx, IndexVec};
use rustc_span::Span;
use rustc_span::{Span, Symbol};
use rustc_target::abi::{FieldIdx, VariantIdx};
use smallvec::SmallVec;
use std::cell::Cell;
Expand All @@ -26,7 +26,7 @@ pub enum UnsafetyViolationKind {
UnsafeFn,
}

#[derive(Copy, Clone, PartialEq, TyEncodable, TyDecodable, HashStable, Debug)]
#[derive(Clone, PartialEq, TyEncodable, TyDecodable, HashStable, Debug)]
pub enum UnsafetyViolationDetails {
CallToUnsafeFunction,
UseOfInlineAssembly,
Expand All @@ -38,10 +38,81 @@ pub enum UnsafetyViolationDetails {
AccessToUnionField,
MutationOfLayoutConstrainedField,
BorrowOfLayoutConstrainedField,
CallToFunctionWith,
CallToFunctionWith { missing_features: Vec<Symbol>, target_features: Vec<Symbol> },
}

impl UnsafetyViolationDetails {
fn missing_features(&self) -> Vec<Symbol> {
Copy link
Member

Choose a reason for hiding this comment

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

Same here, missing_features should return an Option<Vec<Symbol>> instead.

match self {
UnsafetyViolationDetails::CallToFunctionWith {
missing_features,
target_features: _,
} => missing_features.to_vec(),
_ => Vec::<Symbol>::new(),
}
}

fn target_features(&self) -> Vec<Symbol> {
Copy link
Member

Choose a reason for hiding this comment

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

This should return Option<Vec<Symbol>> here instead of an empty vector. Then in the note_missing_features function, you don't need to check is_empty any more but can directly use self.target_features()?.

match self {
UnsafetyViolationDetails::CallToFunctionWith {
missing_features: _,
target_features,
} => target_features.to_vec(),
_ => Vec::<Symbol>::new(),
}
}

pub fn note_missing_features(&self) -> Option<String> {
let target_features_symbol = self.target_features();
let target_features =
target_features_symbol.iter().map(|f| f.as_str()).collect::<Vec<&str>>();

if !target_features.is_empty() {
let target_features_str = if target_features.len() > 1 {
let l_target_features = target_features[..target_features.len() - 1].join(", ");
let last_target_feature = target_features.last().unwrap();
format!("{} and {}", l_target_features, last_target_feature)
} else {
target_features.last().unwrap().to_string()
};

Some(format!(
"the {} target feature{} being enabled in the build configuration does not remove the requirement to list {} in `#[target_feature]`",
target_features_str,
pluralize!(target_features.len()),
if target_features.len() == 1 { "it" } else { "them" },
))
} else {
None
}
}

pub fn help_missing_features(&self) -> Option<String> {
let missing_features = self.missing_features();

if !missing_features.is_empty() {
let missing_features_str = if missing_features.len() > 1 {
let l_missing_features = missing_features[..missing_features.len() - 1]
.iter()
.map(|feature| feature.as_str())
.collect::<Vec<&str>>()
.join(", ");
let last_missing_feature = missing_features.last().unwrap().as_str();
format!("{} and {}", l_missing_features, last_missing_feature)
} else {
missing_features.last().unwrap().to_string()
};

Some(format!(
"in order for the call to be safe, the context requires the following additional target feature{}: {}.",
pluralize!(missing_features.len()),
missing_features_str,
))
} else {
None
}
}

pub fn description_and_note(&self) -> (&'static str, &'static str) {
use UnsafetyViolationDetails::*;
match self {
Expand Down Expand Up @@ -91,15 +162,15 @@ impl UnsafetyViolationDetails {
"references to fields of layout constrained fields lose the constraints. Coupled \
with interior mutability, the field can be changed to invalid values",
),
CallToFunctionWith => (
CallToFunctionWith { missing_features: _, target_features: _ } => (
Copy link
Member

Choose a reason for hiding this comment

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

You can use { .. } here, this holds true for the other occurrences of this pattern in the last commit as well.

"call to function with `#[target_feature]`",
"can only be called if the required target features are available",
),
}
}
}

#[derive(Copy, Clone, PartialEq, TyEncodable, TyDecodable, HashStable, Debug)]
#[derive(Clone, PartialEq, TyEncodable, TyDecodable, HashStable, Debug)]
pub struct UnsafetyViolation {
pub source_info: SourceInfo,
pub lint_root: hir::HirId,
Expand Down
50 changes: 36 additions & 14 deletions compiler/rustc_mir_transform/src/check_unsafety.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ use rustc_middle::query::Providers;
use rustc_middle::ty::{self, TyCtxt};
use rustc_session::lint::builtin::{UNSAFE_OP_IN_UNSAFE_FN, UNUSED_UNSAFE};
use rustc_session::lint::Level;
use rustc_span::Symbol;

use std::ops::Bound;

Expand Down Expand Up @@ -287,22 +288,23 @@ impl<'tcx> UnsafetyChecker<'_, 'tcx> {
.safety;
match safety {
// `unsafe` blocks are required in safe code
Safety::Safe => violations.into_iter().for_each(|&violation| {
Safety::Safe => violations.into_iter().for_each(|violation| {
match violation.kind {
UnsafetyViolationKind::General => {}
UnsafetyViolationKind::UnsafeFn => {
bug!("`UnsafetyViolationKind::UnsafeFn` in an `Safe` context")
}
}
if !self.violations.contains(&violation) {
self.violations.push(violation)
self.violations.push(violation.clone())
}
}),
// With the RFC 2585, no longer allow `unsafe` operations in `unsafe fn`s
Safety::FnUnsafe => violations.into_iter().for_each(|&(mut violation)| {
violation.kind = UnsafetyViolationKind::UnsafeFn;
if !self.violations.contains(&violation) {
self.violations.push(violation)
Safety::FnUnsafe => violations.into_iter().for_each(|violation| {
let mut violation_copy = violation.clone();
violation_copy.kind = UnsafetyViolationKind::UnsafeFn;
if !self.violations.contains(&violation_copy) {
self.violations.push(violation_copy)
}
}),
Safety::BuiltinUnsafe => {}
Expand Down Expand Up @@ -367,9 +369,22 @@ impl<'tcx> UnsafetyChecker<'_, 'tcx> {

// Is `callee_features` a subset of `calling_features`?
if !callee_features.iter().all(|feature| self_features.contains(feature)) {
let missing_features = callee_features
.iter()
.filter(|feature| !self_features.contains(feature))
.cloned()
.collect::<Vec<Symbol>>();
let target_features = self
.tcx
.sess
.target_features
.iter()
.filter(|feature| missing_features.contains(feature))
.cloned()
.collect::<Vec<Symbol>>();
self.require_unsafe(
UnsafetyViolationKind::General,
UnsafetyViolationDetails::CallToFunctionWith,
UnsafetyViolationDetails::CallToFunctionWith { missing_features, target_features },
)
}
}
Expand Down Expand Up @@ -526,13 +541,14 @@ pub fn check_unsafety(tcx: TyCtxt<'_>, def_id: LocalDefId) {

let UnsafetyCheckResult { violations, unused_unsafes, .. } = tcx.unsafety_check_result(def_id);

for &UnsafetyViolation { source_info, lint_root, kind, details } in violations.iter() {
let details = errors::RequiresUnsafeDetail { violation: details, span: source_info.span };
for UnsafetyViolation { source_info, lint_root, kind, details } in violations.iter() {
let unsafe_details =
errors::RequiresUnsafeDetail { violation: details.clone(), span: source_info.span };

match kind {
UnsafetyViolationKind::General => {
let op_in_unsafe_fn_allowed = unsafe_op_in_unsafe_fn_allowed(tcx, lint_root);
let note_non_inherited = tcx.hir().parent_iter(lint_root).find(|(id, node)| {
let op_in_unsafe_fn_allowed = unsafe_op_in_unsafe_fn_allowed(tcx, *lint_root);
let note_non_inherited = tcx.hir().parent_iter(*lint_root).find(|(id, node)| {
if let Node::Expr(block) = node
&& let ExprKind::Block(block, _) = block.kind
&& let BlockCheckMode::UnsafeBlock(_) = block.rules
Expand All @@ -555,15 +571,21 @@ pub fn check_unsafety(tcx: TyCtxt<'_>, def_id: LocalDefId) {
tcx.sess.emit_err(errors::RequiresUnsafe {
span: source_info.span,
enclosing,
details,
details: unsafe_details,
op_in_unsafe_fn_allowed,
help: details.help_missing_features(),
note_missing_features: details.note_missing_features(),
Copy link
Member

Choose a reason for hiding this comment

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

You don't need to store this any more, you can just call note_missing_features() where you'd be using the note_missing_features field. Same goes for the help field.

});
}
UnsafetyViolationKind::UnsafeFn => tcx.emit_spanned_lint(
UNSAFE_OP_IN_UNSAFE_FN,
lint_root,
*lint_root,
source_info.span,
errors::UnsafeOpInUnsafeFn { details },
errors::UnsafeOpInUnsafeFn {
details: unsafe_details,
help: details.help_missing_features(),
note_missing_features: details.note_missing_features(),
},
),
}
}
Expand Down
59 changes: 50 additions & 9 deletions compiler/rustc_mir_transform/src/errors.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,8 @@ pub(crate) struct RequiresUnsafe {
pub details: RequiresUnsafeDetail,
pub enclosing: Option<Span>,
pub op_in_unsafe_fn_allowed: bool,
pub help: Option<String>,
pub note_missing_features: Option<String>,
}

// The primary message for this diagnostic should be '{$label} is unsafe and...',
Expand All @@ -62,9 +64,27 @@ impl<'sess, G: EmissionGuarantee> IntoDiagnostic<'sess, G> for RequiresUnsafe {
handler.struct_diagnostic(crate::fluent_generated::mir_transform_requires_unsafe);
diag.code(rustc_errors::DiagnosticId::Error("E0133".to_string()));
diag.set_span(self.span);
diag.span_label(self.span, self.details.label());
diag.note(self.details.note());
let desc = handler.eagerly_translate_to_string(self.details.label(), [].into_iter());
diag.span_label(self.span, self.details.clone().label());
if let Some(help) = self.help {
diag.help(help);
}

match self.details.violation {
Copy link
Member

Choose a reason for hiding this comment

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

The outer match is not needed here, you can just collapse it to a single if let. If it's None, you just do what you do in the _ arm.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was the old code

Copy link
Member

Choose a reason for hiding this comment

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

It's still part of the diff though? Maybe you haven't added these changes to git yet?

Copy link
Member

@est31 est31 Jun 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Ohh you meant what I am asking you to do reflects the older state of a PR, and you were asked to change it? I don't think so, but please correct me if I'm wrong. The older state also had a match, just no if let inside. Now you are doing both. Ideally you'd just have an if let, like:

if let Some(note_missing_features) = self.details.violation.note_missing_features() {
    diag.note(note_missing_features);
} else {
    diag.note(self.details.clone().note());
}

The note_missing_features function will already return None if it's not a CallToFunctionWith, we just use this fact here.

My demand is more or less the same thing I asked about in march: #109710 (comment)

If you need more clarifications, please ask!

UnsafetyViolationDetails::CallToFunctionWith {
missing_features: _,
target_features: _,
} => {
if let Some(note_missing_features) = self.note_missing_features {
diag.note(note_missing_features);
}
}
_ => {
diag.note(self.details.clone().note());
}
}

let desc =
handler.eagerly_translate_to_string(self.details.clone().label(), [].into_iter());
diag.set_arg("details", desc);
diag.set_arg("op_in_unsafe_fn_allowed", self.op_in_unsafe_fn_allowed);
if let Some(sp) = self.enclosing {
Expand All @@ -74,7 +94,7 @@ impl<'sess, G: EmissionGuarantee> IntoDiagnostic<'sess, G> for RequiresUnsafe {
}
}

#[derive(Copy, Clone)]
#[derive(Clone)]
pub(crate) struct RequiresUnsafeDetail {
pub span: Span,
pub violation: UnsafetyViolationDetails,
Expand All @@ -100,7 +120,9 @@ impl RequiresUnsafeDetail {
BorrowOfLayoutConstrainedField => {
crate::fluent_generated::mir_transform_mutation_layout_constrained_borrow_note
}
CallToFunctionWith => crate::fluent_generated::mir_transform_target_feature_call_note,
CallToFunctionWith { missing_features: _, target_features: _ } => {
crate::fluent_generated::mir_transform_target_feature_call_note
}
}
}

Expand All @@ -123,13 +145,17 @@ impl RequiresUnsafeDetail {
BorrowOfLayoutConstrainedField => {
crate::fluent_generated::mir_transform_mutation_layout_constrained_borrow_label
}
CallToFunctionWith => crate::fluent_generated::mir_transform_target_feature_call_label,
CallToFunctionWith { missing_features: _, target_features: _ } => {
crate::fluent_generated::mir_transform_target_feature_call_label
}
}
}
}

pub(crate) struct UnsafeOpInUnsafeFn {
pub details: RequiresUnsafeDetail,
pub help: Option<String>,
pub note_missing_features: Option<String>,
}

impl<'a> DecorateLint<'a, ()> for UnsafeOpInUnsafeFn {
Expand All @@ -141,10 +167,25 @@ impl<'a> DecorateLint<'a, ()> for UnsafeOpInUnsafeFn {
let desc = diag
.handler()
.expect("lint should not yet be emitted")
.eagerly_translate_to_string(self.details.label(), [].into_iter());
.eagerly_translate_to_string(self.details.clone().label(), [].into_iter());
diag.set_arg("details", desc);
diag.span_label(self.details.span, self.details.label());
diag.note(self.details.note());
diag.span_label(self.details.clone().span, self.details.clone().label());
Copy link
Member

Choose a reason for hiding this comment

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

I think you don't need to clone details to obtain the span, just to obtain the label.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's necessary, if I don't clone it then will be an error here

if let Some(help) = self.help {
diag.help(help);
}
match self.details.violation {
Copy link
Member

Choose a reason for hiding this comment

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

Same holds here, this can be collapsed into a single if let Some(note_missing_features) = self.self.details.note_missing_features().

UnsafetyViolationDetails::CallToFunctionWith {
missing_features: _,
target_features: _,
} => {
if let Some(note_missing_features) = self.note_missing_features {
diag.note(note_missing_features);
}
}
_ => {
diag.note(self.details.note());
}
}
diag
}

Expand Down
Loading