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

Remove label/lifetime shadowing warnings #96296

Merged
merged 9 commits into from
Jun 3, 2022

Conversation

cjgillot
Copy link
Contributor

@cjgillot cjgillot commented Apr 21, 2022

This PR removes some pre-1.0 shadowing warnings for labels and lifetimes.

The current behaviour of the compiler is to warn

  • labels that shadow unrelated labels in the same function --> removed
'a: loop {}
'a: loop {} // STOP WARNING
  • labels that shadow enclosing labels --> kept, but only if shadowing is hygienic
'a: loop {
  'a: loop {} // KEEP WARNING
}
  • labels that shadow lifetime --> removed
fn foo<'a>() {
  'a: loop {} // STOP WARNING
}
  • lifetimes that shadow labels --> removed
'a: loop {
  let b = Box::new(|x: &i8| *x) as Box<dyn for <'a> Fn(&'a i8) -> i8>; // STOP WARNING
}
  • lifetimes that shadow lifetimes --> kept
fn foo<'a>() {
  let b = Box::new(|x: &i8| *x) as Box<dyn for <'a> Fn(&'a i8) -> i8>; // KEEP WARNING
}

Closes #31745.


From @petrochenkov in #95781 (comment)

I think we should remove these silly checks entirely.
They were introduced long time ago in case some new language features appear and require this space.
Now we have another mechanism for such language changes - editions, and if "lifetimes in expressions" or something like that needs to be introduced it could be introduced as an edition change.
However, there was no plans to introduce anything like for years, so it's unlikely that even the edition mechanism will be necessary.

r? rust-lang/lang

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 21, 2022
@rust-highfive
Copy link
Collaborator

r? @estebank

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 21, 2022
@petrochenkov
Copy link
Contributor

r? @petrochenkov

@petrochenkov
Copy link
Contributor

As I see shadowing outer lifetimes/labels with inner lifetimes/labels is still prohibited in this PR.
This is also a mismatch with regular names, where shadowing is allowed, like type parameters (correspond to lifetimes) and local variables (correspond to labels), and I think these errors can also be removed, but it's a separate question.

@petrochenkov
Copy link
Contributor

It would be better if this PR only removed the warnings from compiler/rustc_resolve/src/late/lifetimes.rs, without moving them to AST first.

@petrochenkov
Copy link
Contributor

@rust-lang/lang
This PR removes warnings for collisions between unrelated labels in the same function

fn check<'a>() {
    'a: loop {}
    'a: loop {} // warning: label name `'a` shadows a label name that is already in scope
}

and between lifetimes and labels

fn check<'a>() {
    'a: loop {} // warning: label name `'a` shadows a lifetime name that is already in scope
}

These warnings were introduced as a part of pre-1.0 future-proofing in #24162.
With introduction of editions, and with no plans in sight to introduce features that would benefit from this future proofing, such warnings no longer make sense, especially given that they have hygiene issues that were never addressed (#24278).

Lifetimes and labels are syntactically different from regular paths, but from name resolution point of view they are equivalent to type parameters and local variables (they live in two separate namespaces, type/lifetime and value/label correspondingly, and have same hygiene rules).
So it would be reasonable for them to have same restrictions on shadowing and other name collisions, that means no restrictions.

@petrochenkov petrochenkov added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). T-lang Relevant to the language team, which will review and decide on the PR/issue. I-lang-nominated Nominated for discussion during a lang team meeting. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 22, 2022
@cjgillot cjgillot force-pushed the remove-label-lt-shadow branch 2 times, most recently from 40a5e62 to f1848df Compare April 23, 2022 10:07
@joshtriplett
Copy link
Member

I agree. Labels shouldn't be in scope outside the block they label, since we don't have goto. If we ever need a feature that would require this, we can introduce it using an edition.

@bors

This comment was marked as resolved.

@joshtriplett joshtriplett removed the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 30, 2022
@joshtriplett
Copy link
Member

joshtriplett commented Apr 30, 2022

T-lang, do we have consensus to make label scopes limited to their block, and eliminate spurious shadowing warnings?

@rfcbot merge

@rfcbot
Copy link

rfcbot commented Apr 30, 2022

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Apr 30, 2022
@joshtriplett joshtriplett added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 30, 2022
@scottmcm
Copy link
Member

