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

Also treat impl definition parent as transparent regarding modules #132453

Merged
merged 1 commit into from
Nov 2, 2024

Conversation

Urgau
Copy link
Member

@Urgau Urgau commented Nov 1, 2024

This PR changes the non_local_definitions lint logic to also consider impl definition parent as transparent regarding modules.

See tests and explanation in the changes.

@rustbot label +L-non_local_definitions
Fixes (after beta-backport) #132427
cc @leighmcculloch
r? @jieyouxu

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. L-non_local_definitions Lint: non_local_definitions labels Nov 1, 2024
@jieyouxu
Copy link
Member

jieyouxu commented Nov 1, 2024

(Nit: did u mean threat -> thread in PR title and commit msg?)

@Urgau Urgau changed the title Also threat impl definition parent as transparent regarding modules Also treat impl definition parent as transparent regarding modules Nov 1, 2024
@Urgau
Copy link
Member Author

Urgau commented Nov 1, 2024

(Nit: did u mean threat -> thread in PR title and commit msg?)

Nope, I meant treat.

Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

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

Thanks, this looks reasonable to me, one question about uhh more "exotic" code patterns.

@Urgau Urgau force-pushed the non_local_defs-impl-mod-transparent branch 2 times, most recently from 0dda46d to 6ae4697 Compare November 1, 2024 12:30
Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

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

Thanks, this LGTM, feel free to r=me after PR CI is green.

@jieyouxu
Copy link
Member

jieyouxu commented Nov 1, 2024

Nominating for beta-backport: this PR fixes non_local_definitions lint to not mistakenly fire on a code pattern such as:

pub mod auth {
    const _: () = {
        pub enum ArbitraryContext {}

        const _: () = {
            impl ArbitraryContext {}
        };
    };
}

where the anon-const was nested inside modules but to the impl, we only considered it to be transparent to the anon-const but not the module. This PR fixes that by considering impl definition parent as transparent with respect to modules. This was reported as a regression in #132427.

(When I say "anon-const" I mean a const with an anonymous name const _, not the anon const in types)

@rustbot label +beta-nominated

@rustbot rustbot added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Nov 1, 2024
@Urgau Urgau force-pushed the non_local_defs-impl-mod-transparent branch from 6ae4697 to 54c46c0 Compare November 1, 2024 14:39
@Urgau Urgau force-pushed the non_local_defs-impl-mod-transparent branch from 54c46c0 to 37db365 Compare November 1, 2024 15:07
@Urgau
Copy link
Member Author

Urgau commented Nov 1, 2024

@bors r=jieyouxu

@bors
Copy link
Contributor

bors commented Nov 1, 2024

📌 Commit 37db365 has been approved by jieyouxu

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 1, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 1, 2024
…sparent, r=jieyouxu

Also treat `impl` definition parent as transparent regarding modules

This PR changes the `non_local_definitions` lint logic to also consider `impl` definition parent as transparent regarding modules.

See tests and explanation in the changes.

`@rustbot` label +L-non_local_definitions
Fixes *(after beta-backport)* rust-lang#132427
cc `@leighmcculloch`
r? `@jieyouxu`
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 2, 2024
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#131037 (Move versioned Apple LLVM targets from `rustc_target` to `rustc_codegen_ssa`)
 - rust-lang#132147 (Tweak E0277 output when a candidate is available)
 - rust-lang#132398 (Add a couple of intra-doc links to str)
 - rust-lang#132453 (Also treat `impl` definition parent as transparent regarding modules)
 - rust-lang#132457 (Remove needless #![feature(asm_experimental_arch)] from loongarch64 inline assembly test)
 - rust-lang#132465 (refactor(config): remove FIXME statement in comment of `omit-git-hash`)
 - rust-lang#132471 (Add a bunch of mailmap entries)

r? `@ghost`
`@rustbot` modify labels: rollup
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 2, 2024
…sparent, r=jieyouxu

Also treat `impl` definition parent as transparent regarding modules

This PR changes the `non_local_definitions` lint logic to also consider `impl` definition parent as transparent regarding modules.

See tests and explanation in the changes.

``@rustbot`` label +L-non_local_definitions
Fixes *(after beta-backport)* rust-lang#132427
cc ``@leighmcculloch``
r? ``@jieyouxu``
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 2, 2024
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#125579 (Add `--print host-tuple` to print host target tuple)
 - rust-lang#132398 (Add a couple of intra-doc links to str)
 - rust-lang#132453 (Also treat `impl` definition parent as transparent regarding modules)
 - rust-lang#132457 (Remove needless #![feature(asm_experimental_arch)] from loongarch64 inline assembly test)
 - rust-lang#132465 (refactor(config): remove FIXME statement in comment of `omit-git-hash`)
 - rust-lang#132471 (Add a bunch of mailmap entries)

Failed merges:

 - rust-lang#131037 (Move versioned Apple LLVM targets from `rustc_target` to `rustc_codegen_ssa`)

r? `@ghost`
`@rustbot` modify labels: rollup
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Nov 2, 2024
…sparent, r=jieyouxu

Also treat `impl` definition parent as transparent regarding modules

