Warn on `repr` without hints #51401

estebank · 2018-06-06T17:03:13Z

Fix #51376.

rust-highfive · 2018-06-06T17:03:16Z

r? @cramertj

(rust_highfive has picked a reviewer for you, use r? to override)

cramertj · 2018-06-06T17:16:10Z

r=me with travis passing.

oli-obk · 2018-06-06T18:54:18Z

src/librustc/hir/check_attr.rs

+                    has_hints = !list.is_empty();
+                }
+                if !has_hints {
+                    span_warn!(


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

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?

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.

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.

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.

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

hanna-kruppe · 2018-06-06T18:56:55Z

src/librustc/hir/check_attr.rs

+                if let Some(ref list) = list {
+                    has_hints = !list.is_empty();
+                }
+                if !has_hints {


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

I caught that while reworking this. Changed it.

oli-obk · 2018-06-06T21:20:10Z

src/librustc_lint/builtin.rs

+                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" => {


You still need to expand these int placeholders

Oh, by repr(u*) it means repr(u8), for example, is allowed? We should update the rustonomicon's description, it wasn't clear to me :-/

Yea. This could be better documented

hanna-kruppe · 2018-06-06T21:22:42Z

src/test/ui/suggestions/repr.stderr

+   |
+   = note: #[warn(bad_repr)] on by default
+   = help: valid hints include `#[repr(C)]`, `#[repr(packed)]`, `#[repr(rust)]`, and `#[repr(transparent)]`
+   = note: for more information, visit <https://doc.rust-lang.org/nomicon/other-reprs.html>


I recommend https://doc.rust-lang.org/reference/type-layout.html instead, it's more authorative and up to date

hanna-kruppe · 2018-06-06T21:23:08Z

src/librustc_lint/builtin.rs

+                    let sp = match format!("{}", lit).as_ref() {
+                        "C" | "packed" | "rust" | "u*" | "i*" | "transparent" => {
+                            let lo = attr.span.lo() + BytePos(2);
+                            let hi = attr.span.hi() - BytePos(1);


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?

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.

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)`

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.

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.

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)

oli-obk · 2018-06-06T21:27:11Z

src/librustc_lint/builtin.rs

@@ -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"


usize, isize, too

hanna-kruppe · 2018-06-06T22:28:14Z

src/librustc_lint/builtin.rs

+                        warn.span_suggestion(
+                            sp,
+                            "give `repr` a hint",
+                            format!("repr({})", lit),


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

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

hanna-kruppe · 2018-06-06T23:02:20Z

src/librustc_lint/builtin.rs

@@ -687,6 +687,18 @@ impl EarlyLintPass for BadRepr {
    fn check_attribute(&mut self, cx: &EarlyContext, attr: &ast::Attribute) {
        if attr.name() == "repr" {
            let list = attr.meta_item_list();
+            let outer = match attr.style {
+                ast::AttrStyle::Outer => true,
+                ast::AttrStyle::Inner => false,


Since we all seem to be in agreement this isn't actually allowed, I suggest an assert_eq! and simplifying the code accordingly.

estebank · 2018-06-07T16:15:32Z

@bors r=cramertj

bors · 2018-06-07T16:15:33Z

📌 Commit 0e3f19d has been approved by cramertj

Warn on `repr` without hints Fix rust-lang#51376.

Rollup of 13 pull requests Successful merges: - #50143 (Add deprecation lint for duplicated `macro_export`s) - #51099 (Fix Issue 38777) - #51276 (Dedup auto traits in trait objects.) - #51298 (Stabilize unit tests with non-`()` return type) - #51360 (Suggest parentheses when a struct literal needs them) - #51391 (Use spans pointing at the inside of a rustdoc attribute) - #51394 (Use scope tree depths to speed up `nearest_common_ancestor`.) - #51396 (Make the size of Option<NonZero*> a documented guarantee.) - #51401 (Warn on `repr` without hints) - #51412 (Avoid useless Vec clones in pending_obligations().) - #51427 (compiletest: autoremove duplicate .nll.* files (#51204)) - #51436 (Do not require stage 2 compiler for rustdoc) - #51437 (rustbuild: generate full list of dependencies for metadata) Failed merges:

Warn on repr without hints

36381fa

rust-highfive assigned cramertj Jun 6, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 6, 2018

This comment has been minimized.

Sign in to view

oli-obk reviewed Jun 6, 2018

View reviewed changes

hanna-kruppe reviewed Jun 6, 2018

View reviewed changes

Expand output and suggestions, fix tests

2c7099b

This comment has been minimized.

Sign in to view

estebank changed the title ~~Warn on repr without hints [wip]~~ Warn on repr without hints Jun 6, 2018

This comment has been minimized.

Sign in to view

Turn warning into lint

3580de8

This comment has been minimized.

Sign in to view

oli-obk reviewed Jun 6, 2018

View reviewed changes

hanna-kruppe suggested changes Jun 6, 2018

View reviewed changes

estebank added 2 commits June 6, 2018 14:25

Add transparent as valid repr hint

48e45ee

Expand (fix) u* and i* repr hints

451eb66

estebank force-pushed the warn-repr branch from 0f8ac96 to 451eb66 Compare June 6, 2018 21:26

oli-obk reviewed Jun 6, 2018

View reviewed changes

estebank added 2 commits June 6, 2018 14:33

Change repr documentation link

9a80c2b

Add i/u size

b3810f6

hanna-kruppe reviewed Jun 6, 2018

View reviewed changes

This comment has been minimized.

Sign in to view

estebank force-pushed the warn-repr branch from b741cbc to 770c1f9 Compare June 6, 2018 22:49

hanna-kruppe reviewed Jun 6, 2018

View reviewed changes

This comment has been minimized.

Sign in to view

estebank added 2 commits June 6, 2018 17:36

Use consistent span for repr attr suggestion

3cc09c8

Do not account for inner/outer attr

0e3f19d

estebank force-pushed the warn-repr branch from 770c1f9 to 0e3f19d Compare June 7, 2018 00:40

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 7, 2018

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Jun 8, 2018

Rollup merge of rust-lang#51401 - estebank:warn-repr, r=cramertj

898bb78

Warn on `repr` without hints Fix rust-lang#51376.

Mark-Simulacrum mentioned this pull request Jun 8, 2018

Rollup of 13 pull requests #51448

Merged

bors merged commit 0e3f19d into rust-lang:master Jun 9, 2018

estebank deleted the warn-repr branch November 9, 2023 05:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Warn on `repr` without hints #51401

Warn on `repr` without hints #51401

estebank commented Jun 6, 2018

rust-highfive commented Jun 6, 2018

cramertj commented Jun 6, 2018

This comment has been minimized.

oli-obk Jun 6, 2018

estebank Jun 6, 2018

hanna-kruppe Jun 6, 2018

estebank Jun 6, 2018

hanna-kruppe Jun 6, 2018

estebank Jun 6, 2018

hanna-kruppe Jun 6, 2018

estebank Jun 6, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

oli-obk Jun 6, 2018

estebank Jun 6, 2018 •

edited

oli-obk Jun 6, 2018

hanna-kruppe Jun 6, 2018

estebank Jun 6, 2018

hanna-kruppe Jun 6, 2018

oli-obk Jun 6, 2018

estebank Jun 6, 2018

hanna-kruppe Jun 6, 2018

estebank Jun 6, 2018

hanna-kruppe Jun 6, 2018

oli-obk Jun 6, 2018

estebank Jun 6, 2018

hanna-kruppe Jun 6, 2018

estebank Jun 6, 2018

This comment has been minimized.

hanna-kruppe Jun 6, 2018

This comment has been minimized.

estebank commented Jun 7, 2018

bors commented Jun 7, 2018

Warn on repr without hints #51401

Warn on repr without hints #51401

Conversation

estebank commented Jun 6, 2018

rust-highfive commented Jun 6, 2018

cramertj commented Jun 6, 2018

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

estebank Jun 6, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

Choose a reason for hiding this comment

This comment has been minimized.

estebank commented Jun 7, 2018

bors commented Jun 7, 2018

Warn on `repr` without hints #51401

Warn on `repr` without hints #51401

estebank Jun 6, 2018 •

edited