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.

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

Adding the expect attribute (RFC 2383) #86024

Closed
Closed
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ fn annotation_type_for_level(level: Level) -> AnnotationType {
// FIXME(#59346): Not sure how to map these two levels
Level::Cancelled | Level::FailureNote => AnnotationType::Error,
Level::Allow => panic!("Should not call with Allow"),
Level::Expect => panic!("Should not call with Expect"),
}
}

Expand Down
7 changes: 6 additions & 1 deletion compiler/rustc_errors/src/diagnostic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,12 @@ impl Diagnostic {
match self.level {
Level::Bug | Level::Fatal | Level::Error | Level::FailureNote => true,

Level::Warning | Level::Note | Level::Help | Level::Cancelled | Level::Allow => false,
Level::Warning
| Level::Note
| Level::Help
| Level::Cancelled
| Level::Allow
| Level::Expect => false,
}
}

Expand Down
99 changes: 98 additions & 1 deletion compiler/rustc_errors/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,15 @@ use rustc_span::source_map::SourceMap;
use rustc_span::{Loc, MultiSpan, Span};

use std::borrow::Cow;
use std::clone::Clone;
use std::convert::TryFrom;
use std::hash::{Hash, Hasher};
use std::iter::Iterator;
use std::mem::take;
use std::num::NonZeroUsize;
use std::panic;
use std::path::Path;
use std::result::Result::Ok;
use std::{error, fmt};

use termcolor::{Color, ColorSpec};
Expand Down Expand Up @@ -332,6 +337,10 @@ struct HandlerInner {
/// twice.
emitted_diagnostics: FxHashSet<u128>,

/// This contains all lint emission information from this compiling session
/// with the emission level `Expect`.
expected_lint_emissions: Vec<LintEmission>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: what if we turn this the other way? Say we have a FxHashMap (which is a hash map with a faster hashing algorithm that we use throughout the compiler) that represents all the #[expect] attributes that need to be fulfilled. It takes a (LintStackIndex, LintId) pair as its key and everything that we need to emit the lint (the #[expect] attribute's span, the reason, etc) as its value. Every time we emit an expected lint, we remove that lint's entry from the map, and at the end of compilation, we emit an unfulfilled_lint_expectation lint for every remaining entry in the map.

This would require a few changes to how lints are emitted so I don't feel confident to say "it's the way to go", maybe you'll want to wait on someone more qualified to weigh in.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a very interesting idea that I haven't thought about! I actually think that it won't require any major change to the lint emission. The logic already retrieves the lint level for a specific lint. The implementation would only find away to store which expectation set the level to expect in this concrete stance and mark that one as fulfilled.

I'll try to do some prototyping for this suggestion, as I believe it would be the nicer solution. As log as we can find a way to match the diagnostic to the specific expectation.


/// Stashed diagnostics emitted in one stage of the compiler that may be
/// stolen by other stages (e.g. to improve them and add more information).
/// The stashed diagnostics count towards the total error count.
Expand Down Expand Up @@ -454,6 +463,7 @@ impl Handler {
taught_diagnostics: Default::default(),
emitted_diagnostic_codes: Default::default(),
emitted_diagnostics: Default::default(),
expected_lint_emissions: Vec::new(),
stashed_diagnostics: Default::default(),
future_breakage_diagnostics: Vec::new(),
}),
Expand Down Expand Up @@ -589,6 +599,11 @@ impl Handler {
DiagnosticBuilder::new(self, Level::Allow, msg)
}

/// Construct a builder at the `Expect` level with the `msg`.
pub fn struct_expect(&self, msg: &str) -> DiagnosticBuilder<'_> {
DiagnosticBuilder::new(self, Level::Expect, msg)
}

/// Construct a builder at the `Error` level at the given `span` and with the `msg`.
pub fn struct_span_err(&self, span: impl Into<MultiSpan>, msg: &str) -> DiagnosticBuilder<'_> {
let mut result = self.struct_err(msg);
Expand Down Expand Up @@ -800,6 +815,14 @@ impl Handler {
pub fn delay_as_bug(&self, diagnostic: Diagnostic) {
self.inner.borrow_mut().delay_as_bug(diagnostic)
}

/// This method takes all lint emission information that have been issued from
/// by `HandlerInner` in this session. This will steal the collection from the
/// internal handler and should therefore only be used to check for expected
/// lints. (RFC 2383)
pub fn steal_expect_lint_emissions(&self) -> Vec<LintEmission> {
take(&mut self.inner.borrow_mut().expected_lint_emissions)
}
}

