non_local_definitions lint fires for impl Trait for NonLocalType<SomeLocalType>, probably shouldn't

[jstarks]

#120363 introduces some warnings that seem like bad advice. Nightly now warns if a function has an internal implementation of a trait on a non-internal (to that function) type. That seems like a reasonable warning in the general case, but it doesn't seem so reasonable for types like Box<Inner>, where Inner is defined within the function and cannot be named outside it.

It rather seems like something like the orphan rule should apply. You should be able to impl traits on Foo<T> as long as T is defined at the same level as the impl.

Playground link

I tried this code on nightly:

#![allow(dead_code)]
trait Tr {}
fn foo() {
    struct Bar;
    impl Tr for Box<Bar> {}
}

I expected to see this happen: no errors/warnings

Instead, this happened:

warning: non-local `impl` definition, `impl` blocks should be written at the same level as their item
 --> src/lib.rs:5:5
  |
5 |     impl Tr for Box<Bar> {}
  |     ^^^^^--^^^^^---^^^^^
  |          |      |
  |          |      `Box` is not local
  |          `Tr` is not local
  |
  = note: `impl` may be usable in bounds, etc. from outside the expression, which might e.g. make something constructible that previously wasn't, because it's still on a publicly-visible type
  = note: an `impl` is never scoped, even when it is nested inside an item, as it may impact type checking outside of that item, which can be the case if neither the trait or the self type are at the same nesting level as the `impl`
help: move the `impl` block outside of this function `foo`
 --> src/lib.rs:3:1
  |
3 | fn foo() {
  | ^^^^^^^^
4 |     struct Bar;
  |     ---------- may need to be moved as well
  = 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: `#[warn(non_local_definitions)]` on by default

Meta

rustc --version --verbose (well, from the playground):

Build using the Nightly version: 1.81.0-nightly
(2024-06-19 d8a38b00024cd7156dea)

Backtrace

<backtrace>

[Urgau]

where Inner is defined within the function and cannot be named outside it.

It's not because it cannot be named that it cannot be inferred.

This can be demonstrate with your example, here Bar can be inferred outside the foo function, showing that the impl definition is not local:

trait Tr {}

fn foo() {
    struct Bar;
    impl Tr for Box<Bar> {} // with this impl definition
}

fn do_stuff<U: Tr>() {}

fn main() {
    do_stuff::<Box<_>>(); // this code will compile, as `_` will be resolved to `Bar`
}

@rustbot labels -C-bug +C-discussion +L-non_local_definitions -needs-triage

[traviscross]

@rustbot labels +I-lang-nominated

This is kind of a surprising interaction of things. Nominating as we should discuss for visibility and to confirm whether this is what we meant to do.

(In general, we had wanted to support people making local impls for local types on outer traits. While it's clear the impls in this issue do have a non-local effect, the only workaround is to promote the local type to a broader scope outside of the function. That seems a bit strange and unfortunate, and probably isn't what the user wanted to do. There may or may not be a better option; we should just discuss to confirm.)

[Urgau]

@traviscross I agree that this interaction is surprising and I think that because it is surprising (and non-intuitive) we should warn about it.

@workingjubilee brought up in #125068 (comment) an example where someone might write a private type that for someone might rely on privacy to not be constructible outside of the function:

Also, just to remove all doubt as to whether this is a Real Problem, I present a new example of a completed escape:

pub trait Trait {}
fn test() {
    // Private field to make it more annoying to construct!
    #[derive(Default)] // oh, but what's this...?
    struct Local(i32);
    impl Trait for Option<Local> {}
}

fn do_stuff<U: Trait + Default>() -> U {
    Default::default()
}

fn main() {
    let x = do_stuff::<Option<_>>().unwrap_or_default();
    println!("an instance of {} escaped", std::any::type_name_of_val(&x));
}

I think that code would be something someone would probably write, especially if we're talking about a fairly big private function, not just the 4 lines in this example but more like 400 lines. They happen. But if someone was relying on that type not being constructible outside that fn, they are now dead wrong!

Note that it doesn't work with just U: Default though because that won't unambiguously resolve and it will demand type annotations. So this is relying on the fact rustc secretly knows there is another type, but only one, that can apply.

It's still not clear to me whether we should trigger the lint on the initial example here, as this requires two steps... a way to make the type constructible... but the answer to "is there something unexpected", i.e. is this making the code (and moreover, the code's implications) harder to understand... is probably Very Yes.

