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

resolve: the relative order of items and statements in a block can be important #33118

Closed
jseyfried opened this issue Apr 20, 2016 · 19 comments
Closed
Labels
A-resolve Area: Path resolution C-bug Category: This is a bug. 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

@jseyfried
Copy link
Contributor

jseyfried commented Apr 20, 2016

For example,

fn foo() {}

fn f1() {
    fn bar() {
        foo(); // This resolves to the item
    }
    let foo = 0;
}

fn f2() {
    let foo = 0;
    fn bar() {
        foo(); // This is an error (cannot capture dynamic environment in a fn item)
    }
}

Since items in a block are above the block's local variables in the scope hierarchy, it might make sense to resolve the items' bodies in scopes that descend directly from the scope of the block's items, bypassing the scopes of the local variables.

That being said, I think I prefer the current behavior. However, since it will mildly complicate name resolution in the future, I wanted to make sure that we actually want this behavior.

@jseyfried
Copy link
Contributor Author

jseyfried commented Apr 20, 2016

cc @nrc @petrochenkov @nikomatsakis

@petrochenkov
Copy link
Contributor

My intuition says that function bodies (and constant initializers, etc) should be isolated from local variables in their containing scopes.

I suppose that current behavior is targeted at people coming from languages that allow nested functions to capture their environment, so they can get proper diagnostics - "you can't do that in Rust".
This can probably be done with a lint with the same success:

fn f2() {
    let x = 0;
    fn bar() {
        x += 1; // error: unresolved name `x`
                // note: hey, maybe you wanted a closure and not a function
                // ^ the note shouldn't even be precise and may be reported on best-effort basis
    }
}

If such isolation of function bodies gives some nice algorithmic properties, I'd do that without hesitation.

@nrc
Copy link
Member

nrc commented Apr 21, 2016

I think I would prefer for inner function bodies not to 'see' the enclosing function bodies at all, i.e., f2 in the OP would not be an error. AIUI, the motivation is only that error comparing with closures, which I believe should be a lint, rather than an error (to be clear, there should be a warning lint when there are names in scope both outside the function and inside it, which today would be an error. Obvs, if there is only a name inside the function, then it must still be an error).

I believe the change is backwards compatible since it only admits more programs.

@nrc nrc added T-lang Relevant to the language team, which will review and decide on the PR/issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 21, 2016
@jseyfried
Copy link
Contributor Author

@petrochenkov @nrc ok, you've convinced me that we should allow the example in the OP with a warning lint.

@jseyfried
Copy link
Contributor Author

@nrc Do you think the relative order of macro 2.0 definitions and statements in a block should matter?
For example,

fn foo() {}
fn f1() {
    macro_rules_v2! bar { () => { foo() } }
    let foo = 0;
    bar!(); // `foo` in this expansion would resolve to the item
}
fn f2() {
    let foo = 0;
    macro_rules_v2! bar { () => { foo() } }
    bar!(); // Should `foo` in this expansion resolve to the item or the local?
}

@jseyfried
Copy link
Contributor Author

jseyfried commented Apr 22, 2016

If we decided the relative order of macro 2.0 definitions and statements should not matter, we would be able to use a macro in a statement before it was defined in the statement.

Perhaps a simpler way to state the question is "should a macro 2.0 definition in a block be analogous to a function item or to a local variable initialized with a closure?"

@nikomatsakis
Copy link
Contributor

A couple of points:

cannot capture dynamic environment in a fn item

Maybe I am unusual, but this error always confuses me every time I get it. It assumes (as @petrochenkov said) that I was trying to capture that variable in the first place, but -- because I know Rust's rules -- I never am. I would much prefer a straight-forward error with a HELP attached or something like that.

Just to clarify though, there are two distinct cases to be concerned about:

  1. References in the body of an inner fn.
  2. References in the body of the outer fn.

The example targets case 1, but I just want to be clear that consider this distinct from case 2:

fn bar() {
    let x = foo; // resolves to the inner fn `foo`
    let foo = 22;
    let y = foo; // resolves to the variable
    fn foo() { }
}

Right?

@nikomatsakis
Copy link
Contributor

In any case, I agree that -- in the original example -- the reference should resolve successfully in both cases to the foo at the module scope.

@jseyfried
Copy link
Contributor Author

@nikomatsakis

I would much prefer a straight-forward error with a HELP attached or something like that.

Agreed.

this [is] distinct from case 2 ... Right?

Right.

I think we're all pretty much in agreement on how to handle this issue.

@jseyfried
Copy link
Contributor Author

