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

in variable initialization, dependencies within functions are not recursively resolved #1463

Open
thehowl opened this issue Dec 19, 2023 · 7 comments · May be fixed by #2077
Open

in variable initialization, dependencies within functions are not recursively resolved #1463

thehowl opened this issue Dec 19, 2023 · 7 comments · May be fixed by #2077
Assignees
Labels
🐞 bug Something isn't working in focus Core team is prioritizing this work 📦 🤖 gnovm Issues or PRs gnovm related

Comments

@thehowl
Copy link
Member

thehowl commented Dec 19, 2023

This works:

package main

func main() {

}

func hello(s string) {}

var myDep string

var cmplx = func() { hello(myDep) }

But this doesn't:

package main

func main() {

}

func hello(s string) {}

var myVar = func() { hello(myDep) }

var myDep string

// panic: main/a.gno:9: name myDep not declared

EDIT: After #1854, this is still an issue, you just need to spleet it in different files.

// a.gno
package main

func main() {
	myDep = "123"
	myVar()
}

func hello(s string) {
	println(s)
}

var myVar = func() { hello(myDep) }
// b.gno
package main

var myDep string
@thehowl thehowl added 🐞 bug Something isn't working 📦 🤖 gnovm Issues or PRs gnovm related labels Dec 19, 2023
@thehowl thehowl added this to the 🚀 main.gno.land (required) milestone Dec 19, 2023
@petar-dambovaliev petar-dambovaliev self-assigned this Mar 26, 2024
@petar-dambovaliev petar-dambovaliev moved this from Backlog to In Review in 🧙‍♂️gno.land core team Apr 3, 2024
@zivkovicmilos zivkovicmilos moved this from In Review to In Progress in 🧙‍♂️gno.land core team Apr 3, 2024
@petar-dambovaliev petar-dambovaliev moved this from In Progress to In Review in 🧙‍♂️gno.land core team Apr 3, 2024
@zivkovicmilos zivkovicmilos moved this from In Review to In Progress in 🧙‍♂️gno.land core team Apr 3, 2024
petar-dambovaliev added a commit that referenced this issue Apr 9, 2024
This PR fixes [this](#1849) and
[this](#1463).

Before preprocessing, a dependency graph is created and edges between
the nodes which represent the relationship between global `var` and
`const` declarations.
Then, a new slice of declarations is created that is topologically
sorted.
This enables the rest of the preprocessing code to work the way it is
now.

Small scale refactoring is included by removing unnecessary else
statements in `PredefineFileSet`.
@github-project-automation github-project-automation bot moved this from In Progress to Done in 🧙‍♂️gno.land core team Apr 9, 2024
@thehowl thehowl reopened this Apr 19, 2024
@petar-dambovaliev
Copy link
Contributor

petar-dambovaliev commented Apr 19, 2024

This bug exists because gnovm's "compilation" unit is a file and the Go compiler's compilation unit is a package.
If the dependency graph i wrote would be used, the ast' of all files within a package must be merged and then preprocessed.

After a conversation @jaekwon , we will wait for his evaluation of the problem and reach a conclusion if the graph would be used or not.

@thehowl
Copy link
Member Author

thehowl commented Apr 19, 2024

This bug exists because gnovm's "compilation" unit is a file and the Go compiler's compilation unit is a package.

This is not true, predefineNow is perfectly capable of resolving dependencies on names declared in other files.

@petar-dambovaliev
Copy link
Contributor

This bug exists because gnovm's "compilation" unit is a file and the Go compiler's compilation unit is a package.

This is not true, predefineNow is perfectly capable of resolving dependencies on names declared in other files.

this is not what i meant.

@petar-dambovaliev
Copy link
Contributor

This bug exists because gnovm's "compilation" unit is a file and the Go compiler's compilation unit is a package.

This is not true, predefineNow is perfectly capable of resolving dependencies on names declared in other files.

predefineNow might be able to do many things but it doesn't change the fact that the unit of processing is a file.

@thehowl
Copy link
Member Author

thehowl commented Apr 19, 2024

predefineNow might be able to do many things but it doesn't change the fact that the unit of processing is a file.

By "unit of processing is a file", what I understand is that the implementation of the language is considering the individual file rather than the package, especially when resolving dependencies for names. The obvious example of a language that does this is C, which requires you to import header files for anything else you want to import.

Go, and Gno by extension, are not like this and will support referencing names within the same package. This is a feature of both languages that exists and is implemented; albeit the Gno implementation is undoubtedly still with many more bugs.

Of course, there is one notable exception and that is that package imports are scoped within the file they are declared. For this reason, it is still important for the implementation both in Go and in Gno to keep track of individual files, as they have different imports and as such can have different sets of "global" identifiers.

The fact that the GnoVM processes the package by iterating over each file is not behaviour specific to the GnoVM, inconsistent with what there is in Go. The Go implementation still very much works on individual files deep into the compiling process. Here's an example in the type checker, here's an example in the "noder" (converter to IR)

So, no, as far as I'm concerned, the problem is not architectural as you claim, ie. us having a "file" compilation unit while Go having a "package" compilation unit.

@petar-dambovaliev
Copy link
Contributor

predefineNow might be able to do many things but it doesn't change the fact that the unit of processing is a file.

By "unit of processing is a file", what I understand is that the implementation of the language is considering the individual file rather than the package, especially when resolving dependencies for names. The obvious example of a language that does this is C, which requires you to import header files for anything else you want to import.

Go, and Gno by extension, are not like this and will support referencing names within the same package. This is a feature of both languages that exists and is implemented; albeit the Gno implementation is undoubtedly still with many more bugs.

Of course, there is one notable exception and that is that package imports are scoped within the file they are declared. For this reason, it is still important for the implementation both in Go and in Gno to keep track of individual files, as they have different imports and as such can have different sets of "global" identifiers.

The fact that the GnoVM processes the package by iterating over each file is not behaviour specific to the GnoVM, inconsistent with what there is in Go. The Go implementation still very much works on individual files deep into the compiling process. Here's an example in the type checker, here's an example in the "noder" (converter to IR)

So, no, as far as I'm concerned, the problem is not architectural as you claim, ie. us having a "file" compilation unit while Go having a "package" compilation unit.

This is unralated in 2 different ways, at least.
We are not talking about type checking or code generation and my original comment should be taken into the context of the way my PR solves the problem of the issue.
Since you opened this issue and seem to disagree with that solution, i've assigned it to you.

@petar-dambovaliev
Copy link
Contributor

petar-dambovaliev commented Apr 20, 2024

predefineNow might be able to do many things but it doesn't change the fact that the unit of processing is a file.

By "unit of processing is a file", what I understand is that the implementation of the language is considering the individual file rather than the package, especially when resolving dependencies for names. The obvious example of a language that does this is C, which requires you to import header files for anything else you want to import.

Go, and Gno by extension, are not like this and will support referencing names within the same package. This is a feature of both languages that exists and is implemented; albeit the Gno implementation is undoubtedly still with many more bugs.

Of course, there is one notable exception and that is that package imports are scoped within the file they are declared. For this reason, it is still important for the implementation both in Go and in Gno to keep track of individual files, as they have different imports and as such can have different sets of "global" identifiers.

The fact that the GnoVM processes the package by iterating over each file is not behaviour specific to the GnoVM, inconsistent with what there is in Go. The Go implementation still very much works on individual files deep into the compiling process. Here's an example in the type checker, here's an example in the "noder" (converter to IR)

So, no, as far as I'm concerned, the problem is not architectural as you claim, ie. us having a "file" compilation unit while Go having a "package" compilation unit.

https://github.com/golang/go/blob/a63907808d14679c723e566cb83acc76fc8cafc2/src/cmd/compile/internal/types2/initorder.go#L15

this is where all package level variables are collected.

	// Compute the object dependency graph and initialize
	// a priority queue with the list of graph nodes.
	pq := nodeQueue(dependencyGraph(check.objMap))

@petar-dambovaliev petar-dambovaliev linked a pull request May 13, 2024 that will close this issue
@Kouteki Kouteki moved this from Todo to In Progress in 🧙‍♂️gno.land core team May 15, 2024
@Kouteki Kouteki moved this from In Progress to Todo in 🧙‍♂️gno.land core team Oct 21, 2024
@Kouteki Kouteki moved this from Todo to Backlog in 🧙‍♂️gno.land core team Oct 28, 2024
@petar-dambovaliev petar-dambovaliev added the in focus Core team is prioritizing this work label Nov 22, 2024
@Kouteki Kouteki moved this from Backlog to Todo in 🧙‍♂️gno.land core team Nov 22, 2024
@Kouteki Kouteki moved this from Todo to In Progress in 🧙‍♂️gno.land core team Nov 22, 2024
@Kouteki Kouteki moved this from In Progress to In Review in 🧙‍♂️gno.land core team Dec 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐞 bug Something isn't working in focus Core team is prioritizing this work 📦 🤖 gnovm Issues or PRs gnovm related
Projects
Status: In Review
Development

Successfully merging a pull request may close this issue.

4 participants