As for "bad" or "good", well, I don't like the thought of calling this code "bad" because it doesn't seem inherently objectionable. You may simply not care that much. But I don't think it's a great shining example of code either. It's just... there.

[traviscross]

@rustbot labels -I-lang-nominated -C-discussion +C-bug

We discussed this in the lang call today. We had everyone there, and our unanimous consensus was that we do not want the RFC 3373 lint to fire in these cases. E.g., we do not want to lint against:

trait Tr {}
fn foo() {
    struct S {};
    impl Tr for &S {}
}

This has non-local effect due to the "only one thing could go here" inference rule. But still, we consider this beyond the intention of the lint, and if we were to later try to tighten things up (e.g. for the benefit of tooling), we may consider instead trying, in a new edition, to do away with or limit the inference rule (e.g. by adding a check to error if the type being inferred can't be named). For that, we discussed how there may be some precedent for it in the RFC 2145 work from @petrochenkov.

That is, we'd expect people to be able to write (lint-free) the code above, and so we're OK with accepting the leakage here, as we tentatively plan to deal with that in a different way.

Thanks to @jstarks for raising this point and filing this issue.

[workingjubilee]

Feedback from reviewing the new PR to implement the new heuristic is that it isn't clear why the lint is being implemented at all if this new decision is being made.

I myself am slightly concerned about the summary of the consensus. "Edicts from on high" by T-lang... by which I mean, "we want this, not that, so do it"... are okay when it's clarifying something very small (sometimes someone just needs to make a decision and people don't feel they have authority to make that decision). However, when something is as fundamental as the raison d'être for a lint's very existence and the implementation of it raises fundamental questions like "wait, why?" then a larger explanation is in order, unfortunately.

So it's no longer clear what the goals of this lint are. I realize this is just asking for someone to basically just repeat the already-accepted RFC in summary form, but with the current information we have from implementation woes and experimentation. However, this is not a waste of time: it is worth reevaluating if RFC 3373 + this recent comment make sense when put together.

[traviscross]

We were trying to achieve a certain language outcome with this lint. We wanted to lint against "sneaky inner impls" that look like this:

trait Tr {}
struct S {}
fn foo() {
    impl Tr for S {} //~ Sneaky!
}

We wanted this because these are just weird, and we can't think of any good reasons people should write them.

We specifically wanted to allow this:

trait Tr {}
fn foo() {
    struct S {}
    impl Tr for S {} //~ Not sneaky!
}

And this:

struct S {}
fn foo() {
    trait Tr {}
    impl Tr for S {} //~ Not sneaky!
}

It's clear why someone would want to do this sort of thing.

We didn't think, when adopting the RFC, about the fact that, due to the 1-impl rule, this has non-local effect:

trait Tr {}
fn foo() {
    struct S {}
    impl Tr for &S {} //~ Sneaky?
}

We didn't mean for that to be a "sneaky inner impl". That just isn't in the same bucket for us, as a language matter. It's just as clear to us why someone would want to write that as it is why someone would want to write the impl Tr for S version.

So the perfect implementation of this, in a types sense, results in an undesirable language outcome. It's undesirable, because, in our view, the "sneaky" part is what the language is doing due to the 1-impl rule rather than what the user wrote.

We should have caught this when it happened, here, and then in #122747. My view is that none of us really grasped at that time what the effect of that work would be. That resulted in wasting other people's time and effort, and we're sorry about that. We'll try to do better.

