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

shadowing function declarations with variables shouldn't be allowed #22685

Open
zeozeozeo opened this issue Oct 28, 2024 · 10 comments
Open

shadowing function declarations with variables shouldn't be allowed #22685

zeozeozeo opened this issue Oct 28, 2024 · 10 comments

Comments

@zeozeozeo
Copy link
Contributor

zeozeozeo commented Oct 28, 2024

V doctor:

V full version: V 0.4.8 5c65e58.7bd2bef
OS: windows, Microsoft Windows 10 Pro v19045 64-bit
Processor: 10 cpus, 64bit, little endian, 

getwd: C:\Users\user\Downloads
vexe: C:\Users\user\Desktop\Code\v\v.exe
vexe mtime: 2024-10-28 18:11:20

vroot: OK, value: C:\Users\user\Desktop\Code\v
VMODULES: OK, value: C:\Users\user\.vmodules
VTMP: OK, value: C:\Users\user\AppData\Local\Temp\v_0

Git version: git version 2.42.0.windows.1
Git vroot status: weekly.2024.43-56-g7bd2bef1
.git/config present: true

CC version: Error: 'cc' is not recognized as an internal or external command,

operable program or batch file.


thirdparty/tcc status: thirdparty-windows-amd64 b425ac82

What did you do?
./v -g -o vdbg cmd/v && ./vdbg test.v

fn x() {}

fn main() {
	x := 5
	println(x)
}

What did you expect to see?

An error message about x being redefined

What did you see instead?

5

Huly®: V_0.6-21134

@enghitalo
Copy link
Contributor

I don't know what the best solution is, but the less ambiguity allowed, the better.

@medvednikov
Copy link
Member

V has a check specifically for x := x

That's what's going on here.

@zeozeozeo zeozeozeo changed the title cannot have a variable with the same name as a function declaration shadowing function declarations with variables shouldn't be allowed Oct 28, 2024
@yuyi98
Copy link
Member

yuyi98 commented Oct 29, 2024

There is already a handle for this, if it is used only as a variable and does not report an error, it will report an error if it is also called as a function.

fn foo1(foo1 int) {
	foo1(foo1 + 1)
}

fn foo2() {
	foo2 := 1
	foo2(foo2)
}

fn main() {
	foo1(5)
	foo2()
}

PS D:\Test\v\tt1> v run .
tt1.v:2:2: error: ambiguous call to: `foo1`, may refer to fn `foo1` or variable `foo1`
    1 | fn foo1(foo1 int) {
    2 |     foo1(foo1 + 1)
      |     ~~~~~~~~~~~~~~
    3 | }
    4 |
tt1.v:7:2: error: ambiguous call to: `foo2`, may refer to fn `foo2` or variable `foo2`
    5 | fn foo2() {
    6 |     foo2 := 1
    7 |     foo2(foo2)
      |     ~~~~~~~~~~
    8 | }
    9 |
tt1.v:7:7: error: expected 0 arguments, but got 1
    5 | fn foo2() {
    6 |     foo2 := 1
    7 |     foo2(foo2)
      |          ~~~~
    8 | }
    9 |
Details: have (int)
         want ()

@JalonSolov
Copy link
Contributor

The error message could be improved... it isn't that it is ambiguous, it is that it simply isn't allowed. Perhaps something like

tt1.v:2:2: error: no name shadowing is allowed: either fn `foo1` or variable `foo1` must be renamed

the error message could also come out for the

fn foo1(foo1 int) {

line, since that is where it first occurs.

@zeozeozeo
Copy link
Contributor Author

I think the compiler shouldn't allow shadowing the function at all, like here:

const x = 12345

fn main() {
	x := 5
	println(x)
}

produces an error like this:

test.v:4:2: error: duplicate of a const name `x`
    2 |
    3 | fn main() {
    4 |     x := 5
      |     ^
    5 |     println(x)
    6 | }

@yuyi98
Copy link
Member

yuyi98 commented Oct 29, 2024

This situation is very easy to encounter, and using only variables in functions without using function calls is not a problem.
An error is reported if a function call is used in this situation.

@zeozeozeo
Copy link
Contributor Author

This situation is very easy to encounter, and using only variables in functions without using function calls is not a problem.
An error is reported if a function call is used in this situation.

I think it is a problem from a readability standpoint, take this code for example:

fn x() {}

fn main() {
	x := 5
	println(x)
}

It is not immediately obvious if this code will print 5 or fn (), so given that V disallows shadowing completely, I think that this code should error out immediately

@Wajinn
Copy link

Wajinn commented Oct 30, 2024

Variable shadowing is explicitly not allowed. This case (below) is different. The function "x()" is outside the scope of function "main", thus arguably should be legal (as is).

fn x() 

fn main() {
	x := 5
	println(x)
}

If the user places the function with the same name of a variable in the same scope, then there will be an error. However, I do agree with JalonSolov, that the error should be more clear. Possibly, "No name shadowing within the same function or within the same parent scope."

fn x() {println("Was here!")}

fn main() {
	x := 5
	println(x)
	x() // this will rightfully produce an error
}

@zeozeozeo
Copy link
Contributor Author

The function "x()" is outside the scope of function "main", thus arguably should be legal (as is).

What about this then?

const x = 12345

fn main() {
	x := 5
	println(x)
}

This errors out even though const x is not in the same scope as x := 5. Why should functions be different?

@Wajinn
Copy link

Wajinn commented Oct 30, 2024

const x can be seen by all functions and is defined at the module level. Attempting to use a const with the same name of a variable inside of a function will rightfully cause an error.

Functions have their own separate scopes. There isn't a visible function of global scope that all other functions are within the scope of. The conflict with functions, would be them having the same name, in the same module.

In both cases, where you are correctly getting the error, you are placing something of the same name inside the scope of a function that is already using that name for a variable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants