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

rustc_lint cleanups #125057

Merged
merged 3 commits into from
Jun 3, 2024
Merged

rustc_lint cleanups #125057

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
22ca74f
Fix up comments.
nnethercote May 7, 2024
d070e89
Reduce some `pub` exposure.
nnethercote May 8, 2024
32c8a12
Tweak `CheckLintNameResult::Tool`.
nnethercote May 7, 2024
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
42 changes: 26 additions & 16 deletions compiler/rustc_lint/src/builtin.rs
View file
Open in desktop
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
42 changes: 23 additions & 19 deletions compiler/rustc_lint/src/context.rs
View file
Open in desktop
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,18 +477,17 @@ 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)))
}
Some(Id(id)) => CheckLintNameResult::Tool(slice::from_ref(id), Some(complete_name)),
Some(other) => {
debug!("got renamed lint {:?}", other);
CheckLintNameResult::NoLint(None)
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_lint/src/errors.rs
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Loading