-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Avoid non-local definitions in functions #3373
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.
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
Merged
traviscross
merged 12 commits into
rust-lang:master
from
joshtriplett:avoid-nonlocal-definitions-in-fns
Jan 26, 2024
Merged
Changes from all commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
09a0e24
Avoid non-local definitions in functions
joshtriplett 632d8c9
Set RFC number
joshtriplett 5d5304e
Clarify that tools don't *have* to be looking for these definitions
joshtriplett 85c7849
Address closures as well
joshtriplett c29d41c
Note that no other language feature has this issue
joshtriplett 22ac798
Address macros
joshtriplett 66ef463
Handle other expression-containing items (static, const, enum discrim…
joshtriplett 218949d
3373: Change to a warn-by-default lint
joshtriplett d1e9965
3373: Add prior art
joshtriplett 01045be
3373: Define which token the lint attaches to
joshtriplett 4d28f57
3373: Allow definitions in anonymous `const` items
joshtriplett e2ae261
3373: Add unresolved question about anonymous `const` items
joshtriplett File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,88 @@ | ||
- 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 | ||
|
||
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. Consider ramping that lint to deny-by-default for Rust 2024, and | ||
evaluating a hard error for 2027. | ||
|
||
# Motivation | ||
[motivation]: #motivation | ||
|
||
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. | ||
|
||
Humans cross-referencing such uses and definitions may find themselves | ||
similarly baffled. | ||
|
||
This change helps humans limit the scope of their search and avoid looking for | ||
definitions inside other functions or items, without missing any relevant | ||
definitions. If in the future we manage to forbid it entirely within a | ||
subsequent Rust edtion, tools will be able to rely on this as well. | ||
|
||
# Explanation | ||
[explanation]: #explanation | ||
|
||
The following types of items, "expression-containing items", can contain | ||
expressions, including the definitions of other items: | ||
- Functions | ||
- Closures | ||
- The values assigned to `static` items or non-anonymous `const` items. | ||
- The discriminant values assigned to `enum` variants | ||
|
||
Rust will emit a warn-by-default lint for all of the following cases: | ||
- An item nested inside an expression-containing item (through any level of | ||
nesting) may not define an `impl Type` block unless the `Type` is also nested | ||
inside the same expression-containing item. | ||
- An item nested inside an expression-containing item (through any level of | ||
nesting) may not define an `impl Trait for Type` unless either the `Trait` or | ||
the `Type` is also nested inside the same expression-containing item. | ||
joshtriplett marked this conversation as resolved.
Show resolved
Hide resolved
|
||
- An item nested inside an expression-containing item (through any level of | ||
nesting) may not define an exported macro visible outside the | ||
expression-containing item (e.g. using `#[macro_export]`). | ||
joshtriplett marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
In a future edition, we may consider making this lint deny-by-default, or | ||
eventually making it a hard error. We'll evaluate the impact on the ecosystem | ||
and existing use cases before doing so. | ||
|
||
The lint is considered to attach to the `impl` token of an `impl` block, or the | ||
`macro_rules!` token of a macro definition. | ||
|
||
# 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
|
||
# Prior art | ||
[prior-art]: #prior-art | ||
|
||
Other aspects of Rust's design attempt to enable local reasoning and avoid | ||
global reasoning, including non-inference of function signatures, and not | ||
having the function body affect non-opaque properties of `impl Trait` uses in | ||
the signature without reflecting those properties in the signature. | ||
|
||
# Unresolved questions | ||
[unresolved-questions]: #unresolved-questions | ||
|
||
We'll need a crater run to look at how widespread this pattern is in existing | ||
code. | ||
|
||
Should we flag these definitions in anonymous `const` items as well, or would | ||
that produce unwanted warnings? | ||
|
||
# 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. |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.