scottmcm commented May 1, 2022

This PR removes warnings for collisions between unrelated labels in the same function

Can you clarify what "unrelated" means here?

I'm 💯% in agreement with removing warnings like this one:

    'a: loop { break };
    'a: loop { break }; // warning: label name `'a` shadows a label name that is already in scope

Because there's no shadowing going on there at all, since this is an error:

2 |     'a: loop { break };
3 |     loop { break 'a };
  |                  ^^ undeclared label `'a`

But because hygiene hasn't happened for labels, it seems to me like it would still be good to have a warning for a situation like this

macro_rules! foo {
    ($e:expr) => {
        'a: loop {
            break 'a $e;
        }  
    };
}

'a: loop { 
    foo!(break 'a);

where it does something different from what would happen with bindings.

@petrochenkov
Copy link
Contributor

@scottmcm

Can you clarify what "unrelated" means here?

Given two label definitions, if none of them refers to another when resolved as use.

'a: loop {}
'a: loop {} // unrelated
'a: loop {
    'a: loop {} // shadowing
}

The shadowing case restrictions should also be removed, just maybe in a separate PR.
Label resolution is otherwise equivalent to local variable resolution, and they do not restrict shadowing.

But because hygiene hasn't happened for labels

Labels are hygienic, they are resolved identically to local variables.
Only this specific warning didn't consider hygiene, because it was implemented separately from the main label resolution pass.

@cjgillot

This comment was marked as outdated.

@petrochenkov

This comment was marked as outdated.

@cjgillot cjgillot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 2, 2022
@petrochenkov
Copy link
Contributor

r=me after squashing commits.
@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 2, 2022
@cjgillot
Copy link
Contributor Author

cjgillot commented Jun 3, 2022

@bors r=petrochenkov

@bors
Copy link
Contributor

bors commented Jun 3, 2022

📌 Commit 2aa9c70 has been approved by petrochenkov

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 3, 2022
@bors
Copy link
Contributor

bors commented Jun 3, 2022

⌛ Testing commit 2aa9c70 with merge 3a90bed...

@bors
Copy link
Contributor

bors commented Jun 3, 2022

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing 3a90bed to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 3, 2022
@bors bors merged commit 3a90bed into rust-lang:master Jun 3, 2022
@rustbot rustbot added this to the 1.63.0 milestone Jun 3, 2022
@cjgillot cjgillot deleted the remove-label-lt-shadow branch June 3, 2022 10:04
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (3a90bed): comparison url.

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results
  • Primary benchmarks: 😿 relevant regression found
  • Secondary benchmarks: no relevant changes found
mean1 max count2
Regressions 😿
(primary)
2.3% 2.3% 1
Regressions 😿
(secondary)
N/A N/A 0
Improvements 🎉
(primary)
N/A N/A 0
Improvements 🎉
(secondary)
N/A N/A 0
All 😿🎉 (primary) 2.3% 2.3% 1

Cycles

Results
  • Primary benchmarks: 🎉 relevant improvement found
  • Secondary benchmarks: 😿 relevant regression found
mean1 max count2
Regressions 😿
(primary)
N/A N/A 0
Regressions 😿
(secondary)
4.7% 4.7% 1
Improvements 🎉
(primary)
-2.2% -2.2% 1
Improvements 🎉
(secondary)
N/A N/A 0
All 😿🎉 (primary) -2.2% -2.2% 1

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Footnotes

  1. the arithmetic mean of the percent change 2

  2. number of relevant changes 2

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Jun 3, 2022
…bank

Compute `is_late_bound_map` query separately from lifetime resolution

This query is actually very simple, and is only useful for functions and method.  It can be computed directly by fetching the HIR, with no need to embed it within the lifetime resolution visitor.

Based on rust-lang#96296
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Jun 3, 2022
…bank

Compute `is_late_bound_map` query separately from lifetime resolution

This query is actually very simple, and is only useful for functions and method.  It can be computed directly by fetching the HIR, with no need to embed it within the lifetime resolution visitor.

Based on rust-lang#96296
ehuss added a commit to ehuss/reference that referenced this pull request Jun 3, 2022
flip1995 pushed a commit to flip1995/rust-clippy that referenced this pull request Jun 4, 2022
Compute `is_late_bound_map` query separately from lifetime resolution

