False positives for the new non_local_definitions lint

[weiznich]

Code

I tried this code: https://github.com/diesel-rs/diesel/blob/33ddddcd44d64368c4e8edb630e3f281e4a1a77d/diesel/src/sqlite/connection/mod.rs#L931

This is reproducible with cargo +nightly check -p diesel -F sqlite --all-targets

I expected to see this happen: No warning or at least no warning that tells me that this is an issue with the derive itself. (It's just that the derive is used in a function for scoping reasons).

Instead, this happened: I see the following warning:

warning: non-local `impl` definition, they should be avoided as they go against expectation
   --> diesel/src/sqlite/connection/mod.rs:928:25
    |
928 |         #[derive(Debug, crate::expression::AsExpression)]
    |                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = help: move this `impl` block outside the of the current constant `_` and up 2 bodies
    = note: an `impl` definition is non-local if it is nested inside an item and neither the type nor the trait are at the same nesting level as the `impl` block
    = note: one exception to the rule are anon-const (`const _: () = { ... }`) at top-level module and anon-const at the same nesting as the trait or type
    = note: this lint may become deny-by-default in the edition 2024 and higher, see the tracking issue <https://github.com/rust-lang/rust/issues/120363>
    = note: the derive macro `crate::expression::AsExpression` may come from an old version of the `diesel_derives` crate, try updating your dependency with `cargo update -p diesel_derives`
    = note: `#[warn(non_local_definitions)]` on by default
    = note: this warning originates in the derive macro `crate::expression::AsExpression` (in Nightly builds, run with -Z macro-backtrace for more info)

Version it worked on

It most recently worked on: 1.77.0-beta.5

Version with regression

rustc --version --verbose:

rustc 1.78.0-nightly (fc3800f65 2024-02-26)
binary: rustc
commit-hash: fc3800f65777a365b5125706d60f97e4d0675efe
commit-date: 2024-02-26
host: x86_64-unknown-linux-gnu
release: 1.78.0-nightly
LLVM version: 18.1.0

@rustbot modify labels: +regression-from-beta-to-nightly -regression-untriaged

[Urgau]

See #121621

But TLDR, not a false positive.

[Urgau]

No warning or at least no warning that tells me that this is an issue with the derive itself

Except that the non local impl is coming from the derive.

[weiznich]

Ok, let me clarify this issue again. I believe the emitted message is at least highly misleading for the following reasons:

warning: non-local impl definition, they should be avoided as they go against expectation

It talks about non-local impls, which in terms of traits and rust compiler messages implies coherence rules. The linked impl in question is local in that regard, as the type is local to that scope as well. (Well arguably only if you consider #[fundamental] types as local, but again then we are back to the point that local vs non-local has a quite clear definition in terms of implementing traits.) At very least this should use a different wording for local and non-local so that it will not be confused with the coherence rules. (A better solution in my opinion would be to just apply the coherence here as well, as that's what is actually relevant for the code in question. (After all we don't care about that impl outside that limited scope))

note: the derive macro crate::expression::AsExpression may come from an old version of the diesel_derives crate, try updating your dependency with cargo update -p diesel_derives

This part of the error message is factually wrong. The problem here is not that the derive comes from an older edition or that it needs to be updated. In fact it already uses the "suggested" anonymous constant pattern. The issue here (if it exists at all) is that the derive is used in a "local" scope. So that's a usage issue, instead of an issue with that derive. By wording this error message that way you essentially blame the developers of the derive crate instead of helping users to point out the actual issue with their code. At very least the lint should detect that and skip these lines if the anonymous constant pattern is there in the generated code.

As a more general note: As this is testing code for proc macro related functionality: How would the corresponding rust team expect that code to be written in a future edition of rust? I believe that this pattern is quite common for crates that offer derives (or similar macros), as it allows to easily define these items in scope of the actual test. The lint proposes to move that to a more global scope, which will make it much harder to write these tests as you now need to consider naming collisions and you move code away from the actual test.

[Urgau]

Ok, let me clarify this issue again. I believe the emitted message is at least highly misleading for the following reasons:

warning: non-local impl definition, they should be avoided as they go against expectation

It talks about non-local impls, which in terms of traits and rust compiler messages implies coherence rules. The linked impl in question is local in that regard, as the type is local to that scope as well. (Well arguably only if you consider #[fundamental] types as local, but again then we are back to the point that local vs non-local has a quite clear definition in terms of implementing traits.) At very least this should use a different wording for local and non-local so that it will not be confused with the coherence rules.

As noted this has nothing to do with coherence. I tried making that clear by adding the definition of "non-local impl" in the diagnostic output.

 = note: an `impl` definition is non-local if it is nested inside an item and neither the type nor the trait are at the same nesting level as the `impl` block

(A better solution in my opinion would be to just apply the coherence here as well, as that's what is actually relevant for the code in question. (After all we don't care about that impl outside that limited scope))

Except that it affects code outside that "limited scope", that's the issue!
See for a example: #121621 (comment)

note: the derive macro crate::expression::AsExpression may come from an old version of the diesel_derives crate, try updating your dependency with cargo update -p diesel_derives

This part of the error message is factually wrong. The problem here is *not that the derive comes from an older edition or that it needs to be updated. In fact it already uses the "suggested" anonymous constant pattern. The issue here (if it exists at all) is that the derive is used in a "local" scope. So that's a usage issue, instead of an issue with that derive. By wording this error message that way you essentially blame the developers of the derive crate instead of helping users to point out the actual issue with their code. At very least the lint should detect that and skip these lines if the anonymous constant pattern is there in the generated code.

This note was requested by T-lang and comes from the Crater run were we discovered that more than around 90% of the warnings could be fixed by just updating the dependency.

Regarding "This part of the error message is factually wrong.", I would like to mention that the note is saying "may" and "try", in the hope that macro author already fixed the problematic code.

As a more general note: As this is testing code for proc macro related functionality:

I don't understand this part. This lint doesn't test "proc macro related functionality", it checks impl definition, no matter if it's coming from a proc-macro or not.

[weiznich]

As noted this has nothing to do with coherence. I tried making that clear by adding the definition of "non-local impl" in the diagnostic output.

As written above: It would likely be clearer of you would just not use the terms local/non-local there as these have a clearly understood meaning for traits and impls related to the coherence rules. If that's not about coherence rules, just use different words here.

Except that it affects code outside that "limited scope", that's the issue!
See for a example: #121621 (comment)

Expect that's not relevant for code in a #[test] function in my opinion.

This note was requested by T-lang and comes from #120393 (comment) were we discovered that more than around 90% of the warnings could be fixed by just updating the dependency.

Well in that case the compiler can (and does) know that this cannot help as the code in question:

• Uses the derive in a function
• The derive already generates code that uses the anonymous const pattern.

Therefore there is no need to generate that hint all. The only effect that this hint will have is that users go to the issue tracker of the affected derive crate and fill issues there that their code needs to be updated, which puts more work on the maintainers of that crate. (Rant: Given that this is an edition related change, I shouldn't be surprised by that, given how all last two editions broke diesel. It seems to be easier for the rust project to just let the ecosystem deal with these kind of breaking change.)

I don't understand this part. This lint doesn't test "proc macro related functionality", it checks impl definition, no matter if it's coming from a proc-macro or not.

This lint warns against a commonly used testing pattern for crates that rely on proc macros. So essentially breaks that workflow. So please advice how that workflow can be fixed.

[Urgau]

Expect that's not relevant for code in a #[test] function in my opinion.

The issue can still be triggered in #[test] functions.

pub trait Trait: Sized {
    fn method(self) {}
}

#[test]
fn one_test() {
    struct Thing;
    impl Trait for &Thing {}
}

#[test]
fn second_test() {
    Trait::method(&{ todo!() });
    // this is a bit silly, but you could imagine `Thing: Default`, 
    // or some other way to actually construct `Thing`
}

---

For the rest, I will let T-lang respond.

[weiznich]

The issue can still be triggered in #[test] functions.

Maybe I miss something but it feels wrong to disallow this particular pattern just because it can be reused in some edge case. The lint currently targets all places where some impl is provided that could potentially be used in that way. It would be much better to instead link those places where these impls are actually misused, as that would greatly reduce the impact of that lint. Or can you explain what's wrong with this pattern as long as you don't use the impl outside of the scope of that function? (Or even what's wrong with it when using it outside of the current function, I mean there are more than enough other ways to define voldemort types, so why disallow this only particular one?)

[Urgau]

From the RFC PR:

Motivation

Currently, tools cross-referencing uses and definitions (such as IDEs) must
search inside all function bodies to find potential definitions corresponding
to uses within a function.

Humans cross-referencing such uses and definitions may find themselves
similarly baffled.

With this change, both humans and tools can limit the scope of their search and
avoid looking for definitions inside other functions.

[weiznich]

I'm well aware of that RFC. I raised my criticisms there before the RFC was accepted. I do net remember getting a response to my criticism back then, nor does it feel like any of that was incorporated into the design.

Anyway as you are trying to push this forward without replying to consents let me word my question differently:

Given the code linked in the issue, what would be the official endorsed way to write this specific test such that it fulfilled the proposed goals of the linked RFC.

[Urgau]

Sorry if you feel that I ignored your question, it wasn't my intention.

As for the question, I (personally) don't know¹, and even if I had an opinion this is T-lang call², I'm just a compiler guy.

Footnotes

1. well, apart from moving the struct outside of the function ↩

2. which is hopefully going to be addressed in next T-lang triage meeting ↩

[weiznich]

well, apart from moving the struct outside of the function

That would obviously work, but it will explicitly contraindicate the stated goal of the RFC as it requires moving away the relevant code from the test case. That in turn will require exactly that cross referencing that the RFC calls out as bad.

Also to write that down explicitly: That's no critic at your implementation of the lint, it's more about the underlying semantic change that doesn't seem to be fully understand in terms of impact to the ecosystem.

[apiraino]

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-medium

[apiraino]

Was this ever discussed by T-lang? By looking at the opening comment of #122747 I think it was probably handled by T-types, therefore removing T-lang nomination

@rustbot label -I-lang-nominated

[Urgau]

@apiraino It was discussed at the same time as #121621: #121621 (comment)

(I think we just forgot to remove the nomination here)