Skip to content

Commit

Permalink
Rollup merge of rust-lang#125057 - nnethercote:rustc_lint-cleanups, r…
Browse files Browse the repository at this point in the history 
…=cjgillot

`rustc_lint` cleanups

Minor improvements I found while looking through this code.

r? `@cjgillot`
  • Loading branch information
@matthiaskrgr
matthiaskrgr committed Jun 1, 2024
2 parents 52b29ac + f279d3e commit 1b9e8fa
Show file tree
Hide file tree
Showing 11 changed files with 117 additions and 104 deletions.
42 changes: 26 additions & 16 deletions compiler/rustc_lint/src/builtin.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1494,8 +1494,9 @@ impl<'tcx> LateLintPass<'tcx> for TypeAliasBounds {

let ty = cx.tcx.type_of(item.owner_id).skip_binder();
if ty.has_inherent_projections() {
// Bounds of type aliases that contain opaque types or inherent projections are respected.
// E.g: `type X = impl Trait;`, `type X = (impl Trait, Y);`, `type X = Type::Inherent;`.
// Bounds of type aliases that contain opaque types or inherent projections are
// respected. E.g: `type X = impl Trait;`, `type X = (impl Trait, Y);`, `type X =
// Type::Inherent;`.
return;
}

Expand Down Expand Up @@ -2224,7 +2225,8 @@ impl<'tcx> LateLintPass<'tcx> for ExplicitOutlivesRequirements {
hir_generics.span.shrink_to_hi().to(where_span)
};

// Due to macro expansions, the `full_where_span` might not actually contain all predicates.
// Due to macro expansions, the `full_where_span` might not actually contain all
// predicates.
if where_lint_spans.iter().all(|&sp| full_where_span.contains(sp)) {
lint_spans.push(full_where_span);
} else {
Expand Down Expand Up @@ -2601,7 +2603,8 @@ impl<'tcx> LateLintPass<'tcx> for InvalidValue {
};
// So we have at least one potentially inhabited variant. Might we have two?
let Some(second_variant) = potential_variants.next() else {
// There is only one potentially inhabited variant. So we can recursively check that variant!
// There is only one potentially inhabited variant. So we can recursively
// check that variant!
return variant_find_init_error(
cx,
ty,
Expand All @@ -2611,10 +2614,10 @@ impl<'tcx> LateLintPass<'tcx> for InvalidValue {
init,
);
};
// So we have at least two potentially inhabited variants.
// If we can prove that we have at least two *definitely* inhabited variants,
// then we have a tag and hence leaving this uninit is definitely disallowed.
// (Leaving it zeroed could be okay, depending on which variant is encoded as zero tag.)
// So we have at least two potentially inhabited variants. If we can prove that
// we have at least two *definitely* inhabited variants, then we have a tag and
// hence leaving this uninit is definitely disallowed. (Leaving it zeroed could
// be okay, depending on which variant is encoded as zero tag.)
if init == InitKind::Uninit {
let definitely_inhabited = (first_variant.1 as usize)
+ (second_variant.1 as usize)
Expand Down Expand Up @@ -2825,7 +2828,8 @@ impl<'tcx> LateLintPass<'tcx> for NamedAsmLabels {

let mut found_labels = Vec::new();

// A semicolon might not actually be specified as a separator for all targets, but it seems like LLVM accepts it always
// A semicolon might not actually be specified as a separator for all targets, but
// it seems like LLVM accepts it always.
let statements = template_str.split(|c| matches!(c, '\n' | ';'));
for statement in statements {
// If there's a comment, trim it from the statement
Expand All @@ -2838,7 +2842,8 @@ impl<'tcx> LateLintPass<'tcx> for NamedAsmLabels {
let mut chars = possible_label.chars();

let Some(start) = chars.next() else {
// Empty string means a leading ':' in this section, which is not a label.
// Empty string means a leading ':' in this section, which is not a
// label.
break 'label_loop;
};

Expand All @@ -2855,12 +2860,15 @@ impl<'tcx> LateLintPass<'tcx> for NamedAsmLabels {

// Labels continue with ASCII alphanumeric characters, _, or $
for c in chars {
// Inside a template format arg, any character is permitted for the puproses of label detection
// because we assume that it can be replaced with some other valid label string later.
// `options(raw)` asm blocks cannot have format args, so they are excluded from this special case.
// Inside a template format arg, any character is permitted for the
// puproses of label detection because we assume that it can be
// replaced with some other valid label string later. `options(raw)`
// asm blocks cannot have format args, so they are excluded from this
// special case.
if !raw && in_bracket {
if c == '{' {
// Nested brackets are not allowed in format args, this cannot be a label.
// Nested brackets are not allowed in format args, this cannot
// be a label.
break 'label_loop;
}

Expand All @@ -2873,7 +2881,8 @@ impl<'tcx> LateLintPass<'tcx> for NamedAsmLabels {
in_bracket = true;
} else {
if !(c.is_ascii_alphanumeric() || matches!(c, '_' | '$')) {
// The potential label had an invalid character inside it, it cannot be a label.
// The potential label had an invalid character inside it, it
// cannot be a label.
break 'label_loop;
}
}
Expand All @@ -2892,7 +2901,8 @@ impl<'tcx> LateLintPass<'tcx> for NamedAsmLabels {
.into_iter()
.filter_map(|label| find_label_span(label))
.collect::<Vec<Span>>();
// If there were labels but we couldn't find a span, combine the warnings and use the template span
// If there were labels but we couldn't find a span, combine the warnings and
// use the template span.
let target_spans: MultiSpan =
if spans.len() > 0 { spans.into() } else { (*template_span).into() };

Expand Down
40 changes: 23 additions & 17 deletions compiler/rustc_lint/src/context.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,8 @@ enum TargetLint {

/// A lint name that should give no warnings and have no effect.
///
/// This is used by rustc to avoid warning about old rustdoc lints before rustdoc registers them as tool lints.
/// This is used by rustc to avoid warning about old rustdoc lints before rustdoc registers
/// them as tool lints.
Ignored,
}

Expand Down Expand Up @@ -126,12 +127,16 @@ pub enum CheckLintNameResult<'a> {
Renamed(String),
/// The lint has been removed due to the given reason.
Removed(String),
/// The lint is from a tool. If the Option is None, then either
/// the lint does not exist in the tool or the code was not
/// compiled with the tool and therefore the lint was never
/// added to the `LintStore`. Otherwise the `LintId` will be
/// returned as if it where a rustc lint.
Tool(Result<&'a [LintId], (Option<&'a [LintId]>, String)>),

/// The lint is from a tool. The `LintId` will be returned as if it were a
/// rustc lint. The `Option<String>` indicates if the lint has been
/// renamed.
Tool(&'a [LintId], Option<String>),

/// The lint is from a tool. Either the lint does not exist in the tool or
/// the code was not compiled with the tool and therefore the lint was
/// never added to the `LintStore`.
MissingTool,
}

impl LintStore {
Expand Down Expand Up @@ -384,14 +389,14 @@ impl LintStore {
} else {
// 2. The tool isn't currently running, so no lints will be registered.
// To avoid giving a false positive, ignore all unknown lints.
CheckLintNameResult::Tool(Err((None, String::new())))
CheckLintNameResult::MissingTool
};
}
Some(LintGroup { lint_ids, .. }) => {
return CheckLintNameResult::Tool(Ok(lint_ids));
return CheckLintNameResult::Tool(lint_ids, None);
}
},
Some(Id(id)) => return CheckLintNameResult::Tool(Ok(slice::from_ref(id))),
Some(Id(id)) => return CheckLintNameResult::Tool(slice::from_ref(id), None),
// If the lint was registered as removed or renamed by the lint tool, we don't need
// to treat tool_lints and rustc lints different and can use the code below.
_ => {}
Expand All @@ -411,7 +416,7 @@ impl LintStore {
return if *silent {
CheckLintNameResult::Ok(lint_ids)
} else {
CheckLintNameResult::Tool(Err((Some(lint_ids), (*name).to_string())))
CheckLintNameResult::Tool(lint_ids, Some((*name).to_string()))
};
}
CheckLintNameResult::Ok(lint_ids)
Expand Down Expand Up @@ -472,17 +477,18 @@ impl LintStore {
// Reaching this would be weird, but let's cover this case anyway
if let Some(LintAlias { name, silent }) = depr {
let LintGroup { lint_ids, .. } = self.lint_groups.get(name).unwrap();
return if *silent {
CheckLintNameResult::Tool(Err((Some(lint_ids), complete_name)))
if *silent {
CheckLintNameResult::Tool(lint_ids, Some(complete_name))
} else {
CheckLintNameResult::Tool(Err((Some(lint_ids), (*name).to_string())))
};
CheckLintNameResult::Tool(lint_ids, Some((*name).to_string()))
}
} else {
CheckLintNameResult::Tool(lint_ids, Some(complete_name))
}
CheckLintNameResult::Tool(Err((Some(lint_ids), complete_name)))
}
},
Some(Id(id)) => {
CheckLintNameResult::Tool(Err((Some(slice::from_ref(id)), complete_name)))
CheckLintNameResult::Tool(slice::from_ref(id), Some(complete_name))
}
Some(other) => {
debug!("got renamed lint {:?}", other);
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_lint/src/errors.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ pub struct OverruledAttribute<'a> {
#[subdiagnostic]
pub sub: OverruledAttributeSub,
}
//

pub enum OverruledAttributeSub {
DefaultSource { id: String },
NodeSource { span: Span, reason: Option<Symbol> },
Expand Down
14 changes: 7 additions & 7 deletions compiler/rustc_lint/src/internal.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,11 @@ use rustc_span::Span;
use tracing::debug;

declare_tool_lint! {
/// The `default_hash_type` lint detects use of [`std::collections::HashMap`]/[`std::collections::HashSet`],
/// suggesting the use of `FxHashMap`/`FxHashSet`.
/// The `default_hash_type` lint detects use of [`std::collections::HashMap`] and
/// [`std::collections::HashSet`], suggesting the use of `FxHashMap`/`FxHashSet`.
///
/// This can help as `FxHasher` can perform better than the default hasher. DOS protection is not
/// required as input is assumed to be trusted.
/// This can help as `FxHasher` can perform better than the default hasher. DOS protection is
/// not required as input is assumed to be trusted.
pub rustc::DEFAULT_HASH_TYPES,
Allow,
"forbid HashMap and HashSet and suggest the FxHash* variants",
Expand All @@ -35,7 +35,7 @@ impl LateLintPass<'_> for DefaultHashTypes {
fn check_path(&mut self, cx: &LateContext<'_>, path: &Path<'_>, hir_id: HirId) {
let Res::Def(rustc_hir::def::DefKind::Struct, def_id) = path.res else { return };
if matches!(cx.tcx.hir_node(hir_id), Node::Item(Item { kind: ItemKind::Use(..), .. })) {
// don't lint imports, only actual usages
// Don't lint imports, only actual usages.
return;
}
let preferred = match cx.tcx.get_diagnostic_name(def_id) {
Expand Down Expand Up @@ -75,8 +75,8 @@ declare_tool_lint! {
/// potential query instability, such as iterating over a `HashMap`.
///
/// Due to the [incremental compilation](https://rustc-dev-guide.rust-lang.org/queries/incremental-compilation.html) model,
/// queries must return deterministic, stable results. `HashMap` iteration order can change between compilations,
/// and will introduce instability if query results expose the order.
/// queries must return deterministic, stable results. `HashMap` iteration order can change
/// between compilations, and will introduce instability if query results expose the order.
pub rustc::POTENTIAL_QUERY_INSTABILITY,
Allow,
"require explicit opt-in when using potentially unstable methods or functions",
Expand Down
10 changes: 5 additions & 5 deletions compiler/rustc_lint/src/let_underscore.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -113,11 +113,11 @@ impl<'tcx> LateLintPass<'tcx> for LetUnderscore {

let mut top_level = true;

// We recursively walk through all patterns, so that we can catch cases where the lock is nested in a pattern.
// For the basic `let_underscore_drop` lint, we only look at the top level, since there are many legitimate reasons
// to bind a sub-pattern to an `_`, if we're only interested in the rest.
// But with locks, we prefer having the chance of "false positives" over missing cases, since the effects can be
// quite catastrophic.
// We recursively walk through all patterns, so that we can catch cases where the lock is
// nested in a pattern. For the basic `let_underscore_drop` lint, we only look at the top
// level, since there are many legitimate reasons to bind a sub-pattern to an `_`, if we're
// only interested in the rest. But with locks, we prefer having the chance of "false
// positives" over missing cases, since the effects can be quite catastrophic.
local.pat.walk_always(|pat| {
let is_top_level = top_level;
top_level = false;
Expand Down
Loading

0 comments on commit 1b9e8fa

Please sign in to comment.