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

perf: Delay formatting of lint messages until they are know to be used #67755

Closed
wants to merge 1 commit into from
Closed

perf: Delay formatting of lint messages until they are know to be used #67755

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
32 changes: 21 additions & 11 deletions src/librustc/lint/context.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -497,8 +497,13 @@ pub trait LintContext: Sized {
fn sess(&self) -> &Session;
fn lints(&self) -> &LintStore;

fn lookup_and_emit<S: Into<MultiSpan>>(&self, lint: &'static Lint, span: Option<S>, msg: &str) {
self.lookup(lint, span, msg).emit();
fn lookup_and_emit<S: Into<MultiSpan>>(
&self,
lint: &'static Lint,
span: Option<S>,
msg: impl std::fmt::Display,
) {
self.lookup(lint, span, &msg).emit();
}

fn lookup_and_emit_with_diagnostics<S: Into<MultiSpan>>(
Expand All @@ -508,7 +513,7 @@ pub trait LintContext: Sized {
msg: &str,
diagnostic: BuiltinLintDiagnostics,
) {
let mut db = self.lookup(lint, span, msg);
let mut db = self.lookup(lint, span, &msg);
diagnostic.run(self.sess(), &mut db);
db.emit();
}
Expand All @@ -517,21 +522,26 @@ pub trait LintContext: Sized {
&self,
lint: &'static Lint,
span: Option<S>,
msg: &str,
msg: impl std::fmt::Display,
) -> DiagnosticBuilder<'_>;

/// Emit a lint at the appropriate level, for a particular span.
fn span_lint<S: Into<MultiSpan>>(&self, lint: &'static Lint, span: S, msg: &str) {
fn span_lint<S: Into<MultiSpan>>(
&self,
lint: &'static Lint,
span: S,
msg: impl std::fmt::Display,
) {
self.lookup_and_emit(lint, Some(span), msg);
}

fn struct_span_lint<S: Into<MultiSpan>>(
&self,
lint: &'static Lint,
span: S,
msg: &str,
msg: impl std::fmt::Display,
) -> DiagnosticBuilder<'_> {
self.lookup(lint, Some(span), msg)
self.lookup(lint, Some(span), &msg)
}

/// Emit a lint and note at the appropriate level, for a particular span.
Expand All @@ -543,7 +553,7 @@ pub trait LintContext: Sized {
note_span: Span,
note: &str,
) {
let mut err = self.lookup(lint, Some(span), msg);
let mut err = self.lookup(lint, Some(span), &msg);
if note_span == span {
err.note(note);
} else {
Expand All @@ -554,7 +564,7 @@ pub trait LintContext: Sized {

/// Emit a lint and help at the appropriate level, for a particular span.
fn span_lint_help(&self, lint: &'static Lint, span: Span, msg: &str, help: &str) {
let mut err = self.lookup(lint, Some(span), msg);
let mut err = self.lookup(lint, Some(span), &msg);
self.span_lint(lint, span, msg);
err.span_help(span, help);
err.emit();
Expand Down Expand Up @@ -646,7 +656,7 @@ impl LintContext for LateContext<'_, '_> {
&self,
lint: &'static Lint,
span: Option<S>,
msg: &str,
msg: impl std::fmt::Display,
) -> DiagnosticBuilder<'_> {
let hir_id = self.last_node_with_lint_attrs;

Expand All @@ -673,7 +683,7 @@ impl LintContext for EarlyContext<'_> {
&self,
lint: &'static Lint,
span: Option<S>,
msg: &str,
msg: impl std::fmt::Display,
) -> DiagnosticBuilder<'_> {
self.builder.struct_lint(lint, span.map(|s| s.into()), msg)
}
Expand Down
2 changes: 1 addition & 1 deletion src/librustc/lint/levels.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -471,7 +471,7 @@ impl<'a> LintLevelsBuilder<'a> {
&self,
lint: &'static Lint,
span: Option<MultiSpan>,
msg: &str,
msg: impl std::fmt::Display,
) -> DiagnosticBuilder<'a> {
let (level, src) = self.sets.get_lint_level(lint, self.cur, None, self.sess);
lint::struct_lint_level(self.sess, lint, level, src, span, msg)
Expand Down
25 changes: 19 additions & 6 deletions src/librustc/lint/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -437,14 +437,27 @@ pub fn struct_lint_level<'a>(
level: Level,
src: LintSource,
span: Option<MultiSpan>,
msg: &str,
msg: impl std::fmt::Display,
) -> DiagnosticBuilder<'a> {
struct_lint_level_(sess, lint, level, src, span, &msg)
}

fn struct_lint_level_<'a>(
sess: &'a Session,
lint: &'static Lint,
level: Level,
src: LintSource,
span: Option<MultiSpan>,
msg: &dyn std::fmt::Display,
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm... this does nothing though to prevent similar mistakes in the future.

Perhaps we should:

  • Set the initial message to "" (so there should be no allocation).

  • Invert control: Instead of returning a DiagnosticBuilder<'_>, we take in a function impl Fn(&mut DiagnosticBuilder<'_>) which allows the caller to make modifications, but ultimately this is never called on Level::Allow, thereby avoiding allocations that follow in e.g.

    rust/src/librustc_lint/builtin.rs

    Lines 190 to 200 in 055ce62

    let binding = match binding_annot {
    hir::BindingAnnotation::Unannotated => None,
    hir::BindingAnnotation::Mutable => Some("mut"),
    hir::BindingAnnotation::Ref => Some("ref"),
    hir::BindingAnnotation::RefMut => Some("ref mut"),
    };
    let ident = if let Some(binding) = binding {
    format!("{} {}", binding, ident)
    } else {
    ident.to_string()
    };

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the idea of changing the api surface to always defer evaluation of the message would be a relatively painless big win.

Copy link
Contributor

Choose a reason for hiding this comment

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

Filed #67927.

) -> DiagnosticBuilder<'a> {
let mut err = match (level, span) {
(Level::Allow, _) => return sess.diagnostic().struct_dummy(),
(Level::Warn, Some(span)) => sess.struct_span_warn(span, msg),
(Level::Warn, None) => sess.struct_warn(msg),
(Level::Deny, Some(span)) | (Level::Forbid, Some(span)) => sess.struct_span_err(span, msg),
(Level::Deny, None) | (Level::Forbid, None) => sess.struct_err(msg),
(Level::Warn, Some(span)) => sess.struct_span_warn(span, &msg.to_string()),
(Level::Warn, None) => sess.struct_warn(&msg.to_string()),
(Level::Deny, Some(span)) | (Level::Forbid, Some(span)) => {
sess.struct_span_err(span, &msg.to_string())
}
(Level::Deny, None) | (Level::Forbid, None) => sess.struct_err(&msg.to_string()),
};

// Check for future incompatibility lints and issue a stronger warning.
Expand Down Expand Up @@ -536,7 +549,7 @@ pub fn struct_lint_level<'a>(

if let Some(future_incompatible) = future_incompatible {
const STANDARD_MESSAGE: &str = "this was previously accepted by the compiler but is being phased out; \
it will become a hard error";
it will become a hard error";

let explanation = if lint_id == LintId::of(builtin::UNSTABLE_NAME_COLLISIONS) {
"once this method is added to the standard library, \
Expand Down
12 changes: 6 additions & 6 deletions src/librustc/ty/context.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@ fn validate_hir_id_for_typeck_tables(
ty::tls::with(|tcx| {
bug!(
"node {} with HirId::owner {:?} cannot be placed in \
TypeckTables with local_id_root {:?}",
TypeckTables with local_id_root {:?}",
tcx.hir().node_to_string(hir_id),
DefId::local(hir_id.owner),
local_id_root
Expand Down Expand Up @@ -2553,7 +2553,7 @@ impl<'tcx> TyCtxt<'tcx> {
lint: &'static Lint,
hir_id: HirId,
span: S,
msg: &str,
msg: impl std::fmt::Display,
) {
self.struct_span_lint_hir(lint, hir_id, span.into(), msg).emit()
}
Expand All @@ -2563,7 +2563,7 @@ impl<'tcx> TyCtxt<'tcx> {
lint: &'static Lint,
hir_id: HirId,
span: S,
msg: &str,
msg: impl std::fmt::Display,
note: &str,
) {
let mut err = self.struct_span_lint_hir(lint, hir_id, span.into(), msg);
Expand All @@ -2576,7 +2576,7 @@ impl<'tcx> TyCtxt<'tcx> {
lint: &'static Lint,
id: hir::HirId,
span: S,
msg: &str,
msg: impl std::fmt::Display,
note: &str,
) {
let mut err = self.struct_span_lint_hir(lint, id, span.into(), msg);
Expand Down Expand Up @@ -2629,7 +2629,7 @@ impl<'tcx> TyCtxt<'tcx> {
lint: &'static Lint,
hir_id: HirId,
span: S,
msg: &str,
msg: impl std::fmt::Display,
) -> DiagnosticBuilder<'tcx> {
let (level, src) = self.lint_level_at_node(lint, hir_id);
lint::struct_lint_level(self.sess, lint, level, src, Some(span.into()), msg)
Expand All @@ -2639,7 +2639,7 @@ impl<'tcx> TyCtxt<'tcx> {
self,
lint: &'static Lint,
id: HirId,
msg: &str,
msg: impl std::fmt::Display,
) -> DiagnosticBuilder<'tcx> {
let (level, src) = self.lint_level_at_node(lint, id);
lint::struct_lint_level(self.sess, lint, level, src, None, msg)
Expand Down
27 changes: 15 additions & 12 deletions src/librustc_lint/builtin.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,8 +109,11 @@ impl BoxPointers {
fn check_heap_type(&self, cx: &LateContext<'_, '_>, span: Span, ty: Ty<'_>) {
for leaf_ty in ty.walk() {
if leaf_ty.is_box() {
let m = format!("type uses owned (Box type) pointers: {}", ty);
cx.span_lint(BOX_POINTERS, span, &m);
cx.span_lint(
BOX_POINTERS,
span,
format_args!("type uses owned (Box type) pointers: {}", ty),
);
}
}
}
Expand Down Expand Up @@ -235,8 +238,8 @@ impl EarlyLintPass for UnsafeCode {
cx,
attr.span,
"`allow_internal_unsafe` allows defining \
macros using unsafe without triggering \
the `unsafe_code` lint at their call site",
macros using unsafe without triggering \
the `unsafe_code` lint at their call site",
);
}
}
Expand Down Expand Up @@ -379,7 +382,7 @@ impl MissingDoc {
cx.span_lint(
MISSING_DOCS,
cx.tcx.sess.source_map().def_span(sp),
&format!("missing documentation for {}", desc),
format_args!("missing documentation for {}", desc),
);
}
}
Expand Down Expand Up @@ -563,7 +566,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for MissingCopyImplementations {
MISSING_COPY_IMPLEMENTATIONS,
item.span,
"type could implement `Copy`; consider adding `impl \
Copy`",
Copy`",
)
}
}
Expand Down Expand Up @@ -617,7 +620,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for MissingDebugImplementations {
MISSING_DEBUG_IMPLEMENTATIONS,
item.span,
"type does not implement `fmt::Debug`; consider adding `#[derive(Debug)]` \
or a manual implementation",
or a manual implementation",
)
}
}
Expand Down Expand Up @@ -663,7 +666,7 @@ impl EarlyLintPass for AnonymousParameters {
.span_suggestion(
arg.pat.span,
"Try naming the parameter or explicitly \
ignoring it",
ignoring it",
format!("_: {}", ty_snip),
appl,
)
Expand Down Expand Up @@ -783,7 +786,7 @@ impl UnusedDocComment {
if is_macro_expansion {
err.help(
"to document an item produced by a macro, \
the macro must produce the documentation as part of its expansion",
the macro must produce the documentation as part of its expansion",
);
}

Expand Down Expand Up @@ -1249,7 +1252,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for TrivialConstraints {
span,
&format!(
"{} bound {} does not depend on any type \
or lifetime parameters",
or lifetime parameters",
predicate_kind_name, predicate
),
);
Expand Down Expand Up @@ -1473,7 +1476,7 @@ impl KeywordIdents {
let mut lint = cx.struct_span_lint(
KEYWORD_IDENTS,
ident.span,
&format!("`{}` is a keyword in the {} edition", ident, next_edition),
format_args!("`{}` is a keyword in the {} edition", ident, next_edition),
);
lint.span_suggestion(
ident.span,
Expand Down Expand Up @@ -2033,7 +2036,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for InvalidValue {
err.span_label(
expr.span,
"help: use `MaybeUninit<T>` instead, \
and only call `assume_init` after initialization is done",
and only call `assume_init` after initialization is done",
);
if let Some(span) = span {
err.span_note(span, &msg);
Expand Down