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

lint reasons (RFC 2383, part 1) #54683

Merged
merged 4 commits into from
Oct 28, 2018
Merged
Show file tree
Hide file tree
Changes from all 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
99 changes: 82 additions & 17 deletions src/librustc/lint/levels.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ use rustc_data_structures::stable_hasher::{HashStable, ToStableHashKey,
use session::Session;
use syntax::ast;
use syntax::attr;
use syntax::feature_gate;
use syntax::source_map::MultiSpan;
use syntax::symbol::Symbol;
use util::nodemap::FxHashMap;
Expand Down Expand Up @@ -199,8 +200,7 @@ impl<'a> LintLevelsBuilder<'a> {
let store = self.sess.lint_store.borrow();
let sess = self.sess;
let bad_attr = |span| {
span_err!(sess, span, E0452,
"malformed lint attribute");
struct_span_err!(sess, span, E0452, "malformed lint attribute")
};
for attr in attrs {
let level = match Level::from_str(&attr.name().as_str()) {
Expand All @@ -211,19 +211,76 @@ impl<'a> LintLevelsBuilder<'a> {
let meta = unwrap_or!(attr.meta(), continue);
attr::mark_used(attr);

let metas = if let Some(metas) = meta.meta_item_list() {
let mut metas = if let Some(metas) = meta.meta_item_list() {
metas
} else {
bad_attr(meta.span);
continue
let mut err = bad_attr(meta.span);
err.emit();
continue;
};

if metas.is_empty() {
// FIXME (#55112): issue unused-attributes lint for `#[level()]`
continue;
}

// Before processing the lint names, look for a reason (RFC 2383)
// at the end.
let mut reason = None;
let tail_li = &metas[metas.len()-1];
Copy link
Contributor

Choose a reason for hiding this comment

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

This will panic on #[allow()], no?

Copy link
Member Author

Choose a reason for hiding this comment

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

... right. 😰 😥 💀

Copy link
Member Author

Choose a reason for hiding this comment

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

Should this trigger unused_attributes? (As recently discussed for empty derives and inappropriate #[must_use])

Copy link
Contributor

Choose a reason for hiding this comment

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

Feels reasonable to me to trigger unused_attributes for empty allow; esp. if we do it for empty derives.

if let Some(item) = tail_li.meta_item() {
match item.node {
ast::MetaItemKind::Word => {} // actual lint names handled later
ast::MetaItemKind::NameValue(ref name_value) => {
let gate_reasons = !self.sess.features_untracked().lint_reasons;
if item.ident == "reason" {
// found reason, reslice meta list to exclude it
metas = &metas[0..metas.len()-1];
// FIXME (#55112): issue unused-attributes lint if we thereby
// don't have any lint names (`#[level(reason = "foo")]`)
if let ast::LitKind::Str(rationale, _) = name_value.node {
if gate_reasons {
feature_gate::emit_feature_err(
&self.sess.parse_sess,
"lint_reasons",
item.span,
feature_gate::GateIssue::Language,
"lint reasons are experimental"
);
} else {
reason = Some(rationale);
}
} else {
let mut err = bad_attr(name_value.span);
err.help("reason must be a string literal");
err.emit();
}
} else {
let mut err = bad_attr(item.span);
err.emit();
}
},
ast::MetaItemKind::List(_) => {
let mut err = bad_attr(item.span);
err.emit();
}
}
}

for li in metas {
let word = match li.word() {
Some(word) => word,
None => {
bad_attr(li.span);
continue
let mut err = bad_attr(li.span);
if let Some(item) = li.meta_item() {
if let ast::MetaItemKind::NameValue(_) = item.node {
if item.ident == "reason" {
err.help("reason in lint attribute must come last");
}
}
}
err.emit();
continue;
}
};
let tool_name = if let Some(lint_tool) = word.is_scoped() {
Expand All @@ -245,7 +302,7 @@ impl<'a> LintLevelsBuilder<'a> {
let name = word.name();
match store.check_lint_name(&name.as_str(), tool_name) {
CheckLintNameResult::Ok(ids) => {
let src = LintSource::Node(name, li.span);
let src = LintSource::Node(name, li.span, reason);
for id in ids {
specs.insert(*id, (level, src));
}
Expand All @@ -255,7 +312,9 @@ impl<'a> LintLevelsBuilder<'a> {
match result {
Ok(ids) => {
let complete_name = &format!("{}::{}", tool_name.unwrap(), name);
let src = LintSource::Node(Symbol::intern(complete_name), li.span);
let src = LintSource::Node(
Symbol::intern(complete_name), li.span, reason
);
for id in ids {
specs.insert(*id, (level, src));
}
Expand Down Expand Up @@ -286,7 +345,9 @@ impl<'a> LintLevelsBuilder<'a> {
Applicability::MachineApplicable,
).emit();

let src = LintSource::Node(Symbol::intern(&new_lint_name), li.span);
let src = LintSource::Node(
Symbol::intern(&new_lint_name), li.span, reason
);
for id in ids {
specs.insert(*id, (level, src));
}
Expand Down Expand Up @@ -368,11 +429,11 @@ impl<'a> LintLevelsBuilder<'a> {
};
let forbidden_lint_name = match forbid_src {
LintSource::Default => id.to_string(),
LintSource::Node(name, _) => name.to_string(),
LintSource::Node(name, _, _) => name.to_string(),
LintSource::CommandLine(name) => name.to_string(),
};
let (lint_attr_name, lint_attr_span) = match *src {
LintSource::Node(name, span) => (name, span),
LintSource::Node(name, span, _) => (name, span),
_ => continue,
};
let mut diag_builder = struct_span_err!(self.sess,
Expand All @@ -384,15 +445,19 @@ impl<'a> LintLevelsBuilder<'a> {
forbidden_lint_name);
diag_builder.span_label(lint_attr_span, "overruled by previous forbid");
match forbid_src {
LintSource::Default => &mut diag_builder,
LintSource::Node(_, forbid_source_span) => {
LintSource::Default => {},
LintSource::Node(_, forbid_source_span, reason) => {
diag_builder.span_label(forbid_source_span,
"`forbid` level set here")
"`forbid` level set here");
if let Some(rationale) = reason {
diag_builder.note(&rationale.as_str());
}
},
LintSource::CommandLine(_) => {
diag_builder.note("`forbid` lint level was set on command line")
diag_builder.note("`forbid` lint level was set on command line");
}
}.emit();
}
diag_builder.emit();
// don't set a separate error for every lint in the group
break
}
Expand Down
9 changes: 6 additions & 3 deletions src/librustc/lint/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -470,15 +470,15 @@ pub enum LintSource {
Default,

/// Lint level was set by an attribute.
Node(ast::Name, Span),
Node(ast::Name, Span, Option<Symbol> /* RFC 2383 reason */),

/// Lint level was set by a command-line flag.
CommandLine(Symbol),
}

impl_stable_hash_for!(enum self::LintSource {
Default,
Node(name, span),
Node(name, span, reason),
CommandLine(text)
});

Expand Down Expand Up @@ -578,7 +578,10 @@ pub fn struct_lint_level<'a>(sess: &'a Session,
hyphen_case_flag_val));
}
}
LintSource::Node(lint_attr_name, src) => {
LintSource::Node(lint_attr_name, src, reason) => {
if let Some(rationale) = reason {
err.note(&rationale.as_str());
}
sess.diag_span_note_once(&mut err, DiagnosticMessageId::from(lint),
src, "lint level defined here");
if lint_attr_name.as_str() != name {
Expand Down
3 changes: 3 additions & 0 deletions src/libsyntax/feature_gate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -504,6 +504,9 @@ declare_features! (

// `extern crate foo as bar;` puts `bar` into extern prelude.
(active, extern_crate_item_prelude, "1.31.0", Some(54658), None),

// `reason = ` in lint attributes and `expect` lint attribute
(active, lint_reasons, "1.31.0", Some(54503), None),
);

declare_features! (
Expand Down
4 changes: 4 additions & 0 deletions src/test/ui/feature-gates/feature-gate-lint-reasons.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
#![warn(nonstandard_style, reason = "the standard should be respected")]
//~^ ERROR lint reasons are experimental

fn main() {}
11 changes: 11 additions & 0 deletions src/test/ui/feature-gates/feature-gate-lint-reasons.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
error[E0658]: lint reasons are experimental (see issue #54503)
--> $DIR/feature-gate-lint-reasons.rs:1:28
|
LL | #![warn(nonstandard_style, reason = "the standard should be respected")]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: add #![feature(lint_reasons)] to the crate attributes to enable

error: aborting due to previous error

For more information about this error, try `rustc --explain E0658`.
17 changes: 17 additions & 0 deletions src/test/ui/lint/empty-lint-attributes.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
#![feature(lint_reasons)]

// run-pass

// Empty (and reason-only) lint attributes are legal—although we may want to
// lint them in the future (Issue #55112).

#![allow()]
#![warn(reason = "observationalism")]

#[forbid()]
fn devoir() {}

#[deny(reason = "ultion")]
fn waldgrave() {}

fn main() {}
24 changes: 24 additions & 0 deletions src/test/ui/lint/reasons-erroneous.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
#![feature(lint_reasons)]

#![warn(absolute_paths_not_starting_with_crate, reason = 0)]
//~^ ERROR malformed lint attribute
//~| HELP reason must be a string literal
#![warn(anonymous_parameters, reason = b"consider these, for we have condemned them")]
//~^ ERROR malformed lint attribute
//~| HELP reason must be a string literal
#![warn(bare_trait_objects, reasons = "leaders to no sure land, guides their bearings lost")]
//~^ ERROR malformed lint attribute
#![warn(box_pointers, blerp = "or in league with robbers have reversed the signposts")]
//~^ ERROR malformed lint attribute
#![warn(elided_lifetimes_in_paths, reason("disrespectful to ancestors", "irresponsible to heirs"))]
//~^ ERROR malformed lint attribute
#![warn(ellipsis_inclusive_range_patterns, reason = "born barren", reason = "a freak growth")]
//~^ ERROR malformed lint attribute
//~| HELP reason in lint attribute must come last
#![warn(keyword_idents, reason = "root in rubble", macro_use_extern_crate)]
//~^ ERROR malformed lint attribute
//~| HELP reason in lint attribute must come last
#![warn(missing_copy_implementations, reason)]
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens with #[allow(reason = "foo")]?
I.e. no lints to allow, only the reason.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, let's make this an error (it would have been a no-op if you hadn't pointed this out).

Copy link
Member Author

Choose a reason for hiding this comment

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

... well, actually, I guess I could see a case that, if empty lint attributes (#[allow()] as discussed above) aren't an error (the stable behavior), then we should also allow reason-only lint attributes for consistency/uniformity.

(Presumably the only sane reason for empty lint attributes to exist is for the sake of the base case of some recursive macro, and if we care about that, then we should also care about a macro that's otherwise identical but also includes a reason.)

//~^ WARN unknown lint

fn main() {}
61 changes: 61 additions & 0 deletions src/test/ui/lint/reasons-erroneous.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
error[E0452]: malformed lint attribute
--> $DIR/reasons-erroneous.rs:3:58
|
LL | #![warn(absolute_paths_not_starting_with_crate, reason = 0)]
| ^
|
= help: reason must be a string literal

error[E0452]: malformed lint attribute
--> $DIR/reasons-erroneous.rs:6:40
|
LL | #![warn(anonymous_parameters, reason = b"consider these, for we have condemned them")]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: reason must be a string literal

error[E0452]: malformed lint attribute
--> $DIR/reasons-erroneous.rs:9:29
|
LL | #![warn(bare_trait_objects, reasons = "leaders to no sure land, guides their bearings lost")]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error[E0452]: malformed lint attribute
--> $DIR/reasons-erroneous.rs:11:23
|
LL | #![warn(box_pointers, blerp = "or in league with robbers have reversed the signposts")]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error[E0452]: malformed lint attribute
--> $DIR/reasons-erroneous.rs:13:36
|
LL | #![warn(elided_lifetimes_in_paths, reason("disrespectful to ancestors", "irresponsible to heirs"))]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error[E0452]: malformed lint attribute
--> $DIR/reasons-erroneous.rs:15:44
|
LL | #![warn(ellipsis_inclusive_range_patterns, reason = "born barren", reason = "a freak growth")]
| ^^^^^^^^^^^^^^^^^^^^^^
|
= help: reason in lint attribute must come last

error[E0452]: malformed lint attribute
--> $DIR/reasons-erroneous.rs:18:25
|
LL | #![warn(keyword_idents, reason = "root in rubble", macro_use_extern_crate)]
| ^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: reason in lint attribute must come last

warning: unknown lint: `reason`
--> $DIR/reasons-erroneous.rs:21:39
|
LL | #![warn(missing_copy_implementations, reason)]
| ^^^^^^
|
= note: #[warn(unknown_lints)] on by default

error: aborting due to 7 previous errors

For more information about this error, try `rustc --explain E0452`.
21 changes: 21 additions & 0 deletions src/test/ui/lint/reasons-forbidden.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
#![feature(lint_reasons)]

#![forbid(
unsafe_code,
//~^ NOTE `forbid` level set here
reason = "our errors & omissions insurance policy doesn't cover unsafe Rust"
)]

use std::ptr;

fn main() {
let a_billion_dollar_mistake = ptr::null();

#[allow(unsafe_code)]
//~^ ERROR allow(unsafe_code) overruled by outer forbid(unsafe_code)
//~| NOTE overruled by previous forbid
//~| NOTE our errors & omissions insurance policy doesn't cover unsafe Rust
unsafe {
*a_billion_dollar_mistake
}
}
14 changes: 14 additions & 0 deletions src/test/ui/lint/reasons-forbidden.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
error[E0453]: allow(unsafe_code) overruled by outer forbid(unsafe_code)
--> $DIR/reasons-forbidden.rs:14:13
|
LL | unsafe_code,
| ----------- `forbid` level set here
...
LL | #[allow(unsafe_code)]
| ^^^^^^^^^^^ overruled by previous forbid
|
= note: our errors & omissions insurance policy doesn't cover unsafe Rust

error: aborting due to previous error

For more information about this error, try `rustc --explain E0453`.
Loading