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.

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

non_local_definitions lint is unhappy with multiple nested anonymous constants #131474

Closed
clarfonthey opened this issue Oct 9, 2024 · 4 comments · Fixed by #131498
Closed

non_local_definitions lint is unhappy with multiple nested anonymous constants #131474

clarfonthey opened this issue Oct 9, 2024 · 4 comments · Fixed by #131498
Labels
A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. C-bug Category: This is a bug. L-non_local_definitions Lint: non_local_definitions T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@clarfonthey
Copy link
Contributor

I tried this code:

struct Test;
const _: () = {
    const _: () = {
        impl Default for Test {
            fn default() -> Test {
                Test
            }
        }
    };
};

This should be completely okay, since the impl is only nested within anonymous constant blocks.

Instead, you get this backwards-compatibility warning:

warning: non-local `impl` definition, `impl` blocks should be written at the same level as their item
 --> src/lib.rs:5:9
  |
4 |     const _: () = {
  |     ----------- move the `impl` block outside of this constant `_` and up 2 bodies
5 |         impl Default for Test {
  |         ^^^^^-------^^^^^----
  |              |           |
  |              |           `Test` is not local
  |              `Default` is not local
  |
  = 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`
  = note: items in an anonymous const item (`const _: () = { ... }`) are treated as in the same scope as the anonymous const's declaration for the purpose of this lint
  = 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:

rustc 1.83.0-nightly (6f4ae0f34 2024-10-08)
binary: rustc
commit-hash: 6f4ae0f34503601e54680a137c1db0b81b56cc3d
commit-date: 2024-10-08
host: x86_64-unknown-linux-gnu
release: 1.83.0-nightly
LLVM version: 19.1.1

This is currently affecting the bevy_reflect crate, which has a few instances of doubly-nested anonymous constants generated by macros. Particularly, the impl_type_path! macro nests its result in an anonymous constant, and the impl_reflect_for_atomic! calls this macro inside an anonymous constant. The anonymous constants exist to allow local imports without affecting the larger scope.

@clarfonthey clarfonthey added the C-bug Category: This is a bug. label Oct 9, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Oct 9, 2024
@jieyouxu jieyouxu added L-non_local_definitions Lint: non_local_definitions A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Oct 10, 2024
@jieyouxu
Copy link
Member

cc @Urgau as you might know more about this.

@Urgau
Copy link
Member

Urgau commented Oct 10, 2024

Considering const _ : () as "transparent" is mainly a compatibility enhancement, so considering multiple consecutive of them as "transparent" would go in this direction, even though I'm not too thrilled about the general direction.

cc @traviscross (since this is a lang issue)
@rustbot label +T-lang

@rustbot rustbot added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Oct 10, 2024
@traviscross
Copy link
Contributor

traviscross commented Oct 10, 2024

I'd expect these to be recursively transparent, yes. That is, I would not expect the lint to fire here.

@clarfonthey
Copy link
Contributor Author

Yeah, I think that it would be extremely confusing to only consider a single layer of these, even though I understand why the code currently does this.

To me, it should either be all recursively or none. I think it's fair to say that the presence of recursive nesting makes it worth reconsidering whether they should be added as an exception to the lint, but I don't think that the one-layer option should be considered, especially considering how as the lint is worded, that isn't apparent.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Oct 11, 2024
Consider outermost const-anon in `non_local_def` lint

This PR change the logic for finding the parent of the `impl` definition in the `non_local_definitions` lint to consider multiple level of const-anon items, instead of only one currently.

I also took the opportunity to cleanup the related code.

cc `@traviscross`
Fixes rust-lang#131474
@bors bors closed this as completed in e00f49d Oct 11, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Oct 11, 2024
Rollup merge of rust-lang#131498 - Urgau:transparent-const-anons, r=lcnr

Consider outermost const-anon in `non_local_def` lint

This PR change the logic for finding the parent of the `impl` definition in the `non_local_definitions` lint to consider multiple level of const-anon items, instead of only one currently.

I also took the opportunity to cleanup the related code.

cc ``@traviscross``
Fixes rust-lang#131474
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. C-bug Category: This is a bug. L-non_local_definitions Lint: non_local_definitions T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants