lint: Deny #[no_mangle] const items #21881

richo · 2015-02-03T03:36:03Z

This renames the PrivateNoMangleFns lint to allow both to happen in a
single pass, since they do roughly the same work.

Open questions:

[ ]: Do the tests actually pass (I'm running make check and running out the door now)
[ ]: Is the name of this lint ok. it seems to mostly be fine with convention
[ ]: I'm not super thrilled about the warning text

r? @kmcallister (Shamelessly nominating because you were looking at my other ticket)

richo · 2015-02-03T03:36:34Z

src/librustc/lint/builtin.rs

@@ -2083,6 +2090,12 @@ impl LintPass for PrivateNoMangleFns {
                    cx.span_lint(PRIVATE_NO_MANGLE_FNS, it.span, msg.as_slice());
                }
            },
+            ast::ItemConst(..) => {
+                if attr::contains_name(it.attrs.as_slice(), "no_mangle") {
+                    let msg = format!("const items cannot export symbols. {} therefore cannot be #[no_mangle",


And I guess this line needs to be broken up to appease tidy

This renames the PrivateNoMangleFns lint to allow both to happen in a single pass, since they do roughly the same work.

richo · 2015-02-03T07:12:15Z

I rethought this a bit. I don't think there's ever a reason to #[no_mangle] a const, and private #[no_mangle]'d statics should throw a warning as well.

I added the new lint, and tests for all.

It's possible to reuse some code between the Static and Fn check, but it seemed liek it would be way less readable so I let it be duplicated. I think this is now good to go out

richo · 2015-02-03T08:47:18Z

Aaaaand confirmed that the tests still pass. r? @kmcallister

kmcallister · 2015-02-03T17:49:14Z

src/librustc/lint/builtin.rs

+            },
+            ast::ItemConst(..) => {
+                if attr::contains_name(it.attrs.as_slice(), "no_mangle") {
+                    let msg = "const items should never be #[no_mangle]";


Can you add a few words about why?

Sure. How concerned should I be about overly long messages? (The current
ones all seem pretty terse).

I think this should probably have a hint that "maybe you wanted pub static"
too

On Tuesday, February 3, 2015, Keegan McAllister notifications@github.com
wrote:

In src/librustc/lint/builtin.rs
#21881 (comment):

@@ -2083,6 +2097,20 @@ impl LintPass for PrivateNoMangleFns {
cx.span_lint(PRIVATE_NO_MANGLE_FNS, it.span, msg.as_slice());
}
},

ast::ItemStatic(..) => {

if attr::contains_name(it.attrs.as_slice(), "no_mangle") &&

!cx.exported_items.contains(&it.id) {

let msg = format!("static {} is marked #[no_mangle], but not exported",

it.ident);

cx.span_lint(PRIVATE_NO_MANGLE_STATICS, it.span, msg.as_slice());

}

},

ast::ItemConst(..) => {

if attr::contains_name(it.attrs.as_slice(), "no_mangle") {

let msg = "const items should never be #[no_mangle]";

Can you add a few words about why?

—
Reply to this email directly or view it on GitHub
https://github.com/rust-lang/rust/pull/21881/files#r24023185.

richo · 2015-02-03T20:10:57Z

I added another commit with a comment and a help message about the thing a user probably wanted

kmcallister · 2015-02-03T20:56:15Z

src/librustc/lint/builtin.rs

+                    // don't have anything to attach a symbol to
+                    let msg = "const items should never be #[no_mangle]";
+                    cx.span_lint(NO_MANGLE_CONST_ITEMS, it.span, msg);
+                    cx.sess().span_help(it.span, "maybe you wanted this to be a pub static");


Unfortunately the help message will print even with -A no-mangle-const-items, see #19668

I'm not sure that's a huge issue in this case. I'm not aware of any time
you would actually want this.

On Tuesday, February 3, 2015, Keegan McAllister notifications@github.com
wrote:

In src/librustc/lint/builtin.rs
#21881 (comment):

@@ -2083,6 +2097,23 @@ impl LintPass for PrivateNoMangleFns {
cx.span_lint(PRIVATE_NO_MANGLE_FNS, it.span, msg.as_slice());
}
},

ast::ItemStatic(..) => {

if attr::contains_name(it.attrs.as_slice(), "no_mangle") &&

!cx.exported_items.contains(&it.id) {

let msg = format!("static {} is marked #[no_mangle], but not exported",

it.ident);

cx.span_lint(PRIVATE_NO_MANGLE_STATICS, it.span, msg.as_slice());

}

},

ast::ItemConst(..) => {

if attr::contains_name(it.attrs.as_slice(), "no_mangle") {

// Const items do not refer to a particular location in memory, and therefore

// don't have anything to attach a symbol to

let msg = "const items should never be #[no_mangle]";

cx.span_lint(NO_MANGLE_CONST_ITEMS, it.span, msg);

cx.sess().span_help(it.span, "maybe you wanted this to be a pub static");

Unfortunately the help message will print even with -A
no-mangle-const-items, see #19668
#19668

—
Reply to this email directly or view it on GitHub
https://github.com/rust-lang/rust/pull/21881/files#r24038887.

Still, the lint should behave consistently.

richo · 2015-02-10T20:35:44Z

Turned the warning + hint into one longer warning.

kmcallister · 2015-02-10T22:46:18Z

@bors r+ b6f55ef rollup

bors · 2015-02-11T05:24:04Z

⌛ Testing commit b6f55ef with merge 289ae60...

@kmcallister

This renames the PrivateNoMangleFns lint to allow both to happen in a single pass, since they do roughly the same work. Closes #21856 Open questions: [ ]: Do the tests actually pass (I'm running make check and running out the door now) [ ]: Is the name of this lint ok. it seems to mostly be fine with [convention](https://github.com/rust-lang/rfcs/blob/cc53afbe5dea41e1f7d1c3dce71e013abe025211/text/0344-conventions-galore.md#lints) [ ]: I'm not super thrilled about the warning text r? @kmcallister (Shamelessly nominating because you were looking at my other ticket)

bors · 2015-02-11T06:45:24Z

💔 Test failed - auto-mac-32-opt

alexcrichton · 2015-02-11T06:51:58Z

@bors: retry

@kmcallister

…lister This renames the PrivateNoMangleFns lint to allow both to happen in a single pass, since they do roughly the same work. Closes rust-lang#21856 Open questions: [ ]: Do the tests actually pass (I'm running make check and running out the door now) [ ]: Is the name of this lint ok. it seems to mostly be fine with [convention](https://github.com/rust-lang/rfcs/blob/cc53afbe5dea41e1f7d1c3dce71e013abe025211/text/0344-conventions-galore.md#lints) [ ]: I'm not super thrilled about the warning text r? @kmcallister (Shamelessly nominating because you were looking at my other ticket)

@kmcallister

This renames the PrivateNoMangleFns lint to allow both to happen in a single pass, since they do roughly the same work. Closes rust-lang#21856 Open questions: [ ]: Do the tests actually pass (I'm running make check and running out the door now) [ ]: Is the name of this lint ok. it seems to mostly be fine with [convention](https://github.com/rust-lang/rfcs/blob/cc53afbe5dea41e1f7d1c3dce71e013abe025211/text/0344-conventions-galore.md#lints) [ ]: I'm not super thrilled about the warning text r? @kmcallister (Shamelessly nominating because you were looking at my other ticket)

rust-highfive assigned kmcallister Feb 3, 2015

richo reviewed Feb 3, 2015
View reviewed changes

richo force-pushed the lint-no-mangle-const branch 2 times, most recently from 7e009f9 to 1f64d7a Compare February 3, 2015 06:06

richo added 2 commits February 2, 2015 23:11

lint: Deny #[no_mangle] const items

51ed1ec

This renames the PrivateNoMangleFns lint to allow both to happen in a single pass, since they do roughly the same work.

lint: Warn about no-mangled statics that are not exported

73d5d89

richo force-pushed the lint-no-mangle-const branch from 1f64d7a to 73d5d89 Compare February 3, 2015 07:11

kmcallister reviewed Feb 3, 2015
View reviewed changes

lint: Document the why and how of the no_mangle const lint

b6f55ef

richo force-pushed the lint-no-mangle-const branch from a5cff01 to b6f55ef Compare February 10, 2015 20:09

richo mentioned this pull request Feb 11, 2015

Rollup of 7 pull requests #22170

Closed

bors merged commit b6f55ef into rust-lang:master Feb 12, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lint: Deny #[no_mangle] const items #21881

lint: Deny #[no_mangle] const items #21881

richo commented Feb 3, 2015

richo Feb 3, 2015

richo commented Feb 3, 2015

richo commented Feb 3, 2015

kmcallister Feb 3, 2015

richo Feb 3, 2015

richo commented Feb 3, 2015

kmcallister Feb 3, 2015

richo Feb 3, 2015

kmcallister Feb 3, 2015

richo commented Feb 10, 2015

kmcallister commented Feb 10, 2015

bors commented Feb 11, 2015

bors commented Feb 11, 2015

alexcrichton commented Feb 11, 2015

lint: Deny #[no_mangle] const items #21881

lint: Deny #[no_mangle] const items #21881

Conversation

richo commented Feb 3, 2015

richo Feb 3, 2015

Choose a reason for hiding this comment

richo commented Feb 3, 2015

richo commented Feb 3, 2015

kmcallister Feb 3, 2015

Choose a reason for hiding this comment

richo Feb 3, 2015

Choose a reason for hiding this comment

richo commented Feb 3, 2015

kmcallister Feb 3, 2015

Choose a reason for hiding this comment

richo Feb 3, 2015

Choose a reason for hiding this comment

kmcallister Feb 3, 2015

Choose a reason for hiding this comment

richo commented Feb 10, 2015

kmcallister commented Feb 10, 2015

bors commented Feb 11, 2015

bors commented Feb 11, 2015

alexcrichton commented Feb 11, 2015