This query is actually very simple, and is only useful for functions and method.  It can be computed directly by fetching the HIR, with no need to embed it within the lifetime resolution visitor.

Based on rust-lang/rust#96296
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Jun 9, 2022
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Oct 10, 2022
Pkgsrc changes:
 * Adjust patches as needed & checksum updates.

Upstream changes:

Version 1.63.0 (2022-08-11)
==========================

Language
--------
- [Remove migrate borrowck mode for pre-NLL errors.][95565]
- [Modify MIR building to drop repeat expressions with length zero.][95953]
- [Remove label/lifetime shadowing warnings.][96296]
- [Allow explicit generic arguments in the presence of `impl Trait` args.]
  [96868]
- [Make `cenum_impl_drop_cast` warnings deny-by-default.][97652]
- [Prevent unwinding when `-C panic=abort` is used regardless of
  declared ABI.][96959]
- [lub: don't bail out due to empty binders.][97867]

Compiler
--------
- [Stabilize the `bundle` native library modifier,][95818] also removing the
  deprecated `static-nobundle` linking kind.
- [Add Apple WatchOS compile targets\*.][95243]
- [Add a Windows application manifest to rustc-main.][96737]

\* Refer to Rust's [platform support page][platform-support-doc] for more
   information on Rust's tiered platform support.

Libraries
---------
- [Implement `Copy`, `Clone`, `PartialEq` and `Eq` for
  `core::fmt::Alignment`.][94530]
- [Extend `ptr::null` and `null_mut` to all thin (including extern)
  types.][94954]
- [`impl Read and Write for VecDeque<u8>`.][95632]
- [STD support for the Nintendo 3DS.][95897]
- [Make write/print macros eagerly drop temporaries.][96455]
- [Implement internal traits that enable `[OsStr]::join`.][96881]
- [Implement `Hash` for `core::alloc::Layout`.][97034]
- [Add capacity documentation for `OsString`.][97202]
- [Put a bound on collection misbehavior.][97316]
- [Make `std::mem::needs_drop` accept `?Sized`.][97675]
- [`impl Termination for Infallible` and then make the `Result` impls
  of `Termination` more generic.][97803]
- [Document Rust's stance on `/proc/self/mem`.][97837]

Stabilized APIs
---------------

- [`array::from_fn`]
- [`Box::into_pin`]
- [`BinaryHeap::try_reserve`]
- [`BinaryHeap::try_reserve_exact`]
- [`OsString::try_reserve`]
- [`OsString::try_reserve_exact`]
- [`PathBuf::try_reserve`]
- [`PathBuf::try_reserve_exact`]
- [`Path::try_exists`]
- [`Ref::filter_map`]
- [`RefMut::filter_map`]
- [`NonNull::<[T]>::len`][`NonNull::<slice>::len`]
- [`ToOwned::clone_into`]
- [`Ipv6Addr::to_ipv4_mapped`]
- [`unix::io::AsFd`]
- [`unix::io::BorrowedFd<'fd>`]
- [`unix::io::OwnedFd`]
- [`windows::io::AsHandle`]
- [`windows::io::BorrowedHandle<'handle>`]
- [`windows::io::OwnedHandle`]
- [`windows::io::HandleOrInvalid`]
- [`windows::io::HandleOrNull`]
- [`windows::io::InvalidHandleError`]
- [`windows::io::NullHandleError`]
- [`windows::io::AsSocket`]
- [`windows::io::BorrowedSocket<'handle>`]
- [`windows::io::OwnedSocket`]
- [`thread::scope`]
- [`thread::Scope`]
- [`thread::ScopedJoinHandle`]

These APIs are now usable in const contexts:

- [`array::from_ref`]
- [`slice::from_ref`]
- [`intrinsics::copy`]
- [`intrinsics::copy_nonoverlapping`]
- [`<*const T>::copy_to`]
- [`<*const T>::copy_to_nonoverlapping`]
- [`<*mut T>::copy_to`]
- [`<*mut T>::copy_to_nonoverlapping`]
- [`<*mut T>::copy_from`]
- [`<*mut T>::copy_from_nonoverlapping`]
- [`str::from_utf8`]
- [`Utf8Error::error_len`]
- [`Utf8Error::valid_up_to`]
- [`Condvar::new`]
- [`Mutex::new`]
- [`RwLock::new`]

Cargo
-----
- [Stabilize the `--config path` command-line argument.][cargo/10755]
- [Expose rust-version in the environment as
  `CARGO_PKG_RUST_VERSION`.][cargo/10713]

Compatibility Notes
-------------------

- [`#[link]` attributes are now checked more strictly,][96885]
  which may introduce errors for invalid attribute arguments that
  were previously ignored.

Internal Changes
----------------

These changes provide no direct user facing benefits, but represent
significant improvements to the internals and overall performance
of rustc and related tools.

- [Prepare Rust for LLVM opaque pointers.][94214]

[94214]: rust-lang/rust#94214
[94530]: rust-lang/rust#94530
[94954]: rust-lang/rust#94954
[95243]: rust-lang/rust#95243
[95565]: rust-lang/rust#95565
[95632]: rust-lang/rust#95632
[95818]: rust-lang/rust#95818
[95897]: rust-lang/rust#95897
[95953]: rust-lang/rust#95953
[96296]: rust-lang/rust#96296
[96455]: rust-lang/rust#96455
[96737]: rust-lang/rust#96737
[96868]: rust-lang/rust#96868
[96881]: rust-lang/rust#96881
[96885]: rust-lang/rust#96885
[96959]: rust-lang/rust#96959
[97034]: rust-lang/rust#97034
[97202]: rust-lang/rust#97202
[97316]: rust-lang/rust#97316
[97652]: rust-lang/rust#97652
[97675]: rust-lang/rust#97675
[97803]: rust-lang/rust#97803
[97837]: rust-lang/rust#97837
[97867]: rust-lang/rust#97867
[cargo/10713]: rust-lang/cargo#10713
[cargo/10755]: rust-lang/cargo#10755

[`array::from_fn`]: https://doc.rust-lang.org/stable/std/array/fn.from_fn.html
[`Box::into_pin`]: https://doc.rust-lang.org/stable/std/boxed/struct.Box.html#method.into_pin
[`BinaryHeap::try_reserve_exact`]: https://doc.rust-lang.org/stable/alloc/collections/binary_heap/struct.BinaryHeap.html#method.try_reserve_exact
[`BinaryHeap::try_reserve`]: https://doc.rust-lang.org/stable/std/collections/struct.BinaryHeap.html#method.try_reserve
[`OsString::try_reserve`]: https://doc.rust-lang.org/stable/std/ffi/struct.OsString.html#method.try_reserve
[`OsString::try_reserve_exact`]: https://doc.rust-lang.org/stable/std/ffi/struct.OsString.html#method.try_reserve_exact
[`PathBuf::try_reserve`]: https://doc.rust-lang.org/stable/std/path/struct.PathBuf.html#method.try_reserve
[`PathBuf::try_reserve_exact`]: https://doc.rust-lang.org/stable/std/path/struct.PathBuf.html#method.try_reserve_exact
[`Path::try_exists`]: https://doc.rust-lang.org/stable/std/path/struct.Path.html#method.try_exists
[`Ref::filter_map`]: https://doc.rust-lang.org/stable/std/cell/struct.Ref.html#method.filter_map
[`RefMut::filter_map`]: https://doc.rust-lang.org/stable/std/cell/struct.RefMut.html#method.filter_map
[`NonNull::<slice>::len`]: https://doc.rust-lang.org/stable/std/ptr/struct.NonNull.html#method.len
[`ToOwned::clone_into`]: https://doc.rust-lang.org/stable/std/borrow/trait.ToOwned.html#method.clone_into
[`Ipv6Addr::to_ipv4_mapped`]: https://doc.rust-lang.org/stable/std/net/struct.Ipv6Addr.html#method.to_ipv4_mapped
[`unix::io::AsFd`]: https://doc.rust-lang.org/stable/std/os/unix/io/trait.AsFd.html
[`unix::io::BorrowedFd<'fd>`]: https://doc.rust-lang.org/stable/std/os/unix/io/struct.BorrowedFd.html
[`unix::io::OwnedFd`]: https://doc.rust-lang.org/stable/std/os/unix/io/struct.OwnedFd.html
[`windows::io::AsHandle`]: https://doc.rust-lang.org/stable/std/os/windows/io/trait.AsHandle.html
[`windows::io::BorrowedHandle<'handle>`]: https://doc.rust-lang.org/stable/std/os/windows/io/struct.BorrowedHandle.html
[`windows::io::OwnedHandle`]: https://doc.rust-lang.org/stable/std/os/windows/io/struct.OwnedHandle.html
[`windows::io::HandleOrInvalid`]: https://doc.rust-lang.org/stable/std/os/windows/io/struct.HandleOrInvalid.html
[`windows::io::HandleOrNull`]: https://doc.rust-lang.org/stable/std/os/windows/io/struct.HandleOrNull.html
[`windows::io::InvalidHandleError`]: https://doc.rust-lang.org/stable/std/os/windows/io/struct.InvalidHandleError.html
[`windows::io::NullHandleError`]: https://doc.rust-lang.org/stable/std/os/windows/io/struct.NullHandleError.html
[`windows::io::AsSocket`]: https://doc.rust-lang.org/stable/std/os/windows/io/trait.AsSocket.html
[`windows::io::BorrowedSocket<'handle>`]: https://doc.rust-lang.org/stable/std/os/windows/io/struct.BorrowedSocket.html
[`windows::io::OwnedSocket`]: https://doc.rust-lang.org/stable/std/os/windows/io/struct.OwnedSocket.html
[`thread::scope`]: https://doc.rust-lang.org/stable/std/thread/fn.scope.html
[`thread::Scope`]: https://doc.rust-lang.org/stable/std/thread/struct.Scope.html
[`thread::ScopedJoinHandle`]: https://doc.rust-lang.org/stable/std/thread/struct.ScopedJoinHandle.html

[`array::from_ref`]: https://doc.rust-lang.org/stable/std/array/fn.from_ref.html
[`slice::from_ref`]: https://doc.rust-lang.org/stable/std/slice/fn.from_ref.html
[`intrinsics::copy`]: https://doc.rust-lang.org/stable/std/intrinsics/fn.copy.html
[`intrinsics::copy_nonoverlapping`]: https://doc.rust-lang.org/stable/std/intrinsics/fn.copy_nonoverlapping.html
[`<*const T>::copy_to`]: https://doc.rust-lang.org/stable/std/primitive.pointer.html#method.copy_to
[`<*const T>::copy_to_nonoverlapping`]: https://doc.rust-lang.org/stable/std/primitive.pointer.html#method.copy_to_nonoverlapping
[`<*mut T>::copy_to`]: https://doc.rust-lang.org/stable/std/primitive.pointer.html#method.copy_to-1
[`<*mut T>::copy_to_nonoverlapping`]: https://doc.rust-lang.org/stable/std/primitive.pointer.html#method.copy_to_nonoverlapping-1
[`<*mut T>::copy_from`]: https://doc.rust-lang.org/stable/std/primitive.pointer.html#method.copy_from
[`<*mut T>::copy_from_nonoverlapping`]: https://doc.rust-lang.org/stable/std/primitive.pointer.html#method.copy_from_nonoverlapping
[`str::from_utf8`]: https://doc.rust-lang.org/stable/std/str/fn.from_utf8.html
[`Utf8Error::error_len`]: https://doc.rust-lang.org/stable/std/str/struct.Utf8Error.html#method.error_len
[`Utf8Error::valid_up_to`]: https://doc.rust-lang.org/stable/std/str/struct.Utf8Error.html#method.valid_up_to
[`Condvar::new`]: https://doc.rust-lang.org/stable/std/sync/struct.Condvar.html#method.new
[`Mutex::new`]: https://doc.rust-lang.org/stable/std/sync/struct.Mutex.html#method.new
[`RwLock::new`]: https://doc.rust-lang.org/stable/std/sync/struct.RwLock.html#method.new
jongiddy added a commit to jongiddy/typle that referenced this pull request Jan 29, 2024
The expansion creates a labelled loop with the same label as its
enclosing block. This causes a warning during compilation that cannot be
suppressed. Case 2 from rust-lang/rust#96296
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. merged-by-bors This PR was explicitly merged by bors. 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. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

there's no #[allow(shadowed_label_names)]