resolve: Make self unhygienic

[petrochenkov]

Currently self behaves exactly like any other function parameter name, so hygiene rules apply to it.
This patch makes it unhygienic and therefore behaving less like an usual function parameter and more like a special entity (which it already is in many other aspects).
See #15682 and rust-lang/rfcs#1606 for the motivation.

---

Why this is not a breaking change and why making self unhygienic is not so bad from the name resolution / hygene point of view.

self in value namespace¹ can only be defined as a function-local binding in a function parameter.
(EDIT: but see the discussion about closures below):

fn f(mut self: Self, a: u8) {}
         ^^^^

As a local binding it's available only in the function it's defined in. Nested items, including methods with their own self, can't refer to it.
So, one self can never shadow another self or create ambiguity, at any point in the code the set of scopes contains either 1 self or 0.
Basically, this means that self either resolves correctly or not resolves at all, but it can't be resolved incorrectly.
In this circumstances hygiene makes much less sense than for other local bindings.

Some part of the motivation for self hygiene still stays though, for example in this function

fn f(&mut self) {
    m!();
}

macro m!() can sneakily modify contents of self without user passing it explicitly, i.e. unhygienic self gives macros more abilities to create "side effects".
In addition, if in the future self is used for something else than it is used today, the argumentation above may be invalidated.
So, this needs a decision from the lang team.

¹self in type namespace is already unhygienic and resolves to the current module.

cc @jseyfried
r? @nrc

[petrochenkov]

I've made a terrible mistake!
There's one more way to define a binding named self - lambda captures:

#[derive(Clone, Copy, Debug)]
struct S(u8);

impl S {
    fn f(mut self) {
        println!("arg {:?}", self);
        self.0 += 1;
        println!("arg {:?}", self);
        (move || {
            println!("capture level 1 {:?}", self);
            self.0 += 1;
            println!("capture level 1 {:?}", self);
            (move || {
                println!("capture level 2 {:?}", self);
                self.0 += 1;
                println!("capture level 2 {:?}", self);
                (move || { // (Need to go deeper)
                    println!("capture level 3 {:?}", self);
                    self.0 += 1;
                    // For a minute there, I lost my `self`, I lost my `seeelf`
                    println!("capture level 3 {:?}", self);
                })();
                println!("capture level 2 {:?}", self);
            })();
            println!("capture level 1 {:?}", self);
        })();
        println!("arg {:?}", self);
    }
}

fn main() {
    S(0).f();
}

--------
Prints:

arg S(0)
arg S(1)
capture level 1 S(1)
capture level 1 S(2)
capture level 2 S(2)
capture level 2 S(3)
capture level 3 S(3)
capture level 3 S(4)
capture level 2 S(3)
capture level 1 S(2)
arg S(1)

So, yes, self bindings can shadow each other and unhygienic self can lead to extremely subtle bugs in corner cases.

[jseyfried]

@petrochenkov

unhygienic self can lead to extremely subtle bugs in corner cases.

I don't think that follows from your example -- lambda captures are not bindings during name resolution, so all the selfs in your example would resolve to the single self binding from the method.

[jseyfried]

That being said, I would prefer not to land this. I like that a macro expansion can only reference the locals used in the invocation, analogous to how a function call can only access the locals in the arguments.

Also, if we landed this then it could be impossible to refactor out a closure inside a method body:

impl {
    fn f(&self) {
        m!(); // If `m`'s expansion uses `self`, we wouldn't be able to refactor it ...
        let g = |this: &Self| { // ... into this closure
            m!();
        };
        g(self);
    }
}

[petrochenkov]

@jseyfried

lambda captures are not bindings during name resolution ...

That means nested closures are already subtly buggy. Consider the next example:

macro_rules! m {
    ($arg1: ident, $arg2: ident) => {
        (move || {
            $arg1 += 1;
            println!("{:?}", $arg1);
            (move || {
                $arg2 += 1;
                println!("{:?}", $arg2);
            })();
        })();
    }
}


fn main() {
    let mut a = 0;
    let mut b = 0;
    println!("{:?}", a);
    m!(a, b);
}

Both closures are supposed to capture the passed arguments and increment them by 1, so the program should print

0
1
1

and it indeed prints that value.

Now, lets use this macro with two identical arguments:

fn main() {
    let mut a = 0;
    let mut b = 0;
    println!("{:?}", a);
    m!(a, a);
}

Whoops, now the inner closure doesn't capture a, but captures its independent copy kept in the outer closure, and the program prints

0
1
2

[petrochenkov]

That being said, I would prefer not to land this.

Well, despite opening this PR, that would be my preference as well.

[petrochenkov]

That means nested closures are already subtly buggy. Consider the next example:

Nah, it's a bad example. Even if it feels unhygienic, it's still compatible with an equivalent non-capturing example (this non-capturing example feels unhygienic too, though):

macro_rules! m {
    ($arg1: ident, $arg2: ident) => {
        {
            let mut $arg1 = $arg1;
            $arg1 += 1;
            println!("{:?}", $arg1);
            {
                // $arg1 and $arg2 share the context, so $arg2 here picks up
                // the incremented value of $arg1 and not the original argument
                let mut $arg2 = $arg2;
                $arg2 += 1;
                println!("{:?}", $arg2);
            }
        }
    }
}

fn main() {
    let mut a = 0;
    println!("{:?}", a);
    m!(a, a);
}

-------
Prints:

0
1
2

I wonder if there's another way to capture a variable, shadowed by one of its previous captures, which can be applied to self. EDIT: It seems there is none. All captures share the context with the original captured variable and it can't be subverted by macros, so more nested captures always shadow less nested ones. That makes them compatible with unhygienic self.

[nrc]

My preference is not to make breaking changes to the macro system unless there is a very strong case. It seems everyone is in agreement to close this PR for now?

[petrochenkov]

@nrc

breaking changes

Well, no one actually came up with a breaking example so far.

[jseyfried]

@petrochenkov @nrc I'm pretty sure there are no breaking examples.

Collecting information about lambda captures could be easily refactored into a post-processing pass after name resolution; that is, lambda captures are irrelevant to name resolution, hygiene, etc.

[bors]

☔ The latest upstream changes (presumably #33532) made this pull request unmergeable. Please resolve the merge conflicts.

[petrochenkov]

Name resolution has been moved to AST, so this implementation doesn't work anymore.
If you are interested in unhygienic self, feel free to reimplement and submit a PR.

[joelself]

What gives? I checked the merge request and apparently it has been merged into master. But the current file doesn't show the changes. I even cloned the repo and checked the file myself and it is unchanged. Even the history of the file doesn't show any changes 10 days ago which is when the merge supposedly happened.

So where are the changes so I can work on this?

[tinco]

I has not been merged. Something changed that caused this PR to be invalid. People in this thread generally either view this change as unnecessary or unhygienic. As Petro says if you want to have a go at implementing this you can submit a new PR with a new implementation, and see how the discussion goes then.

[joelself]

That's not at all what I was asking about.

It appears that the "Something changed that caused this PR to be invalid." merge doesn't exist in the code. hir/lowering.rs doesn't show the merge in it's history and the changes that were supposed to have been made looked like they were unmade. At least at first. After looking at it some more it seems it was refactored again and it just happens to make the code I want to change look a lot like the original code from before the merge which may or may not have happened.