Skip to content

Warn on repr without hints #51401

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

Merged
merged 9 commits into from
Jun 9, 2018
Merged

Warn on repr without hints #51401

Show file tree
Hide file tree
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
1 change: 1 addition & 0 deletions src/librustc/diagnostics.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2139,6 +2139,7 @@ register_diagnostics! {
E0657, // `impl Trait` can only capture lifetimes bound at the fn level
E0687, // in-band lifetimes cannot be used in `fn`/`Fn` syntax
E0688, // in-band lifetimes cannot be mixed with explicit lifetime binders
E0689, // `#[repr]` must have a hint

E0906, // closures cannot be static
}
17 changes: 16 additions & 1 deletion src/librustc/hir/check_attr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,22 @@ impl<'a, 'tcx> CheckAttrVisitor<'a, 'tcx> {
let hints: Vec<_> = item.attrs
.iter()
.filter(|attr| attr.name() == "repr")
.filter_map(|attr| attr.meta_item_list())
.filter_map(|attr| {
let list = attr.meta_item_list();
let mut has_hints = false;
if let Some(ref list) = list {
has_hints = !list.is_empty();
}
if !has_hints {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I think this is better written as let is_empty = list.map(|list| list.is_empty()).unwrap_or(true); and then if empty { ... }.

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 caught that while reworking this. Changed it.

span_warn!(
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 be an unsilencable warning instead of a lint, is that intended? So far we reserved that for future incompatibility warnings.

A test for this would be good, 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.

I'll be adding a test. I feel that an empty repr (or it's cousin, #[repr = "C"]) should be hard failures in the future. What do you think?

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 all the overhead of carefully managing the transition to a hard error, and the eventual breakage of (hypothetical) old crates, is worth it for this. This is just supposed to catch a silly programmer mistake, it's not a soundness issue or anything. IMO this should be a lint, warn-by-default for now and maybe later upgraded to deny-by-default.

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 feel strongly that having an incorrect repl (specially #[repr = "C"]) is hiding a pretty severe bug. Having said that, I can see how it should be a lint instead. I'll push my latest changes and rework this into a lint at a later time.

Copy link
Contributor

Choose a reason for hiding this comment

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

It does indicate a severe bug, and it should report an error, but making it a hard error rather than a deny-by-default-lint error just unnecessarily breaks stability promises and unmaintained crates without helping users any more.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will turn into a lint once I free myself to work on this a bit more. Please verify that the current output is reasonable.

self.tcx.sess,
item.span,
E0689,
"`repr` attribute cannot be empty",
);
}
list
})
.flat_map(|hints| hints)
.collect();

Expand Down