Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Avoid non-local definitions in functions #3373

Merged
merged 12 commits into from
Jan 26, 2024
Merged

Avoid non-local definitions in functions #3373

Changes from 5 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
09a0e24
Avoid non-local definitions in functions
joshtriplett Jan 20, 2023
632d8c9
Set RFC number
joshtriplett Jan 20, 2023
5d5304e
Clarify that tools don't *have* to be looking for these definitions
joshtriplett Jan 20, 2023
85c7849
Address closures as well
joshtriplett Jan 20, 2023
c29d41c
Note that no other language feature has this issue
joshtriplett Jan 20, 2023
22ac798
Address macros
joshtriplett Jan 20, 2023
66ef463
Handle other expression-containing items (static, const, enum discrim…
joshtriplett Jan 20, 2023
218949d
3373: Change to a warn-by-default lint
joshtriplett Jan 3, 2024
d1e9965
3373: Add prior art
joshtriplett Jan 3, 2024
01045be
3373: Define which token the lint attaches to
joshtriplett Jan 3, 2024
4d28f57
3373: Allow definitions in anonymous `const` items
joshtriplett Jan 3, 2024
e2ae261
3373: Add unresolved question about anonymous `const` items
joshtriplett Jan 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 63 additions & 0 deletions text/3373-avoid-nonlocal-definitions-in-fns.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
- Feature Name: N/A
- Start Date: 2022-01-19
- RFC PR: [rust-lang/rfcs#3373](https://github.com/rust-lang/rfcs/pull/3373)
- Rust Issue: [rust-lang/rust#0000](https://github.com/rust-lang/rust/issues/0000)

# Summary
[summary]: #summary

Starting in Rust 2024, stop allowing items inside functions to implement
methods or traits that are visible outside the function.

# Motivation
[motivation]: #motivation

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

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, without missing any
relevant definitions.

# Explanation
joshtriplett marked this conversation as resolved.
Show resolved Hide resolved
[explanation]: #explanation

Starting in the Rust 2024 edition:
- An item nested inside a function or closure (through any level of nesting)
may not define an `impl Type` block unless the `Type` is also nested inside
the same function or closure.
- An item nested inside a function or closure (through any level of nesting)
may not define an `impl Trait for Type` unless either the `Trait` or the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about impl Inner<Outer> for Type or impl Trait for Inner<Outer>?

Copy link
Member

@lnicola lnicola Jan 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If Inner and Outer (and you've called that simply because generics are involved) are non-local traits/types, these are just as problematic.

If Inner is a local (declared next to the impl) type or trait and Outer is non-local, I think it's fine to allow them because they won't be visible to the code outside.

I think it boils down to: "looking at some code, can we resolve the methods and names in it without parsing the bodies of the other functions (statics, consts etc.), with the exception of the ancestors of the given code?".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant "Inner" == "local", and "Outer" == "non-local".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If only parameters of the trait/type are non-local I think it should be fine? Those impls still can't be observed from the outside since you'd need to be able to mention the trait/type still if I am not mistaken

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not necessarily, you can actually leak local types to the outside like this: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=1921433dd4209e89c7c96949a23e99b8

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neither the trait nor the type in that impl are local to the function though, so that should be rejected by these restrictions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And potentially also cases like in https://internals.rust-lang.org/t/overly-strict-coherence-for-constrained-blanket-implementations/18204/8. I think there's like a whole coherence aspect to this?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can still leak non-opaque types while following the proposed rules. If that seems far fetched, consider -> impl IntoIterator<Item = X> where the IntoIter associated type is not specified (but you're going to generate one).

`Type` is also nested inside the same function or closure.
joshtriplett marked this conversation as resolved.
Show resolved Hide resolved

Rust 2015, 2018, and 2021 continue to permit this, but will produce a
joshtriplett marked this conversation as resolved.
Show resolved Hide resolved
warn-by-default lint.

No other language features provide a means of defining a name inside a function
and referencing that name outside the function.
joshtriplett marked this conversation as resolved.
Show resolved Hide resolved

# Drawbacks
[drawbacks]: #drawbacks

Some existing code makes use of this pattern, and would need to migrate to a
different pattern. In particular, this pattern may occur in macro-generated
code, or in code generated by tools like rustdoc. Making this change would
require such code and tools to restructure to meet this requirement.

joshtriplett marked this conversation as resolved.
Show resolved Hide resolved
# Unresolved questions
[unresolved-questions]: #unresolved-questions

We'll need a crater run to look at how widespread this pattern is in existing
code.

# Future possibilities
[future-possibilities]: #future-possibilities

If in the future Rust provides a "standalone `derive`" mechanism (e.g. `derive
Trait for Type` as a standalone definition separate from `Type`), the `impl`
produced by that mechanism would be subject to the same requirements.