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.

[pnkfelix]

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

Having said that, I think I might have a plausible branch to do this.

I am still not sure if the machinery ends up being worth it, but I will at least try to put up a Pull Request illustrating the idea.

[pnkfelix]

hmm. Did this manage to leak all the way to stable?

[pnkfelix]

Yes, apparently this change in behavior was actually part of 1.33:

% cargo clean && cargo +1.32.0 build
   Compiling bar v0.1.0 (/home/pnkfelix/Dev/Mozilla/issue58502/bar)
   Compiling client v0.1.0 (/home/pnkfelix/Dev/Mozilla/issue58502/client)
    Finished dev [unoptimized + debuginfo] target(s) in 0.26s
% cargo clean && cargo +1.33.0 build
   Compiling bar v0.1.0 (/home/pnkfelix/Dev/Mozilla/issue58502/bar)
   Compiling client v0.1.0 (/home/pnkfelix/Dev/Mozilla/issue58502/client)
warning: function `AnotherNonSnakeCaseName` should have a snake case name
 --> src/main.rs:6:32
  |
6 | 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

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

I'm going to pause working on my supposed bug fix, and instead just try to focus on discussing the question of how we should handle issues like this (as a matter of policy) at the next compiler meeting.

(I am somewhat disappointed that the change made it into stable without my realizing it. I was relying too much on the existing tagging, even though I knew that the regression-to-nightly had to be out of date.)

[pnkfelix]

As a kind of post-mortem, here is a timeline of events:

• January 14th: PR Use structured suggestions for nonstandard style lints #57387 lands on nightly
• January 17th: nightly is promoted to beta (via Prepare beta 1.33.0 #57670)
• February 15th: this issue (fn generated by macro exported from crate loses global #![allow(non_snake_case)] #58502) is reported, and points out the likely cause.
• Feburary 16th: this issue is tagged as a stable-to-nightly regression (but I believe it was at point a stable-to-beta regression).
  • Since it required using a macro defined in an external crate, it was not trivial to check (e.g. via play), and thus I am not surprised about the mistaken tag
• also on February 16th: @euclio pointed out that the problem is a natural consequence of tightening of spans within the compiler: namely, span-tightening can cause the a lint to no longer be caught by the existing external-crate filtering, and thus trigger a "new" warning (or error, in case of #[deny(..)])
• February 21st: issue was tagged as P-high but not assigned to anyone.
• Feburary 25th: issue leaked into 1.33 release
• Feburary 28th: I assigned issue to myself but didn't do anything immediately.

So lets see, process failures:

• I failed to double-check whether the regression stable-to-nightly tag was accurate, which should probably be first priority when deciding how to prioritize work on a bug.
• I didn't assign the P-high issue to anyone even after giving it that tag. But I don't think we're going to able to avoid instances of this in the future.

[pnkfelix]

nominating for discussion.

(I'm inclined to close this as wont-fix at this point, but it needs to be lifted to group discussion, at least among T-compiler, and potentially among T-lang.)

[petrochenkov]

Looks like a bug fix to me.
The non_snake_case is about identifiers, so it should use the identifier span to determine whether it's "user provided" and the lint can be acted upon, or "from an external macro" and the lint is not actionable.

Lint improvements happen from time to time for different reasons and we don't normally consider them as regressions AFAIK.

[pnkfelix]

I essentially agree (and indeed, said so above) that PR #57387 seems like a bugfix

But just to play devil's advocate: The main counter-argument that I can imagine is that the lint being applied here (non_snake_case) is only applied to identifiers that are used for certain kinds of definitions, and the particular kind of defintion in the scenario here arguably comes from the macro implementation, not the identifier itself.

(In other words, the problem that the lint is observing is arguably due to interaction between the macro body and the input identifier given to the macro.)

This counter-argument is not really meant as a basis for any real corrective action on this particular issue.

What my counter-argument is meant to be: It is a potential justification for trying, in some future API, to differentiate between the span(s) used in a diagnostic report vs the span(s) used for allow/warn/deny determination (which is an idea that @nikomatsakis quickly sketched (1, 2) during yesterday's meeting).

[pnkfelix]

Discussed at T-compiler meeting. Closing as wont-fix not-a-bug, but noting for the future that changing the spans of lints can have unexpected consequences over time.