ayecue / greybel-vs

VSCode extension for GreyScript.
https://marketplace.visualstudio.com/items?itemName=ayecue.greybel-vs
MIT License
16 stars 7 forks source link

Outer locals should not propagate automatically through multiple levels of function scopes #100

Closed Olipro closed 1 year ago

Olipro commented 1 year ago

I absolutely loathe that I have to report this, because frankly, the way Greybel does it should be the way MiniScript does. In fact, keeping this behaviour through some sort of flag or setting for anyone who's just writing Greybel-specific code wouldn't be a bad thing at all.

Given:

a = function
    foo = {"bar": 123}
    b = function
        c = function
            print foo.bar
        end function
        c
    end function
    b
end function
a

An error should be thrown when the inner-most function attempts to print foo.bar.

In order for the code to run correctly, it would need to look like this:

a = function
    foo = {"bar": 123}
    b = function
                locals.foo = foo // or of course locals.foo = outer.foo
        c = function
            print foo.bar
        end function
        c
    end function
    b
end function
a

In essence, at any given scope level, a scope's outer is always that of the parent scope's locals and never any higher. Exposure to child scope(s) requires explicitly creating an entry in the current scope's locals.

Special caveat

If, instead of locals.foo = foo you were to write foo = foo, MiniScript will issue a deprecation warning, but nonetheless it will behave as if you wrote locals.foo = outer.foo - more precisely, it will do a walk to find the rhs operand, then evaluate the lhs and, since locals lacks the key foo, put it there. This also means that foo = foo; foo = foo would be the equivalent of locals.foo = outer.foo; locals.foo = locals.foo

Olipro commented 1 year ago

Couple of additional cases that might be related (or not):

self should never participate in implicit lookup

Foo = {"a":123}
Foo.a = function
  locals.self = self // Without this line, the current scope is "self-less" and should never work.
  b = function
    print self.a // should fail
  end function
  b
end function
Foo.a

The print should fail because self would need to be referenced using outer.self.a or, of course by setting locals.self = outer.self

Function has implicit self where none should exist

Foo = {"a":123}
Foo.a = function
    locals.self = self
    b = function
        print locals.hasIndex("self") // should be false
    end function
    b
end function
Foo.a

0 should be printed. I also noticed while testing that there seems to be a tree inside locals where you can walk each depth which I imagine is what makes this happen. Essentially, each scope is disconnected and has its own independent tables. A shame since your way would make the language much better.

ayecue commented 1 year ago

Very helpful thank you.

I absolutely loathe that I have to report this, because frankly, the way Greybel does it should be the way MiniScript does. In fact, keeping this behaviour through some sort of flag or setting for anyone who's just writing Greybel-specific code wouldn't be a bad thing at all.

But probably people might be confused then why it works in greybel and not in MiniScript/GreyScript so probably best to remove the capability.

The "fix" should be available as soon https://github.com/ayecue/greybel-vs/pull/102 is merged.

self should never participate in implicit lookup

Interestingly when I tested that code in the online MiniScript editor it just crashes with a timeout. Anyway with https://github.com/ayecue/greybel-vs/pull/102 merged it will throw unknown path.

Function has implicit self where none should exist

Interesting I wasn't aware of that. With https://github.com/ayecue/greybel-vs/pull/102 merged it should be fixed though. I also added test cases now for this specific case checking globals, locals, outer, self and super.

I'll give an update once it's merged.

ayecue commented 1 year ago

The fixes addressing these issues should be merged now. Please check if it works for you as well.

Olipro commented 1 year ago

Broke a few things that were supposed to be broken so I'm calling it goooooood!

Olipro commented 1 year ago

Dang, gotta reopen it - this started failing:

TEST_SETUP function()
    globals.mockGlobals = MiniMock.build({})
    toMock.forEach function(key)
        globals[key] = mockGlobals.define(key, 1) // This line
    end function
    globals.mockRouter = MiniMock.build({})
    mockRouter.define("firewall_rules", 0)
    mockRouter.define("local_ip", 0)
    mockRouter.define("devices_lan_ip", 0)
    mockRouter.define("device_ports", 1)
    mockRouter.define("used_ports", 0)
end function

Runtime error: Unknown path globals.

Globals should always be in scope :)

Olipro commented 1 year ago

This seems to be specifically related to using globals with the [] operator. if I print globals within the same scope, it outputs the map just fine.

I have currently worked around this by doing glbl = globals and then assigning through the freshly-minted local.

ayecue commented 1 year ago

I've implemented some optimizations when it comes to globals, outer, locals and self lookups. The interpreter usually pre builds certain parts that are not dynamic but it seems like I forgot to test certain cases such as globals["test"]. I have a fix in the pipeline. Hopefully that will resolve the issue.

ayecue commented 1 year ago

Okay fix should be available in a bit.