The key question, with respect to this lint, is whether its purpose should be to:

1. Catch everything that could leak out and have non-local effect due to the way the type system operates.
2. Lint against certain patterns the lang team finds undesirable and which we see no good reason for users to write.

We chose Option 2, and felt that's what we had meant to do in adopting RFC 3373.¹

For now, the non_local_definitions lint has been switched back to allow-by-default:

• Switch back non_local_definitions lint to allow-by-default #127015

...and beta-backported. That all seems the right thing to do.

Does RFC 3373 still make sense at all? We felt it did, from the perspective of wanting to lint against the specific patterns that we felt were undesirable and unmotivated, but I'm interested to hear reasons if people don't think this makes sense, in light of the context here about what we were trying to achieve.

Footnotes

1. As a historical note, I should say that in the discussions for RFC 3373, there were other broader motivations that were proposed, and the original drafts of RFC 3373 correspondingly went further in pushing to forbid these patterns. We didn't achieve consensus on wanting to go that far, and correspondingly, to those broader motivations, so the RFC was cut back to that on which we did have consensus, which was to lint at warn-by-default against certain patterns that we felt were undesirable and unmotivated. ↩

[GoldsteinE]

Motivation part of RFC 3373 specifically names tooling as a reason for the lint:

Currently, tools cross-referencing uses and definitions (such as IDEs) must either search inside all function bodies and other expression-containing items to find potential definitions corresponding to uses within another function, or not cross-reference those definitions at all.

From the perspective of e.g. “find references” or “find implementations”, the second case is sneaky. “find implementations” on trait Tr can’t find struct S {}, which is an implementor, without inspecting function bodies. A case like the third one,

trait Tr {
    fn method() -> Self;
}

fn foo() {
    struct S;
    impl Tr for &S {
        fn method() -> Self { &S }
    }
}

fn main() {
    let x = <&_ as Tr>::method(); // ← go-to-definition here
}

would also require tooling to scan method bodies.

Unless I’m missing something, the lint in its currently proposed form can’t really help tooling even if strictly enforced, so the first part of the motivation is no longer relevant?

[traviscross]

Sometimes when RFCs get pared back to match a consensus, the normative sections don't end up matching the non-normative sections exactly. That's what happened here.¹ We didn't achieve consensus on committing ourselves to forbidding these patterns, and without a commitment to forbidding these, we're not actually addressing that motivation, regardless of how we implement this lint. A warn-by-default lint doesn't really help the tooling.

E.g., from our minutes:

NM: I'm a bit worried about this. First of all, I'm unconvinced that this is as important as people think it is for performance. Secondly, I'm worried about fallout from trying to drive this, e.g. for the reason that TC noted.

I.e., we didn't end up having consensus on that motivation. We had consensus on linting at warn-by-default against these weird patterns.

See also the footnote in my comment above.

Footnotes

1. In fact, we ended up paring back the RFC so much that we discussed how having an RFC just for adding a lint like this was overkill, and that we wouldn't typically need or accept an RFC here other than that it was already written and was the path-dependent way in which we had decided to add this lint. ↩

[GoldsteinE]

From the perspective of human readability (which seems to be the only remaining motivation), is it useful to trigger the lint in macro expansion results? Humans don’t usually read these, and it’s often impossible to tell apart e.g. named and unnamed consts on the macro callsite. Any implementations coming from macros are invisible for humans, regardless of whether they’re on the top level or not.

Macros seemed to be a big source of churn with this lint, and I’m not sure having it in the macro expansions is worth it.

[joshtriplett]

I think a particular point @traviscross mentioned is worth highlighting here:

and if we were to later try to tighten things up (e.g. for the benefit of tooling), we may consider instead trying, in a new edition, to do away with or limit the inference rule (e.g. by adding a check to error if the type being inferred can't be named).

In other words, if we do want to try to achieve the goal of perfectly forbidding "sneaky inner impls", we may want to consider in part doing this by preventing the "only one possible type" rule from allowing those impls to be extracted from a function.

Also, an implementation note: that should be "can't name" (as in a type hidden inside a function), not "don't have in scope"; it's important to be able to infer types that haven't been imported, so that let thing = make_a_thing(); doesn't require importing Thing.

[workingjubilee]

@traviscross Thank you for the summary!

I have a question regarding this passage:

We didn't think, when adopting the RFC, about the fact that, due to the 1-impl rule, this has non-local effect:

trait Tr {}
fn foo() {
    struct S {}
    impl Tr for &S {} //~ Sneaky?
}

...

We should have caught this when it happened, here, and then in #122747. My view is that none of us really grasped at that time what the effect of that work would be. That resulted in wasting other people's time and effort, and we're sorry about that. We'll try to do better.

My understanding is that this had been brought up in RFC 3373, hereabouts, and that this comment was neither resolved nor decided on, nor even explicitly accepted as an unknown question. In fact it was also brought up some time afterwards in the then-closed issue (not very helpful as a venue, I know, but...). I believe these discussions are both referencing, essentially, the same thing that this issue is about (and that other issues are about, as a consequence...).

Is my understanding correct: that this question was known and raised before the RFC completed its FCP (...by about 5 hours, but nevertheless) and not addressed?

[traviscross]

Looking now at the comment by @petrochenkov that you noted, it does look like we missed it, and certainly that we missed the applied consequences of it.

We're planning a retrospective on this topic to look at what went wrong in processes and communication so that we can do better going forward.

[BoxyUwU]

I am still somewhat unclear on the purpose of this lint. If it's not there to help tooling then what is the point, I see vague references to it being an "undesirable" or "weird" pattern but.... why? I do understand that it's... a slightly peculiar thing to write but it doesn't necessarily seem confusing or even dangerous to me. "Lint against certain patterns the lang team finds undesirable and ...." to me implies that T-lang has talked about this but somehow this reasoning was not communicated on this issue, only the "final decision" that T-lang came to.

I find it a bit disappointing that after jubilee's comment asking for clarification on the motivation behind this lint there was... a lot of text! It basically just amounts to "we find this pattern undesirable and weird", but that seems slightly self evident by the fact that T-lang wants this lint even with it not being "fully correct". I have come away from reading this thread with still no idea why this lint is desirable.

In general I find the motivation for this lint to be very weird. There were two motivations:

• IDEs don't want to have to look into arbitrary function bodies
• Humans "cross referencing uses and definitions" may be confused

The first one has been completely discarded. The second one is weird? I'm not sure how many rust users there are (other than @workingjubilee, hi) that try to find out "does this type implement this trait" or "what traits does this type implement" by manually looking through every crate and module that could possibly write such an impl. That seems like a significant amount of work and also error prone as it relies on having a good mental model of coherence/orphan rules.

Almost everybody who wants to answer that question ought to be able to ask a tool this question and get the correct answer, (e.g. look at rustdoc on the trait or type, or go to the type and look at the little "N implementations" pop up r-a provides). For people without the "official tooling", I would expect general purpose text search tools to also be used to find impl for the relevant type/trait, this is not going to have any issues with impls nested inside of function bodies.

The fact that the first motivation is completely gone, and the set of users that would benefit from this lint seems likely small, makes me wonder why this is a valuable lint to have in rustc instead of an opt-in clippy lint for "interesting" code that somebody might prefer not to write for stylistic reasons.

Primarily that kind of thing is what I'd T-lang to give input on. What is the benefit of having this lint? Why is impl Trait for Local so undesirable to allow in a function body that it would warrant an on by default lint in rustc instead of an opt-in style lint in clippy? Do we have any actual concrete knowledge about how useful this is for users?

Nominating this (again 😭) to hopefully get this info. All in all, thanks to everybody who's worked on this, I imagine this has been pretty stressful for everybody involved ❤️

[BoxyUwU]

As a side note @traviscross you mentioned:

A warn-by-default lint doesn't really help the tooling.

The original RFC specified this would wind up being deny by default in some future edition so I do not think it is accurate to say that a lint would never have addressed the tooling motivation.

[Veykril]

To also comment from rust-analyzer's side, code like

trait Tr {
    fn fun() {}
}
fn foo() {
    struct S {};
    impl Tr for &S {}
}
fn bar() {
    <&_>::fun();
//  ^^^^^^ error: no such associated item
}

will simply always error in r-a. There is no heuristic rust-analyzer can apply to resolve this or at least figure out that this might exist in a local impl. So moving back to syntactic instead of type-driven hurts tooling in this regard.

Now this step would be fine in theory if you are actually planning on getting rid of the 1-impl rule in the future, but will that ever happen? I kind of doubt that.

Also note one other smaller thing regarding the questioning of the lints usefulness after this proposed change, there are two things being checked here: Leaking local impls and leaking local #[macro_export] the latter not being effected by any of the changes to the lint here.

[workingjubilee]

I am personally not even convinced that warn-by-default is not helpful: even a warning helps tooling by discouraging people from writing edge cases that the tooling will do poorly on, which allows it to deprioritize satisfying those edge cases. I mean, we would all like our tools to be fast and correct even in unusual edge cases, but in reality, those tools are necessarily being written in less time than the compiler has had to mature and accumulate complexity. Something something, arrow of time and all that.

[traviscross]

Some on lang may have thought this was a useful first step in that way. Some had reservations we could or would want to ever go much further. Some thought it would get us more information for whether that is possible or desirable. Everyone could agree to lint against these specific cases.

And everyone on lang cares deeply about tooling.

[ChrisDenton]

Some thought it would get us more information for whether that is possible or desirable.

Can this sort of thing not be done in Clippy, as boxy suggested?

[davidbarsky]

We, on rust-analyzer, get questions about this decently frequently. I'd say we've received about 4-5 questions about this over the past year in new and existing issues, but knowing the ratio of people complaining to simply putting up with an issue, I'd say the ratio is roughly 1:100. Which is to say: it's a decently prevalent issue and having lint would help a bunch of people who are otherwise confused.

As @GoldsteinE said, the second case is sneaky for rust-analyzer. I'd like to that add to that statement by citing this blog post by matklad where he noted that the second case is not sneaky for rustc because of the respective architectures of IDEs and compilers ("bottom up" versus "top down"). This means that rustc is uniquely positioned to implement this lint, whereas rust-analyzer cannot have a similar lint without running into the issue that this lint seeks to prevent in the first place!

@traviscross are these the meeting minutes? I'm not able to find where Niko said "I'm a bit worried about this. First of all, I'm unconvinced that this is as important as people think it is for performance. Secondly, I'm worried about fallout from trying to drive this, e.g. for the reason that TC noted."

To respond to Niko's comment, I'm not sure what sort of data we can reasonably provide because it feels like a chicken-and-egg issue. However, I can say that our efforts to shrink the search space of O(project)-style operations have dramatically improved rust-analyzer's performance and this lint makes an unsound trick that rust-analyzer uses substantially less unsound.

[traviscross]

We decided to pare back the RFC in our meeting on 2024-01-03. The minutes for that are as follows:

TC: The nominated question from tmandry is roughly, "where is this with respect to Rust 2024?"

TC: As far as I know, there hasn't been movement on this since February.

Felix: Quick note: ekuber and I discovered other probable-undesirable variants of this, like allowing impl items in const blocks.

JT: No additional context here, but happy to update the RFC based on whatever we decide.

tmandry: Was just hoping for a straw poll here. It's probably a more complicated change than it first seems on the surface.

JT: It's certainly not trivial. If we were going to do it, we'd probably want the RFC merged in the next week or two.

tmandry: We may need to replace uses of this with things like an anonymous module.

Felix: This is used in const initializers also.

TC: That's actually used for hygiene in macros.

NM: I'm a bit worried about this. First of all, I'm unconvinced that this is as important as people think it is for performance. Secondly, I'm worried about fallout from trying to drive this, e.g. for the reason that TC noted.

NM: If they're even tokenizing, as they have to do, then's probably an 80/20 thing that tools like RA could do.

JT: There are two potential benefits to this. One is simplifying things for IDEs. The other is simplifying it for humans. The second is the more important reason for me.

pnkfelix: But that could be a lint. My feeling is that if we do anything at all, it should start as a lint.

NM: It seems like a lint could have a lot of the benefit.

JT: Certainly if we were to add a lint it would cause us to learn about the potential cases.

NM/tmandry: +1.

pnkfelix: Clippy first?

JT: That would be too slow.

JT: I'd be happy to update the RFC to say that we add a deny-by-default lint for this, then we could reevaluate in 2027.

TC: Is deny-by-default too strong to start?

pnkfelix: I'd start with warn-by-default.

NM: Warn-by-default seems better. This seems similar, e.g., to warning about pub not reachable from the crate root.

JT: deny-by-default we could do with the 2024 edition.

JT: I'll propose warn-by-default in all editions, we'll later evaluate deny-by-default for 2024, and we'll reevaluate forbid for 2027.

Consensus: As above.

We decided to not warn about the impl Tr for &S cases in the meeting on 2024-06-26. The minutes for that are as follows:

TC: ...But the outcome is a kind of surprising language result. Specifically, people had probably wanted the type or trait to be local, and the only way for them to resolve this warning is to make them both non-local. (The diagnostic for this case is wrong too, because it tells people to put the impl at the same level as the item, which it already is.)

Anyway, it seemed worth confirming that this is what we really meant to do.

TC: What do we think?

Josh: It seems that our reasonable options are to not warn about this, since it doesn't seem like a big deal that it could leak in this way, or go ahead and continue linting against it, since it is technically leaking, or the third option is to reconsider the "only one thing could go here" rule in a future edition.

pnkfelix: I would have thought that one could have implemented this lint as a simple syntactic heuristic rather than being a complicated type system thing.

Josh: I don't think anything stops us from implementing it that way. Right now it's been implemented very technically on the question of "could it leak out at all".

NM: I think I'm in the camp of that this is an unwanted inference result in the language. I don't think, in the program above, that we should infer an inner struct that you can't name. I lean toward the option of not linting for now, then, if we later wanted to move toward a harder error, we could look at making the inference not work this way.

Josh: That would be my inclination as well. I don't think we necessarily need to rip out the "only one thing could go here" rule; we could add a post check for whether that thing is a completely inaccessible type defined inside an inner scope.

NM: I believe we have some precedent here. petrochenkov wrote an RFC revisiting the rules on public and private. Would check after inference whether the type you inferred was public.

TC: It's RFC 2145:

rust-lang/rfcs#2145

Josh: Let's not lint against this now (on that basis that we may later want to stop doing this strange inference anway).

NM: Agreed. I'm concerned about this case leading to subtle bugs in unsafe code along the lines of the problems we've seen with Pin -- i.e., authors assuming the struct is inaccessible outside the function and relying on that. I'd rather we revisit the inference rules and try to address that.

tmandry: I'm aligned on not linting on this case. I'd expect to be able to write the code above. Hopefully we can close the inference loophole here eventually.

scottmcm: I'm excited by the direction of "let's change bigger rules in other places" and so if, for now, we don't lint here, that's good for me.

pnkfleix: +1, let's not lint there.

[traviscross]

We, on rust-analyzer, get questions about this decently frequently. I'd say we've received about 4-5 questions about this over the past year in new and existing issues...

In the relatively short period that this lint was in nightly and beta, there were a number of duplicate issues filed over the surprising behavior of it. That is, extending the lint to these impl Tr for &S cases has a measurable cost too.

[traviscross]

@rustbot labels -I-lang-nominated

Talked with @BoxyUwU who meant to remove the nomination here, so let's do that.