Use from library
Closed this issue · 18 comments
It'd be nice to use compat53 without having global side effects.
e.g. local xpcall = require "compat53.base".xpcall
The top level "compat53" could then import into the global scope.
I have implemented what you ask for in the "isolated" branch. However, I'm not convinced that this is necessary (given that the newer functions are pretty much backwards compatible) or that the result is particularly robust (e.g. the compatibilty functions for debug.traceback
, xpcall
, and coroutine.running
are interdependent -- this could already bite us now if someone stores the old functions in upvalues). Also some features only work when applied to the global environment (e.g. string methods, file methods, and the package.searchers
alias).
I have also added another shortcut module ("compat53.module") which replaces the environment of the calling chunk, so you don't modify the environment of other files but you can use the new functions without storing them in locals one by one. The same gotchas as above apply though (no global features, unexpected results when mixing functions from different versions).
I want to hear some more opinions about this (e.g. @hishamhm), and some more testing wouldn't hurt either, before I consider merging this into master ...
The idea behind 'base' was it would be the things from the base library. Then compat53.math
would be the math things, etc.
Regarding shared metatables (e.g. string), it's easy enough to write code in a non-method fashion if you know you need 5.3 only features.
The xpcall+coroutine.status is a tricky one though...
I guess we have to untangle the xpcall
situation first -- if we pursue this approach (since using a single file module also has its charm). Other than that I see no big problems (and it probably helps maintainability).
we have to untangle the xpcall situation
Is it a one way dependency?
If so we can at least get it half done....
I took a closer look. There is shared state (pcall_previous
, pcall_callOf
, xpcall_running
, pcall_mainOf
) between the following public functions:
debug.traceback
(r)coroutine.running
(r)pcall
(rw)xpcall
(rw)
We could probably move the shared state to some helper module -- I don't want to redo the LuaJIT detection in every submodule anyway ...
I want to hear some more opinions about this (e.g. @hishamhm), and some more testing wouldn't hurt either, before I consider merging this into master ...
I don't like the concept of compat53.base
. The purpose of compat-5.x is to encourage developers to write code targeting the latest Lua version even if they're tied to an older Lua VM for practical reasons, not to help them stick to writing code to old Lua versions by letting them cherry-pick features from the current version.
OTOH, if I follow the idea of compat53.module
correctly, it is to help a developer to distribute modules targeting the 5.3 API, advertise them as Lua 5.1+5.2+5.3 compatible and not affect their users' global environment... is that right? That sounds good to me in principle, but the gotchas are worrying and I'm not sure if it's worth the added complexity.
In short, I prefer if the compat modules adopt an all-or-nothing approach (at the very least at module level) instead of promoting even more API-dialect fragmentation.
OTOH, if I follow the idea of compat53.module correctly, it is to help a developer to distribute modules targeting the 5.3 API, advertise them as Lua 5.1+5.2+5.3 compatible and not affect their users' global environment... is that right?
Yeah that's what I wanted: to develop modules against 5.3 and have them work with 5.1 and 5.2. As a good library writer, you want no global side effects of loading the library.
Ok, this is my analysis of the xpcall
situation:
- It concerns only PUC-Rio Lua 5.1. (LuaJIT and Lua 5.2+ don't need/use the code in question.)
- The compatibility versions for
pcall
/xpcall
use coroutines under the hood to allow yielding. The shared variables are either used to hide the internal coroutines (pcall_mainOf
) or to collect accurate stack traces in case of errors withdebug.traceback
set as message handler (pcall_callOf
,pcall_previous
, andxpcall_running
). - Therefore, if you don't use the compat52
pcall
orxpcall
, you can't yield (which is to be expected), but all other compatibility functions should be able to handle the missing info. - If you mix old/new
pcalls
/xpcalls
, you could get incorrect/incomplete stack traces bydebug.traceback
. (I don't think that's very serious, and I probably would have ignored that issue altogether if I had implemented the yieldablepcall
/xpcall
.) - If you use the compat52
pcall
/xpcall
(from within a running coroutine), but the oldcoroutine.running
, you will get the internal coroutine used by the compatibility functions, not the one you'd ordinarily get. I don't know how serious that is, but I would imagine that this could result in subtle bugs in some very rare circumstances.
@renatomaia: Please correct me if I'm wrong.
Anyhow, if I read that correctly we are down to two options:
1.) Leave it as it is (compat53.lua
modifies the global environment)
2.) Provide compat53.lua
which modifies the global environment and an isolated, module-friendly alternative.
(I.e. the fine-grained option of compat53.string
, compat53.table
, etc. is off the table.)
Btw., I think most of those shared tables should be weak ...
Anyhow, if I read that correctly we are down to two options:
1.) Leave it as it is (compat53.lua modifies the global environment)
2.) Provide compat53.lua which modifies the global environment and an isolated, module-friendly alternative.
(I.e. the fine-grained option of compat53.string, compat53.table, etc. is off the table.)
This is how I read it too.
Since the module-friendly version will have more limitations than the global version anyway (such as no extended string methods), maybe we could avoid the extra gotchas by simply not supporting enhanced pcall/xpcall for PUC-Rio Lua 5.1 in the module-friendly version. This is just an idea I'm throwing, I'm not sure if it's a good one.
On 13 Apr 2015, at 14:22, Philipp Janda wrote:
� Therefore, if you don't use the compat52 pcall or xpcall, you can't yield (which is to be expected), but all other compatibility functions should be able to handle the missing info.
There is one problem in my original implementation of this "yieldable pcall" support for Lua 5.1+Compat52, which are the debug functions. I didn't do any wrapping debug API to hide this corotouine-based infrastructure, mainly because it would be very complicated. I have a utility module in pure Lua to transverse all values in a lua_State that relies on the 'debug' module that have different implementations for 5.1 and 5.2 because of the lack of this support. :-(
� If you mix old/new pcalls/xpcalls, you could get incorrect/incomplete stack traces by debug.traceback. (I don't think that's very serious, and I probably would have ignored that issue altogether if I had implemented the yieldable pcall/xpcall.)
My main motivation for this is that I use a lot of modules written in Lua that interact with each other (e.g. through 'require'). Therefore if my applications don't guarantee that the global 'pcall' and other standard libraries are consistent (e.g. by requiring 'compat52' in Lua 5.1) then some modules might mix them and get wrong results because the semantics they rely on are not respected.
� If you use the compat52 pcall/xpcall (from within a running coroutine), but the old coroutine.running, you will get the internal coroutine used by the compatibility functions, not the one you'd ordinarily get. I don't know how serious that is, but I would imagine that this could result in subtle bugs in some very rare circumstances.
Again the motivation was to guarantee the standard semantics of these functions for all modules written in Lua that are written expecting such semantics.
@renatomaia: Please correct me if I'm wrong.
I far as I can recall from what I wrote, you are absolutely right in your analysis.
Anyhow, if I read that correctly we are down to two options:
1.) Leave it as it is (compat53.lua modifies the global environment)
For the implementation of compat53 for Lua 5.1, I think this is useful for the reasons I mentioned earlier. In case of a compat53 for Lua 5.2, this whole "yieldable pcall" implementation over coroutines should just be dropped for good as you suggested.
2.) Provide compat53.lua which modifies the global environment and an isolated, module-friendly alternative.
(I.e. the fine-grained option of compat53.string, compat53.table, etc. is off the table.)
I personally agree with Hisham's concerns about the support with older versions of Lua and the complexity of providing all these features.
Renato Maia
After thorough consideration and the feedback so far I have modified the second option (the first is still to leave everything as it is now (@renatomaia: the coroutine based pcall
is only active if running PUC-Rio Lua 5.1, already)):
The compat53.module
would not install pcall
, xpcall
, coroutine.running
, and debug.traceback
into the custom environment. Instead, those functions are transfered to a separate module compat53.pcall
(which is loaded by the main compat53
module automatically of course). If the module author can make sure that all functions pcall
ed by him/her don't reference the old coroutine.running
, he/she can load the yieldable pcall
support explicitly. I would also rename compat53.base
to something that indicates that it's supposed to be a private module (like private
, impl
, internal
, or core
, although I figure that one's for C modules -- other suggestions?). What I still don't like about compat53.module
is that it uses debug functions, and I consider it "impolite" for normal modules to rely on the debug module (not much better than monkey patching in fact, but you could at least choose the lesser of the two evils) ...
Further opinions/feedback welcome. If nothing new comes up, I'll decide in a few days.
Anyhow, I shall add a wiki page for coroutine.running
to describe the problem (which could occur if someone saves the old version in an upvalue before compat53
is require
d).
Ok, I chose option 2c ;-)
I think that providing a way to use Compat-5.3 in a Lua module without affecting other modules would greatly increase the usefulness of the Lua part of this project. Therefore, I want to split the current single file module in two modules (one with global effects, one for module authors). However, using debug functions to achieve isolation would prevent many uses (I also considered using a C module for that, but that could exclude other developers interested in this module). There is a way that does not need debug functions but instead shifts some of the burden to the module developer:
local _ENV = require("compat53.module")
if setfenv then setfenv(1, _ENV) end -- if you need to support Lua 5.1
This approach also makes it possible to merge compat53.module
and compat53.impl
, so we end up with only two files (both of which can be require
d by other people).
compat53.module
will not include yieldable pcall
support (like @hishamhm suggested). I decided against the extra compat53.pcall
module because the implementation became too messy: When compat53.pcall
is require
d you would need to identify the environment in which to import the functions. One can not simply add those to the module table because then a user of compat53.module
would end up with the modified functions if some other module developer decided to require
both compat53.module
and compat53.pcall
. This requires the use of debug functions, in which case the main module (compat53
) would have a dependency on debug functions as well (or we would end up with two different compat53.pcall
modules). Also some coroutine functions depend on a shared upvalue (a fake main coroutine), so mixing modules written with/without compat53.pcall
could lead to similar problems as the current coroutine.running
issue. If you really need yieldable pcall
in a module for PUC-Rio Lua 5.1, there's always coxpcall (which should be fit for Lua 5.3 as soon as my pull request is merged).
For users of the normal compat53
module nothing changes (other than that you need two files now): yieldable pcall
, string/file methods, etc. are all available.
Regarding cherry picking of individual functions/mixed APIs: This is definitely possible in this approach, and frankly I don't care that much. I've made it clear in the readme how compat53.module
is supposed to be used, and that we don't recommed/support cherry picking. But with pcall
etc. gone from compat53.module
I can see no technical reason why it shouldn't work (other than bad style).
I've implemented the outlined approach in the isolated
branch for testing. I plan to merge it into master
in a few days, so this is the last chance for objections/feedback.
Done.
I was reading through the xpcall
code today, is it intentional that both compat.init and compat.module have their own main_coroutine
? it seems we're doing that wrapping twice...
Yes, that was intentional. main_coroutine
is needed for three of the functions from the coroutine
library (one of them coroutine.running
). coroutine.running
has to change when (x)pcall
become yieldable. I found it easier/cleaner to redefine the other two coroutine
functions as well and keep main_coroutine
as an upvalue than to share main_coroutine
between compat.module
and compat.init
somehow.
Yes, that was intentional. main_coroutine is needed for three of the functions from the coroutine library (one of them coroutine.running). coroutine.running has to change when (x)pcall become yieldable. I found it easier/cleaner to redefine the other two coroutine functions as well and keep main_coroutine as an upvalue than to share main_coroutine between compat.module and compat.init somehow.
I thought you could write all the stuff in init.lua without recreating the main
coroutine.
Couldn't you have:
local coroutine_running = M.coroutine.running -- from compat53.module
function M.coroutine.running()
local co, main = coroutine_running()
if not main then
return pcall_mainOf[co] or co, false
else
return co, true
end
end
Once you have this, resume
and status
don't need to be overridden a second time.
Right, that should work ...