impl HandlerInner {
Expand Down Expand Up @@ -856,6 +879,14 @@ impl HandlerInner {
// Only emit the diagnostic if we've been asked to deduplicate and
// haven't already emitted an equivalent diagnostic.
if !(self.flags.deduplicate_diagnostics && already_emitted(self)) {
if diagnostic.level == Level::Expect {
LeSeulArtichaut marked this conversation as resolved.
Show resolved Hide resolved
if let Ok(emission) = LintEmission::try_from(diagnostic) {
self.expected_lint_emissions.push(emission);
}
// Diagnostics with the level `Expect` shouldn't be emitted or effect internal counters.
return;
}

self.emitter.emit_diagnostic(diagnostic);
if diagnostic.is_error() {
self.deduplicated_err_count += 1;
Expand Down Expand Up @@ -1090,6 +1121,70 @@ impl DelayedDiagnostic {
}
}

/// Used to track all emitted lints to later evaluate if expected lints have been
/// emitted.
#[must_use]
#[derive(Clone, Debug, PartialEq, Hash)]
pub struct LintEmission {
/// The lint name that was emitted.
pub lint_name: String,

/// The spans of the emission.
///
/// FIXME: We should define which span is taken from the diagnostic, this simply takes all spans
// until that is defined (xFrednet 2021-06-03)
pub lint_span: MultiSpan,
}

impl TryFrom<&Diagnostic> for LintEmission {
type Error = ();

fn try_from(diagnostic: &Diagnostic) -> Result<Self, Self::Error> {
if let Some(DiagnosticId::Lint { name, .. }) = &diagnostic.code {
Ok(LintEmission { lint_name: name.clone(), lint_span: extract_all_spans(diagnostic) })
} else {
Err(())
}
}
}

fn extract_all_spans(source: &Diagnostic) -> MultiSpan {
let mut result = Vec::new();

result.append(&mut extract_spans_from_multispan(&source.span));

// Some lints only have a suggestion span. Example: unused_variables
for sugg in &source.suggestions {
for substitution in &sugg.substitutions {
for part in &substitution.parts {
result.push(part.span);
}
}
}

// Some lints only have `SubDiagnostic`s. Example: const_item_mutation
for sub in &source.children {
result.append(&mut extract_spans_from_multispan(&sub.span));
}

MultiSpan::from_spans(result)
}

fn extract_spans_from_multispan(source: &MultiSpan) -> Vec<Span> {
let mut result: Vec<Span> = source.primary_spans().into();

// Some lints only have span_lints for notes. Example: clashing_extern_declarations
result.extend(
source
.span_labels()
.iter()
.filter(|span| span.is_primary)
.map(|span_label| span_label.span),
);

result
}

#[derive(Copy, PartialEq, Clone, Hash, Debug, Encodable, Decodable)]
pub enum Level {
Bug,
Expand All @@ -1101,6 +1196,7 @@ pub enum Level {
Cancelled,
FailureNote,
Allow,
Expect,
}

impl fmt::Display for Level {
Expand All @@ -1126,7 +1222,7 @@ impl Level {
spec.set_fg(Some(Color::Cyan)).set_intense(true);
}
FailureNote => {}
Allow | Cancelled => unreachable!(),
Allow | Expect | Cancelled => unreachable!(),
}
spec
}
Expand All @@ -1141,6 +1237,7 @@ impl Level {
FailureNote => "failure-note",
Cancelled => panic!("Shouldn't call on cancelled error"),
Allow => panic!("Shouldn't call on allowed error"),
Expect => panic!("Shouldn't call on expected error"),
}
}

Expand Down
4 changes: 4 additions & 0 deletions compiler/rustc_feature/src/builtin_attrs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,10 @@ pub const BUILTIN_ATTRIBUTES: &[BuiltinAttribute] = &[
// Lints:
ungated!(warn, Normal, template!(List: r#"lint1, lint2, ..., /*opt*/ reason = "...""#)),
ungated!(allow, Normal, template!(List: r#"lint1, lint2, ..., /*opt*/ reason = "...""#)),
gated!(
expect, Normal, template!(List: r#"lint1, lint2, ..., /*opt*/ reason = "...""#), lint_reasons,
experimental!(expect)
),
ungated!(forbid, Normal, template!(List: r#"lint1, lint2, ..., /*opt*/ reason = "...""#)),
ungated!(deny, Normal, template!(List: r#"lint1, lint2, ..., /*opt*/ reason = "...""#)),
ungated!(must_use, AssumedUsed, template!(Word, NameValueStr: "reason")),
Expand Down
4 changes: 4 additions & 0 deletions compiler/rustc_lint/src/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,7 @@ struct LintGroup {
depr: Option<LintAlias>,
}

#[derive(Debug)]
pub enum CheckLintNameResult<'a> {
Ok(&'a [LintId]),
/// Lint doesn't exist. Potentially contains a suggestion for a correct lint name.
Expand Down Expand Up @@ -373,6 +374,9 @@ impl LintStore {
Level::ForceWarn => "--force-warn",
Level::Deny => "-D",
Level::Forbid => "-F",
Level::Expect => {
unreachable!("lints with the level of `expect` should not run this code");
}
},
lint_name
);
Expand Down
Loading