~technomancy/fennel#220: 
Disable global checking in scopes with __ENV present

In Lua (5.2+), when a variable named _ENV is present, all attempts to read globals are redirected to reads in the _ENV table. This means that the allowed-globals list isn't relevant, and shouldn't be tested.

This change will enable fennel users to more easily write confusing and bad fennel code. For example:

(local my-html-dsl (require :my-html-dsl))

(my-html-dsl
  (fn [_ENV]
    (html
      (body
        (h1 "my document")
        (p "this is my document")))))
Status
RESOLVED WONT_FIX
Submitter
~xerool
Assigned to
No-one
Submitted
8 months ago
Updated
7 months ago
Labels
enhancement needs-design

~xerool 8 months ago

I don't think we can always guarantee that an "_ENV" sym always demangles to "_ENV"

~technomancy 8 months ago

I think this one is pretty straightforward? I just pushed a fix in ff9fd5f. Curious what you think still needs design.

~technomancy 8 months ago

Oh, maybe "needs design" is about the mangling issue? I would rather treat that as a separate bug.

~xerool 8 months ago*

The needs-design tag is about

  1. the mangling issue. We don't support nested _ENV because you sometimes get the mangling _ENV0 instead.
  2. I want to think about what to do with __fnl_global_foo_2dbar style manglings. Should we emit _ENV["foo-bar"] instead?

Design issues aside, I'm not sure if we want to take on support of _ENV or not. To me, _ENV is like global and require-macros in that it (sort of) breaks the rules of lexical scoping and makes code harder to understand. Fennel already asks users to use _G.field for accessing globals. It seems backward to let users skip _ENV.field when the behavior is more confusing than regular globals.

~technomancy 8 months ago

Hm... I think if we required people to explicitly call _ENV.field then it defeats the purpose of _ENV; you might as well just let-bind a regular table at that point.

Thinking it thru more, the problem with this change is that _ENV uses dynamic scope and as such it is incompatible with the entire mechanism of strict globals checking. This patch is only a half-measure because it disables strict globals checking when _ENV is lexically bound, which isn't how _ENV actually works! It gives the impression of supporting it while in reality only supporting a limited subset of it, kind of like how operators support multivals but only when the final arg is a values form. (and we all know how THAT goes)

With that in mind, maybe it's better to say that because it's impossible to statically determine which code will run under the "real" _G and which will run under a modified _ENV that you need to set --globals to a union of both. In that case, it's probably better to revert my change.

~technomancy 8 months ago

The mangling issues remain legitimate problems we need to fix in the compiler.

  • We should never mangle -ENV to _ENV
  • We should never mangle _ENV to anything else

These seem pretty clear to me.

I want to think about what to do with __fnl_global_foo_2dbar style manglings. Should we emit _ENV["foo-bar"] instead?

I think implementing global mangling in the first place was a mistake, but I'm concerned that it's now impossible to improve without introducing a breaking change. Because _ENV is dynamically scoped, emitting _ENV["foo-bar"] isn't really viable because we can't know where it needs to be done. Ideally we would have emitted _G["foo-bar"] from the beginning probably. Maybe we can fix that in 2.0?

~xerool 8 months ago

for lua 5.2+, _ENV["foo-bar"] always does what we want, but it would cause problems in lua5.1/luajit.

~technomancy 8 months ago

OK I was getting confused between _ENV and setfenv because I mostly only use the latter. Turns out _ENV isn't dynamically scoped at all and the comparison to _G is apt in terms of how it actually functions, tho the intent is different.

To me, _ENV is like global and require-macros in that it (sort of) breaks the rules of lexical scoping and makes code harder to understand. Fennel already asks users to use _G.field for accessing globals.

It really depends; you can use it to expand the set of globals, in which case I agree it makes code harder to understand, or you can use it to reduce the set of globals, in which case it makes the code easier to understand. (when capabilities are passed on the stack) But this change of skipping global checks when _ENV is in scope really only relevant in the case of expanding globals, so yeah... probably not worth encouraging? I think I will revert it.

It would be really cool if we could detect a table literal being bound to _ENV and temporarily reset allowedGlobals based on the keys it has. But that's probably more trouble than it's worth.

~technomancy REPORTED WONT_FIX 7 months ago

Register here or Log in to comment, or comment via email.