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

Warn on repr without hints #51401

Merged
merged 9 commits into from
Jun 9, 2018
Merged

Warn on repr without hints #51401

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
36381fa
Warn on `repr` without hints
estebank Jun 6, 2018
2c7099b
Expand output and suggestions, fix tests
estebank Jun 6, 2018
3580de8
Turn warning into lint
estebank Jun 6, 2018
48e45ee
Add `transparent` as valid `repr` hint
estebank Jun 6, 2018
451eb66
Expand (fix) u* and i* `repr` hints
estebank Jun 6, 2018
9a80c2b
Change repr documentation link
estebank Jun 6, 2018
b3810f6
Add i/u size
estebank Jun 6, 2018
3cc09c8
Use consistent span for repr attr suggestion
estebank Jun 6, 2018
0e3f19d
Do not account for inner/outer attr
estebank Jun 7, 2018
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
6 changes: 6 additions & 0 deletions src/librustc/lint/builtin.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,12 @@ declare_lint! {
"potentially-conflicting impls were erroneously allowed"
}

declare_lint! {
pub BAD_REPR,
Warn,
"detects incorrect use of `repr` attribute"
}

declare_lint! {
pub DEPRECATED,
Warn,
Expand Down
71 changes: 71 additions & 0 deletions src/librustc_lint/builtin.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -673,6 +673,77 @@ impl EarlyLintPass for AnonymousParameters {
}
}

/// Checks for incorrect use use of `repr` attributes.
#[derive(Clone)]
pub struct BadRepr;

impl LintPass for BadRepr {
fn get_lints(&self) -> LintArray {
lint_array!()
}
}

impl EarlyLintPass for BadRepr {
fn check_attribute(&mut self, cx: &EarlyContext, attr: &ast::Attribute) {
if attr.name() == "repr" {
let list = attr.meta_item_list();

// Emit warnings with `repr` either has a literal assignment (`#[repr = "C"]`) or
// no hints (``#[repr]`)
let has_hints = list.as_ref().map(|ref list| !list.is_empty()).unwrap_or(false);
if !has_hints {
let mut suggested = false;
let mut warn = if let Some(ref lit) = attr.value_str() {
// avoid warning about empty `repr` on `#[repr = "foo"]`
let sp = match format!("{}", lit).as_ref() {
| "C" | "packed" | "rust" | "transparent"
| "u8" | "u16" | "u32" | "u64" | "u128"
Copy link
Contributor

Choose a reason for hiding this comment

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

usize, isize, too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

| "i8" | "i16" | "i32" | "i64" | "i128" => {
let lo = attr.span.lo() + BytePos(2);
let hi = attr.span.hi() - BytePos(1);
Copy link
Contributor

Choose a reason for hiding this comment

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

So, this skips the leading #[ and trailing ], but only for some cases (e.g., repr = "C" but not repr = "B"). What's the rationale for that extra work and inconsistency? Why not always use attr.span?

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that such span operations break down in the presence of macros. We've had some pain with those in clippy. It's usually better to have less prettier ^^^^^^^^^ in the non-macro case than to throw up a bunch of macro internals at the user.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's to be 100% confident that the suggested code will be correct like in the following case:

warning: `repr` attribute isn't configurable with a literal
 --> file.rs:1:3
  |
1 | #[   repr   =    "C"   ]
  |   ^^^^^^^^^^^^^^^^^^^^^ help: give `repr` a hint: `repr(C)`

In the cases where we're not suggesting anything attr.span is good enough. If we use attr.span for the case above, the output would be:

warning: `repr` attribute isn't configurable with a literal
 --> file.rs:1:3
  |
1 | #[   repr   =    "C"   ]
  |   ^^-------------------^
  |     |
  |     help: give `repr` a hint: `repr(C)`

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really follow. I guess it might explain why these calculations are only done in one branch, but why are they done at all? Why try to exclude the #[ and ] from the labels in the first place? It doesn't significantly change how the diagnostic looks and I haven't seen other attribute-related diagnostics do it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I though that repr attrs could be applied to the enclosing element, like other attributes #![repr(C)], but I just tried it and it was rejected. Is this the case everywhere? In that case I don't have to worry about the possible difference between #![repr] and #[repr] and wouldn't have to do this span wrangling.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think #![repr] is valid, but I also don't get why that would make any difference for the span. The span points at the attribute either way, doesn't it? (Actually, I am now even more confused: if #![repr] was valid, lo + 2 would be off by one for it)

suggested = true;
attr.span.with_lo(lo).with_hi(hi)
}
_ => attr.span, // the literal wasn't a valid `repr` arg
};
let mut warn = cx.struct_span_lint(
BAD_REPR,
sp,
"`repr` attribute isn't configurable with a literal",
);
if suggested {
// if the literal could have been a valid `repr` arg,
// suggest the correct syntax
warn.span_suggestion(
sp,
"give `repr` a hint",
format!("repr({})", lit),
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I guess as written this suggestion needs the smalelr span to avoid IDEs dropping the #[ and ]?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe that the last changes would be agreeable to you (IDEs should not be mucking around with escaping of suggestion text).

);
} else {
warn.span_label(attr.span, "needs a hint");
}
warn
} else {
let mut warn = cx.struct_span_lint(
BAD_REPR,
attr.span,
"`repr` attribute must have a hint",
);
warn.span_label(attr.span, "needs a hint");
warn
};
if !suggested {
warn.help("valid hints include `#[repr(C)]`, `#[repr(packed)]`, \
`#[repr(rust)]` and `#[repr(transparent)]`");
warn.note("for more information, visit \
<https://doc.rust-lang.org/nomicon/other-reprs.html>");
}
warn.emit();
}
}
}
}

/// Checks for use of attributes which have been deprecated.
#[derive(Clone)]
pub struct DeprecatedAttr {
Expand Down
1 change: 1 addition & 0 deletions src/librustc_lint/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ pub fn register_builtins(store: &mut lint::LintStore, sess: Option<&Session>) {
UnusedImportBraces,
AnonymousParameters,
UnusedDocComment,
BadRepr,
);

add_early_builtin_with_new!(sess,
Expand Down
9 changes: 8 additions & 1 deletion src/test/ui/feature-gate/issue-43106-gating-of-builtin-attrs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,9 @@
#![start = "x4300"] //~ WARN unused attribute
// see issue-43106-gating-of-test.rs for crate-level; but non crate-level is below at "4200"
// see issue-43106-gating-of-bench.rs for crate-level; but non crate-level is below at "4100"
#![repr = "3900"] //~ WARN unused attribute
#![repr = "3900"]
//~^ WARN unused attribute
//~| WARN `repr` attribute isn't configurable with a literal
#![path = "3800"] //~ WARN unused attribute
#![abi = "3700"] //~ WARN unused attribute
#![automatically_derived = "3600"] //~ WARN unused attribute
Expand Down Expand Up @@ -309,20 +311,25 @@ mod bench {

#[repr = "3900"]
//~^ WARN unused attribute
//~| WARN `repr` attribute isn't configurable with a literal
mod repr {
mod inner { #![repr="3900"] }
//~^ WARN unused attribute
//~| WARN `repr` attribute isn't configurable with a literal

#[repr = "3900"] fn f() { }
//~^ WARN unused attribute
//~| WARN `repr` attribute isn't configurable with a literal

struct S;

#[repr = "3900"] type T = S;
//~^ WARN unused attribute
//~| WARN `repr` attribute isn't configurable with a literal

#[repr = "3900"] impl S { }
//~^ WARN unused attribute
//~| WARN `repr` attribute isn't configurable with a literal
}

#[path = "3800"]
Expand Down
Loading