Github [Edition vNext] Consider deprecating weird nesting of items · Issue #6551...

pnkfelix commented on Oct 17, 2019

(Note it can be nice, however, to be able to define types locally within functions and then attach impls to those types...)

Xanewok commented on Oct 17, 2019

If we decide to spend time investigating this, it'd be great to also consider IDE support case.

IIRC (and somewhat oversimplifying) we can introduce impl blocks (and trait impls) in nested scoped that affect the outer scope, which means that the local information is not enough to reason about associated consts/methods of a given type (to simplify this somehow) and we potentially need to scan the entire crate to do that.

@matklad I think this is related to "quick parse" that we talked about which could also skip parsing the entire function bodies, right?

This approach, if feasible at all and if it doesn't reasonably reduce expressiveness, could potentially speed up some name/type resolution.

memoryruins commented on Oct 19, 2019

Though at the same time, it's nice to allow all sorts of things for how ?eval bots work on Discord and IRC

It’s also nice in cases like evcxr, which provides a repl and jupyter notebook support for rust. plotters uses evcxr in some of its examples https://plotters-rs.github.io/plotters-doc-data/evcxr-jupyter-integration.html

matklad commented on Oct 19, 2019

unfortunate for incremental

It’s bad for laziness, not for incremental.

The property we want here for IDE is:

Function body can be type-checked without looking at other functions’ bodies

That is, it’s not the nesting itself that creates troubles, but the fact that nested items are sometimes observable from the outside. Here’s the list of things that cause trouble that I know of:

• local inherent impls
• local impls of global traits for global types
• local macro_export ed macros

The firsts two can be fixed by treating item bodies as downstream crates from coherence pov.

Additionally, another trouble some feature for ide is

• local mod declarations (mod foo;, with semicolon)

IDE gets requests like “what’s the definition of thing at offset 92 in file foo.rs”. The first thing IDE needs to do is to figure out, from the set of known crate roots, how foo.rs fits into the module system. At the moment, to do this precisely one has to look into the body of every function, precisely because a mod declaration might be hidden there. Note that we currently require an explicit path attribute for local module, but that doesn’t really help, as you need still need to parse everything to find out there isn’t a module with an attr.

matklad commented on Oct 19, 2019

Note also that we would still need to parse some function bodies due to autotrait leakage via impl trait, but only if you directly use that function. This is different from having to parse all function bodies because somewhere and odd impl might be hidden, which affects type inference.

memoryruins commented on Oct 19, 2019 •

Beyond function bodies, it's currently permitted inside const bodies, including const generics.
For a hopefully unrealistic example:

#![feature(const_generics)]

struct Foo<const T: usize>;
struct Bar;

const C: usize = {
    impl Bar { const fn a() -> usize { 42 } };
    Bar::a() + Bar::b()
};

impl Foo<{
    mod m { impl super::Bar { pub const fn b() -> usize { 42 } } }
    C
}> {}

imo being able to impl a trait on a struct defined in the same scope is a think i would want to keep. my main example for it would be a serde use case

#[derive(Serialize, Debug, Eq, PartialEq, Clone)]
#[serde(untagged)]
pub enum StringOrNumber {
    String(String),
    Number(u64),
}

impl<'de> de::Deserialize<'de> for StringOrNumber {
    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
    where
        D: de::Deserializer<'de>,
    {
        struct Visitor;

        impl<'de> de::Visitor<'de> for Visitor {
            type Value = StringOrNumber;

            /** .... further Visitor impl here **/
        }

        deserializer.deserialize_any(Visitor)
    }
}

it allows one to implement the visitor for a struct, without exposing it to the scope around it.

I think we should keep the ability to do this.

matklad commented on Apr 16, 2020 •

Given that there are already four comments saying "we should allow impls for types that are themselves nested", I feel it would be useful to repeat:

1. Obviously yes
2. The precise rule we want is "function body acts as a downstream crate from coherence point of view for trait impls; for inherent impls, only private visibility (scoped to block/function) is allowed".

EDIT: I think I first heard the coherence formulation from @vlad20012.

dtolnay commented on May 27, 2020

To give an example on the case of trait impls:

// very useful, not problematic for lazy compilation. keep this
pub fn f() {
    struct S;
    impl Trait for S {}
}

// bad, also not usually useful. do not keep
pub struct S;
pub fn f() {
    impl Trait for S {}
}

matklad commented on May 28, 2020

Another slightly problematic thing (again by vlad20012 ) are inner function attributes:

fn foo() {
    #![cfg(unix)]
}

fn main() {
  foo()
}

Strictly speaking, we can live with that, as it only requires bounded parsing of the prefix of the function. But not having that seems better.

cramertj commented on Jan 5

@dtolnay

// bad, also not usually useful. do not keep
pub struct S;
pub fn f() {
    impl Trait for S {}
}

There may be an exception when the trait is defined in f? This feels related to coherence to me.

pub struct S;
pub fn f() {
    trait Trait {}
    impl Trait for S {}
}

nikomatsakis commented on Feb 8

