fn generated by macro exported from crate loses global #![allow(non_snake_case)]

[mykmelez]

Given a bar library crate with a global #![allow(non_snake_case)] and a macro that generates functions with a given name, with or without a local #[allow(non_snake_case)]:

#![allow(non_snake_case)]

#[macro_export]
macro_rules! snakes_on_a_case {
    (global_allow $name:ident) => {
        pub fn $name() {}
    };
    (local_allow $name:ident) => {
        #[allow(non_snake_case)]
        pub fn $name() {}
    };
}

If that crate is used by another crate to generate non-snake-case-named functions:

use bar::*;

snakes_on_a_case!(local_allow ANonSnakeCaseName);
snakes_on_a_case!(global_allow AnotherNonSnakeCaseName);

fn main() {
    ANonSnakeCaseName();
    AnotherNonSnakeCaseName();
}

Then nightly rustc since 2019-01-15 (rustc 1.33.0-nightly (03acbd71c 2019-01-14)) will warn that functions generated by the macro without the local #[allow(non_snake_case)] should have a snake case name:

warning: function `AnotherNonSnakeCaseName` should have a snake case name
 --> src/main.rs:4:32
  |
4 | snakes_on_a_case!(global_allow AnotherNonSnakeCaseName);
  |                                ^^^^^^^^^^^^^^^^^^^^^^^ help: convert the identifier to snake case: `another_non_snake_case_name`
  |
  = note: #[warn(non_snake_case)] on by default

Whereas nightly rustc on 2019-01-14 (rustc 1.33.0-nightly (2fadb0a16 2019-01-13)) doesn't emit this warning.

Here's the comparison between the two nightly builds. To my untrained eye, @euclio's 7c0d145 (improve non_snake_case diagnostics) and @bors's 1d029c6 (Auto merge of #57387 - euclio:nonstandard-style-suggestions) look potentially related.

Note: I can't reproduce if the macro is in a module within the same crate that uses it, only if it's exported from a different crate (thus complicating the reduction of the test case to a single code snippet).

[euclio]

This is a consequence of how the lint system interacts with macros, and isn't limited to the non_snake_case lint. Lints with spans that are part of the expansion in the source code will fire, but lints with spans that include expanded tokens will not be fired. Since non_snake_case's span only applies to the identifier, the lints will fire if the expansion results in a node that would trigger the lint (in your case, a function definition). Since the lint node is considered part of the binary crate instead of the library crate, the global #![allow(non_snake_case)] in the library crate doesn't apply. You can, however, add the allow to the macro body, which would silence the lint.

Note that dead_code exhibits this property as well:

#![allow(dead_code)]

#[macro_export]
macro_rules! wrapper {
    ($($tokens:tt)*) => { $( $tokens )* }
}

#[macro_export]
macro_rules! struct_wrapper {
    ($($tokens:tt)*) => { struct $($tokens)* {} }
}

#[macro_use]
extern crate mac;

wrapper!(struct Foo {});

struct_wrapper!(Bar);

rustc will only lint Foo as dead code:

$ cargo build
   Compiling mac v0.1.0 (/home/euclio/scratch/hello/mac)
   Compiling hello v0.1.0 (/home/euclio/scratch/hello)
warning: struct is never constructed: `Foo`
 --> src/main.rs:4:10
  |
4 | wrapper!(struct Foo {});
  |          ^^^^^^^^^^
  |
  = note: #[warn(dead_code)] on by default

    Finished dev [unoptimized + debuginfo] target(s) in 0.43s

[pnkfelix]

triage P-high

[nikomatsakis]

Did the span being used for the lint message change, perhaps as part of @euclio's PR?

[euclio]

Yes, the span was tightened to include just the identifier.

[pnkfelix]

triage. assigning to self to figure out as part of name-resolution/macro-expansion spec work.

[pnkfelix]

So, my immediate reaction here is that this strikes me as a bug fix, not a misfeature. (I'll address whether we should still treat it as a regression in a moment.)

In particular, I would expect the use of #![allow(non_snake_case)] in the macro-provider crate bar to only apply its lint-loosening effect to identifiers that occur textually within the crate bar itself.

I think that expectation jibes with @euclio's comment above.

---

Still, it is a change in behavior, and it might break crates that deny lints. E.g. if the binary crate in the example used #![deny(non_snake_case)], they would compile successfully on nightly-2019-01-14 but error on nightly-2019-01-15.

We should at least investigate whether it is feasible to deploy a warning cycle in a case like this. That is, I would like to know if it is possible to spend one or more release cycles downgrading from error to warning when this scenario arises. (I don't know if the associated future-compat warning diagnositc would fire solely for deny/forbid lints; it would probably make sense, but it could also just be noise when one is only seeing a warning in the first place.)

[pnkfelix]

Skimming over 7c0d145, I am not sure it is worth the effort to try to deploy a warning cycle here.

As I understand it, the change in behavior here was solely due to the narrowing of the span for this particular lint so that it focuses solely on the identifier, and not the whole item being defined.

Trying to deploy a warning cycle would probably involve threading the original span into check_snake_case again (an easy change), and then somehow making the structured-lint machinery consume both the ident.span and the original span, and have it downgrade the lint from an error to a warning when the original span would have caused no error even though the ident.span causes one to be signalled (a much harder change).

The only way I could justify putting effort into adding such functionality to the structured-lint machinery would be if we planned to deploy similar future compatibility warning-cycles when we implement future changes to lint spans.