fix: global var dependencies #2077
Conversation
petar-dambovaliev
requested review from
jaekwon,
moul,
piux2,
deelawn,
thehowl and
mvertes
as code owners
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2077 +/- ##
========================================
Coverage 54.96% 54.96%
========================================
Files 595 595
Lines 79568 79689 +121
========================================
+ Hits 43731 43805 +74
- Misses 32529 32571 +42
- Partials 3308 3313 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
petar-dambovaliev
requested review from
a team,
gfanton and
leohhhn
as code owners
github-actions
bot
added
🧾 package/realm
petar-dambovaliev
force-pushed
the
vardeps
branch
from
c7365aa
to
813b7e1
Compare
petar-dambovaliev
force-pushed
the
vardeps
branch
from
d8cee96
to
ea4d5d0
Compare
petar-dambovaliev
force-pushed
the
vardeps
branch
from
ea4d5d0
to
db713cc
Compare
There was a problem hiding this comment.
The tests are still not covering a bunch of the cases:
https://app.codecov.io/gh/gnolang/gno/pull/2077?src=pr&el=tree
DeferStmt, BlockStmt and other are not tested entirely. Please address the issues and add test cases which check against their conditions, as well as error conditions.
For a feature like this, there cannot be only 1 filetest.
|
|
||
| func findUndefinedStmt(store Store, last BlockNode, stmt Stmt, t Type) Name { | ||
| switch s := stmt.(type) { | ||
| case *BranchStmt: |
There was a problem hiding this comment.
Can this be placed next to the case for nil, as they have the same result?
| curr := bn | ||
| for { |
There was a problem hiding this comment.
This code is quite convoluted, to do a for loop which should run at most twice
I also don't understand what's going on. Shouldn't this iterate through the parents? (Consider the case of a local declared in a function, accessed in a nested for loop / if block)
| } | ||
| } | ||
|
|
||
| if name != "" { |
There was a problem hiding this comment.
Can this not be a if name == "" { return "" } earlier on?
| pkg := packageOf(last) | ||
|
|
||
| if _, _, ok := pkg.FileSet.GetDeclForSafe(name); !ok { | ||
| // skip it if it's a local identifier |
There was a problem hiding this comment.
This comment does not match what's going on
There was a problem hiding this comment.
Can we also test for some error conditions?
Like, if there is actually a circular variable dependency when declaring closures.
There was a problem hiding this comment.
There is no code handling ForStmt
| return un | ||
| } | ||
| } | ||
| case *IfStmt: |
There was a problem hiding this comment.
There is no code handling the Init stmt
| func findUndefinedStmt(store Store, last BlockNode, stmt Stmt, t Type) Name { | ||
| switch s := stmt.(type) { | ||
| case *BranchStmt: | ||
| case *ValueDecl: |
| return name | ||
| } | ||
|
|
||
| func findUndefinedStmt(store Store, last BlockNode, stmt Stmt, t Type) Name { |
There was a problem hiding this comment.
Type expressions are generally not handled, both for ValueDecl, but also TypeDecl's. (Types can be declared within a function block).
This code panics, for instance:
package main
var a = func() int {
type B b
x := B(1)
return int(x)
}
type b int
func main() {
println(a())
}
zivkovicmilos
modified the milestones:
🏗4️⃣ test4.gno.land,
🏗4️⃣ test4.gno.land [POST LAUNCH]
fixes this by adding missing logic in in
findUndefined2