Related question -- should this compile? (currently, it doesn't)

enum E { V }

fn f<E>(_: E) {
    fn g() {
        let _ = E::V; // is the type parameter `E` in scope here? (currently, it is)
    }
}

Since the type parameter E is above the function item g in the scope hierarchy (unlike f's locals), we might want to consider E to be in the scope of g's body.

Of course, since we can't use the type parameter from g's body, we also might want to consider it to be not in scope. I don't have a strong opinion either way.

@nrc
Copy link
Member

nrc commented Apr 26, 2016

@jseyfried well, we get into hygiene questions, but assuming we're only talking about stuff visible due to the definition (i.e., default hygiene rules), then I think macros should follow the same rules as functions, i.e., they don't 'see' the function body at all.

@jseyfried
Copy link
Contributor Author

@nrc

macros should follow the same rules as functions, i.e., they don't 'see' the function body at all.

Sounds good, I agree. Note that this differs from today's macros, which can see the function body.

@arielb1
Copy link
Contributor

arielb1 commented Apr 26, 2016

@nrc

That's quite unlike Scheme macros! I think the interaction between macros and let is subtle enough and insufficiently-useful that we should not have it.

@nrc
Copy link
Member

nrc commented Apr 26, 2016

That's quite unlike Scheme macros! I think the interaction between macros and let is subtle enough and insufficiently-useful that we should not have it.

I'm not aware of exactly how Scheme macros work here. I think I am proposing that there should not be interaction between macro definitions and let statements. I'm saying that when a macro is declared inside a function body, then it should (effectively) not see the rest of the function body, in the same way that functions inside function bodies don't.

@nikomatsakis
Copy link
Contributor

We have to be careful about backwards compatibility here.
On Apr 26, 2016 3:51 PM, "Nick Cameron" notifications@github.com wrote:

That's quite unlike Scheme macros! I think the interaction between macros
and let is subtle enough and insufficiently-useful that we should not have
it.

I'm not aware of exactly how Scheme macros work here. I think I am
proposing that there should not be interaction between macro definitions
and let statements. I'm saying that when a macro is declared inside a
function body, then it should (effectively) not see the rest of the
function body, in the same way that functions inside function bodies don't.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#33118 (comment)

@jseyfried
Copy link
Contributor Author

jseyfried commented Apr 27, 2016

@nikomatsakis

We have to be careful about backwards compatibility here.

The current plan is to implement a new, backwards incompatible "macro_rules_v2" that can be used alongside the today's macro_rules (which would continue to interact with let statements).

@nikomatsakis
Copy link
Contributor

@jseyfried

The current plan is to implement a new, backwards incompatible "macro_rules_v2" that can be used alongside the today's macro_rules (which would continue to interact with let statements).

I see, I thought that we were talking about the interaction of today's macros with let statements, not "macros 2.0".

@petrochenkov
Copy link
Contributor

I wanted identifiers in syntactically unambiguous contexts to always be interpreted as fresh bindings, i.e.

const C: u8 = 10;
let C @ SUBPATTERN = 11; // OK, no error, C is interpreted as a fresh binding, because it cannot be anything else syntactically

but this issue needs to be fixed first because together they can result in some ridiculous behavior:

const C: u8 = 10; // C(0)
let C @ _ = 11; // C(1)
fn f() {
     match xxx {
         C => {} // C(2)
         _ => {}
     }
}

C(2) is interpreted as a fresh binding and not constant C(0), because it's first resolved as local C(1) and this resolution is rejected in favor of a fresh bindings.
(Note that before #34570 C(2) would be resolved to C(0) because the "disambiguation lookup" in patterns worked "unhygienically" and ignored local variables.)

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue May 26, 2021
…nkov

Disallow shadowing const parameters

This pull request fixes rust-lang#85348. Trying to shadow a `const` parameter as follows:
```rust
fn foo<const N: i32>() {
    let N @ _ = 0;
}
```
currently causes an ICE. With my changes, I get:
```
error[E0530]: let bindings cannot shadow const parameters
 --> test.rs:2:9
  |
1 | fn foo<const N: i32>() {
  |              - the const parameter `N` is defined here
2 |     let N @ _ = 0;
  |         ^ cannot be named the same as a const parameter

error: aborting due to previous error
```
This is the same error you get when trying to shadow a constant:
```rust
const N: i32 = 0;
let N @ _ = 0;
```
```
error[E0530]: let bindings cannot shadow constants
 --> src/lib.rs:3:5
  |
2 | const N: i32 = 0;
  | ----------------- the constant `N` is defined here
3 | let N @ _ = 0;
  |     ^ cannot be named the same as a constant

error: aborting due to previous error
```
The reason for disallowing shadowing in both cases is described [here](rust-lang#33118 (comment)) (the comment there only talks about constants, but the same reasoning applies to `const` parameters).
@Enselic
Copy link
Member

Enselic commented Sep 13, 2023

Triage: A PR looking into fixing this issue was closed with the motivation that the current behavior should be kept unchanged after all. Let's also close this issue.

@Enselic Enselic closed this as not planned Won't fix, can't repro, duplicate, stale Sep 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-resolve Area: Path resolution C-bug Category: This is a bug. 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

No branches or pull requests

7 participants