This PR changes the `non_local_definitions` lint logic to also consider `impl` definition parent as transparent regarding modules.

See tests and explanation in the changes.

```@rustbot``` label +L-non_local_definitions
Fixes *(after beta-backport)* rust-lang#132427
cc ```@leighmcculloch```
r? ```@jieyouxu```
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 2, 2024
…sparent, r=jieyouxu

Also treat `impl` definition parent as transparent regarding modules

This PR changes the `non_local_definitions` lint logic to also consider `impl` definition parent as transparent regarding modules.

See tests and explanation in the changes.

````@rustbot```` label +L-non_local_definitions
Fixes *(after beta-backport)* rust-lang#132427
cc ````@leighmcculloch````
r? ````@jieyouxu````
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Nov 2, 2024
…sparent, r=jieyouxu

Also treat `impl` definition parent as transparent regarding modules

This PR changes the `non_local_definitions` lint logic to also consider `impl` definition parent as transparent regarding modules.

See tests and explanation in the changes.

`````@rustbot````` label +L-non_local_definitions
Fixes *(after beta-backport)* rust-lang#132427
cc `````@leighmcculloch`````
r? `````@jieyouxu`````
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 2, 2024
…iaskrgr

Rollup of 11 pull requests

Successful merges:

 - rust-lang#131037 (Move versioned Apple LLVM targets from `rustc_target` to `rustc_codegen_ssa`)
 - rust-lang#132170 (Add a Few Codegen Tests)
 - rust-lang#132333 (rust_analyzer_helix.toml: add library/ manifest)
 - rust-lang#132398 (Add a couple of intra-doc links to str)
 - rust-lang#132411 (CI: switch dist-x86_64-musl to free runner)
 - rust-lang#132453 (Also treat `impl` definition parent as transparent regarding modules)
 - rust-lang#132457 (Remove needless #![feature(asm_experimental_arch)] from loongarch64 inline assembly test)
 - rust-lang#132465 (refactor(config): remove FIXME statement in comment of `omit-git-hash`)
 - rust-lang#132466 (Account for late-bound depth when capturing all opaque lifetimes.)
 - rust-lang#132471 (Add a bunch of mailmap entries)
 - rust-lang#132488 (Remove or fix some more `FIXME(async_closure)`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit fa69b67 into rust-lang:master Nov 2, 2024
6 checks passed
@rustbot rustbot added this to the 1.84.0 milestone Nov 2, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 2, 2024
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#131037 (Move versioned Apple LLVM targets from `rustc_target` to `rustc_codegen_ssa`)
 - rust-lang#132398 (Add a couple of intra-doc links to str)
 - rust-lang#132453 (Also treat `impl` definition parent as transparent regarding modules)
 - rust-lang#132457 (Remove needless #![feature(asm_experimental_arch)] from loongarch64 inline assembly test)
 - rust-lang#132465 (refactor(config): remove FIXME statement in comment of `omit-git-hash`)
 - rust-lang#132466 (Account for late-bound depth when capturing all opaque lifetimes.)
 - rust-lang#132471 (Add a bunch of mailmap entries)
 - rust-lang#132488 (Remove or fix some more `FIXME(async_closure)`)

r? `@ghost`
`@rustbot` modify labels: rollup
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Nov 2, 2024
Rollup merge of rust-lang#132453 - Urgau:non_local_defs-impl-mod-transparent, r=jieyouxu

Also treat `impl` definition parent as transparent regarding modules

This PR changes the `non_local_definitions` lint logic to also consider `impl` definition parent as transparent regarding modules.

See tests and explanation in the changes.

``````@rustbot`````` label +L-non_local_definitions
Fixes *(after beta-backport)* rust-lang#132427
cc ``````@leighmcculloch``````
r? ``````@jieyouxu``````
@apiraino
Copy link
Contributor

apiraino commented Nov 7, 2024

Beta backport accepted as per compiler team on Zulip. A backport PR will be authored by the release team at the end of the current development cycle.

@rustbot label +beta-accepted

@rustbot rustbot added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Nov 7, 2024
@cuviper cuviper mentioned this pull request Nov 7, 2024
@cuviper cuviper modified the milestones: 1.84.0, 1.83.0 Nov 7, 2024
@cuviper cuviper removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Nov 7, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 8, 2024
[beta] backports

- rustdoc: skip stability inheritance for some item kinds rust-lang#132481
- Avoid use imports in thread_local_inner! in static rust-lang#132101
- Also treat `impl` definition parent as transparent regarding modules rust-lang#132453
- Revert "Avoid nested replacement ranges" from rust-lang#129346. rust-lang#132587

r? cuviper
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 8, 2024
[beta] backports

- rustdoc: skip stability inheritance for some item kinds rust-lang#132481
- Avoid use imports in thread_local_inner! in static rust-lang#132101
- Also treat `impl` definition parent as transparent regarding modules rust-lang#132453
- Revert "Avoid nested replacement ranges" from rust-lang#129346. rust-lang#132587

r? cuviper
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. L-non_local_definitions Lint: non_local_definitions S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants