Tracking Issue for RFC 3373: Avoid non-local definitions in functions

[traviscross]

This is a tracking issue for the RFC 3373: Avoid non-local definitions in functions (rust-lang/rfcs#3373).

Summary

Add a warn-by-default lint for items inside functions or expressions that implement methods or traits that are visible outside the function or expression.

warning: non-local `impl` definition, they should be avoided as they go against expectation
  --> src/de/impls.rs:21:9
   |
21 | /         impl Visitor for Place<()> {
22 | |             fn null(&mut self) -> Result<()> {
23 | |                 self.out = Some(());
24 | |                 Ok(())
25 | |             }
26 | |         }
   | |_________^
   |
   = help: move this `impl` block outside the of the current associated function `begin`
   = 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: `#[warn(non_local_definitions)]` on by default

About tracking issues

Tracking issues are used to record the overall progress of implementation. They are also used as hubs connecting to other relevant issues, e.g., bugs or open design questions. A tracking issue is however not meant for large scale discussion, questions, or bug reports about a feature. Instead, open a dedicated issue for the specific matter and add the relevant feature gate label.

Steps

• Implement the RFC: Implement RFC 3373: Avoid non-local definitions in functions #120393
• Adjust documentation (see instructions on rustc-dev-guide)
• Stabilization PR (see instructions on rustc-dev-guide)

Unresolved questions

• We need a crater run to look at how widespread this pattern is in existing code.
  • Implement RFC 3373: Avoid non-local definitions in functions #120393 (comment)
• Should we flag these definitions in anonymous const items as well, or would that produce unwanted warnings?
  • We do not currently warn on const _: () = { /* .. */ } since the pattern is widely used in the ecosystem, including serde_derive.

Related

• non_local_definitions common issues: impl for &Local, From<Local> for Global, ... #121621 (comment)
• Implement T-types suggested logic for perfect non-local impl detection #122747
• False positive and highly misleading suggestions for the non-local-definitions lint #124396
• non-local-definitions lint fires for impl using private types #125068
• non_local_definitions lint fires for impl Trait for NonLocalType<SomeLocalType>, probably shouldn't #126768
• Switch back non_local_definitions lint to allow-by-default #127015
• Rework non_local_definitions lint to only use a syntactic heuristic #127117

[dtolnay]

I pasted an example of such a warning into the summary above. This issue previously was not appearing in a GitHub search for "non_local_definitions".

[dtolnay]

Should an exception be made for #[fundamental] types?

Filed as #121621.

[bjorn3]

They are treated the same for the purpose of this lint. They still encapsulate name resolution.

[Manishearth]

@bjorn3 Hmm, in that case the current error message appears misleading. To me, it more or less explicitly says that they don't encapsulate name resolution.

[jstarks]

Is it intended that this lint fires in cases like these?

trait Tr {}
struct Foo<T>(T);
fn foo() {
    struct Bar;
    impl Tr for Foo<Bar> {}
}

Currently it fires.

This seems somewhat against the spirit of the lint. Foo<Bar> is cannot be named outside of foo(), so the reasoning behind the lint doesn't to apply. The impl isn't at the same level as its item, but it is at the outermost level it can be without making Bar visible to surrounding functions.

The lint also fires on fundamental types like Box<T>, which is even more unexpected:

trait Tr{}
fn foo() {
    struct Bar;
    impl Tr for Box<Bar> {}
}

This seems like a bug or at least a wart.

[traviscross]

@jstarks: That is worth filing a dedicated issue about, if you would.

[jstarks]

OK, #126768. Feel free to clean up the title and such.

[chinedufn]

I quickly scanned though the RFC rust-lang/rfcs#3373 comments and didn't see anything about macro generated runtime tables.

Here's a minimal example of code that should not become a hard error:

struct MyType;

// Heavily simplified version of a macro in my codebase. 
macro_rules! my_macro {
	($($fn_name:ident),*) => {
	    impl MyType {
		pub fn $fn_name(&self) {
	            SomeHandler::$fn_name()
		}
	    }

            do_stuff(&some_type, stringify!($fn_name));
	}
}

fn initialize(some_type: &MyType) {
    my_macro!(
        method1,
        method2,
        method_foo,
        method_bar,
    );
}

fn do_stuff(some_type: &MyType, method_name: &str) {
    // Oversimplified, in real code the key and value might be different here.
    some_type.insert_into_runtime_generated_table(method_name, method_name);
}

In the above example we initialize function takes in MyType.

It calls the macro my_macro! with names of methods to implement on MyType.

my_macro! also generates a runtime call to fn do_stuff that adds the stringified method name to some table.

To satisfy the lint the above could would need to be changed to:

// BAD: Now we're using two macro invocations instead of one.

struct MyType;


macro_rules! implement {
	($($fn_name:ident),*) => {
		impl MyType {
			pub fn $fn_name(&self) {
			    SomeHandler::$fn_name()
			}
		}
	}

}

macro_rules! bind {
	($($fn_name:ident),*) => {
        do_stuff(&some_type, stringify!($fn_name));
	}
}


implement! {
	method1,
	method2,
	method_foo,
	method_bar,
};

fn initialize(some_type: MyType) {
	bind!(
        method1,
        method2,
        method_foo,
        method_bar,
    );
}

fn do_stuff(some_type: &MyType, method_name: &str) {
    // ...
}

Pattern

The pattern here is when you need to implement a method on a type AND also register that method name in some runtime generated table.

Real World

This section is included to help prove that the above problem can be encountered in real-world code. It does not need to be read to understand the problem. For this reason I did not take the time to make this real-world example easy to understand / illustrate with simple code examples.

A real world example of when something like this comes up is when using wasm-bindgen to instantiate a WebAssembly module instance at runtime.

To expose a Rust type's methods to the wasm module you need to write code that essentially maps NAME OF WASM IMPORT -> NAME OF HOST METHOD.

This way when the wasm module calls my_wasm_module_import the host_method gets called.

Closing Thoughts

RFC 3373 rust-lang/rfcs#3373 states the following as the motivation for the change:

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.

In my case the human would be more confused if they had to check two macros to understand this pattern instead of one.

My alternative would be to write a build script or some other form of codegen to generate this, but that would be more difficult for the human to find and maintain than a simple inlined macro.

[bjorn3]

Can you generate the initialize function directly from your macro?

[chinedufn]

My real initialize method does a little bit of setup before invoking the macro.

My development environment's syntax highlighting / other tools work better outside of macros than inside, so I'd prefer to avoid moving code into a macro to satisfy a lint.

---

That said, I could have my macro generate the initialize and then have initialize call some other initialize_inner_setup() function, then move my other setup code into fn initialize_inner_setup.
Then my macro could remain fairly lean.

my_macro! {
    // generated impl blocks here

    fn initialize() {
        initialize_inner_setup();
        
        // ... generated code that inserts into a runtime table here ...
    }
}

my_macro!();

#[inline]
fn initialize_inner_setup() {
    // ... setup code that does not need to be in the macro ...
}

---

The downside of extracting out the initialize_inner_setup is that my initialization routine no longer reads top to bottom.
You need to go look at initialize_inner_setup, which should only be a few lines that would've been better off at the top of the fn initialize.

That said, when given a better name this hypothetical initialize_inner_setup would be named clearly enough that you wouldn't need to go read it to it to understand.
Something like fn specific_thing_that_is_happening() {}

---

None of this is a big deal for my own codebase since this pattern only occurs once, just illustrating the trade-off.

---

In practice it would be fine for me to generate the entire initialize function in a single macro invocation since it barely has any code in it.
And if it had a lot of code I would want to move parts of it into other sub-functions like the initialize_inner_setup example.

So generating everything in a single macro invocation would be okay in my case.

[traviscross]

We didn't make a decision to increase the severity of this lint in Rust 2024, so this is not a Rust 2024 edition item.

We could always decide to bump the severity of this lint in a future edition, of course, just as we could for any other lint. But we probably don't need to hold the tracking issue open for that.

So, seeing as how everything else seems done here in terms of implementing RFC 3373, let's go ahead and mark this as completed.