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

Changes from 1 commit
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
4 changes: 3 additions & 1 deletion src/librustc_lint/builtin.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -696,7 +696,9 @@ impl EarlyLintPass for BadRepr {
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" | "u*" | "i*" | "transparent" => {
| "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;
Expand Down