Nominating for the @rust-lang/lang meeting -- if we want these to happen, we're going to have to pay attention.

cramertj commented on Feb 8

It's probably also worth pointing out that impl Trait creates a minor hole in the "type-check function body without looking at impls of other functions" approach due to handling of auto traits. This probably isn't hugely impactful from a practical perspective, but it's more than slightly annoying that you'd have to parse the bodies of -> impl Trait functions in order to correctly check Send/Sync/Unpin bounds.

danielhenrymantilla commented on Feb 9 •

FWIW, there are also people (myself included), that try to reduce the number of "meaningful top-level items" or to have "deeply tied code sit next to each other".

An example of the former would be:

impl DerefMut for SomeType {
    fn deref_mut … -> &mut SomePointee {
        impl Deref for SomeType { … }
        …
    }
}

impl Ord for SomeType {
    fn cmp … {
        impl PartialOrd for SomeType {
            … Some(self.cmp(other)) …
        }
        …
    }
}

// And on nightly
impl Fn<(…,)> for SomeType {
    fn call (&self, …) -> Ret {
        impl FnMut…
        impl FnOnce…

        …
    }
}

An example of the latter would be:

struct WrapsAPtr(*const Pointee);

impl WrapsAPtr {
    pub fn new(…) -> Self {
        let pointee = …;
        let ptr = Box::into_raw(Box::new(pointee));
        impl Drop for WrapsAPtr { fn drop(&mut self) { unsafe {
             let Self(ptr) = *self;
             drop::<Box<Pointee>>(Box::from_raw(ptr as _));
        }}}
        WrapsAPtr(ptr)
    }
}

• This way witching from Box to Arc is locally contained within new().

    pub fn new(…) -> Self {
        let pointee = …;
  -     let ptr = Box::into_raw(Box::new(pointee));
  +     let ptr = Arc::into_raw(Arc::new(pointee));
        impl Drop for WrapsAPtr { fn drop(&mut self) { unsafe {
             let Self(ptr) = *self;
  -          drop::<Box<Pointee>>(Box::from_raw(ptr as _));
  +          drop::<Arc<Pointee>>(Arc::from_raw(ptr as _));
        }}}
        WrapsAPtr(ptr)
    }

In that regarding, losing the ability to do either of these things would be a big deal breaker w.r.t. upgrading the edition I use, and I'm afraid coherence would prevent both of these.

Another nice thing to do is to avoid polluting the top-level namespace when dealing with some impls, such as:

const _: () = {
    use ::core::fmt::{self, Debug, Display};

    impl Display for MyType { … }
};

Finally, regarding inline modules inside an fn body, that's also a desireable feature, not only for eval queries with the Playground, but also for macros that may mock types using modules: Example with a macro implementing type-level enums.

On the other hand, I have never encountered the need for a non-inline module defined within a function; although one can always emulate that with an inline module by using include!, I guess.

pnkfelix commented 5 days ago

We discussed this issue at a recent lang design team meeting (agenda item).

Here is my attempt to summarize the discussion as posted on this issue, as well as convey concerns raised during the lang team meeting itself.

• The original proposal in the issue description makes it sound like the goal would be to remove the nesting of these items entirely.
• There are cases where supporting nesting is valuable. As an example, it is used right now as the basis for generating doc tests in rustdoc, where you need in some cases to have item declarations sitting alongside Rust expressions, and that is something that you can only do within an fn body.
• Subsequent comments here have pointed out that completely removing support for nesting is not necessarily the right goal. For example, matklad points out that just restricting the visibility of such nested items could have a lot of benefit for lazily-evaluating fragments of Rust programs (which is useful for IDEs).

The basic consensus of the lang design team was that we are not prepared to address this for the 2021 edition.

We might be willing to consider a soft-deprecation of the feature in the future, that would not need to be tied to an edition, but that can be a separate proposal (that would presumably be written in a manner that takes the dialogue here into account).

(We didn't actually directly address the more narrow proposal put forth by matklad in the lang team meeting. But I think the same principles hold there: We are not prepared to address that for the 2021 edition. We would be willing to consider strategies for revising/restricting the semantics that are not tied to an edition.)

So, the lang design team recommends that this issue be closed.

@rfcbot fcp close

rfcbot commented 5 days ago •

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

No concerns currently listed.

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.

cramertj commented 5 days ago

To clarify: my position is that this is a change we should be considering, but not for the 2021 edition. There is too much remaining design and implementation work for this to ship in that constrained of a time period.

rfcbot commented 5 days ago

This is now entering its final comment period, as per the review above.

scottmcm commented 5 days ago •

Totally agreed for 2021. What's the right way to track it for 2024? The tag here is "202x"; should we have a "edition-the-next-one" tag? (Do we need an FCP to move out of an edition? Could we just say "nope" and change the label?)

bstrie commented 5 days ago •

It would be generally useful to have an A-edition-wishlist label, to make it easier to gather up ideas for edition-related changes at the beginning of each edition cycle. Agreed that this issue ought to be postponed, rather than closed.