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.

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
Changes from 1 commit
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
1 change: 1 addition & 0 deletions src/librustc/diagnostics.rs
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
@@ -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();