[PATCH] setmetatable({}) causes error

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
29 messages Options
12
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH] setmetatable({}) causes error

Nathaniel Musgrove
Hello,

I noticed that setmetatable({}, nil) works normally, but setmetatable({})
causes an argument type error. This patch fixes the error and restores
expected behavior.

When calling setmetatable() with 1 argument, the lua_type() call returns
LUA_TNONE, which fails the condition in the following luaL_argcheck() because
it only checks for a table or an _explicit_ nil, despite having an identical
meaning. By moving lua_settop(L, 2) to the beginning of the function, a
1-argument stack is extended with an explicit nil, and a 2-argument (or more)
stack behaves the same as before.

Regards,
Nathaniel

lbaselib.c
@@ -125,11 +125,11 @@
  static int luaB_setmetatable (lua_State *L) {
+  lua_settop(L, 2);
   int t = lua_type(L, 2);
   luaL_checktype(L, 1, LUA_TTABLE);
   luaL_argcheck(L, t == LUA_TNIL || t == LUA_TTABLE, 2,
                     "nil or table expected");
   if (luaL_getmetafield(L, 1, "__metatable") != LUA_TNIL)
     return luaL_error(L, "cannot change a protected metatable");
-  lua_settop(L, 2);
   lua_setmetatable(L, 1);
   return 1;
 }

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] setmetatable({}) causes error

Luiz Henrique de Figueiredo
> I noticed that setmetatable({}, nil) works normally, but setmetatable({})
> causes an argument type error. This patch fixes the error and restores
> expected behavior.

What is the expected behavior? The signature of setmetatable is
setmetatable(v,t); it sets the setmetatable of v to t or removes the
setmetatable of v if t is nil. Allowing v to be omitted is confusing.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] setmetatable({}) causes error

Nathaniel Musgrove
On Thu, Jul 13, 2017 at 5:32 AM, Luiz Henrique de Figueiredo
<[hidden email]> wrote:
> What is the expected behavior? The signature of setmetatable is
> setmetatable(v,t); it sets the setmetatable of v to t or removes the
> setmetatable of v if t is nil. Allowing v to be omitted is confusing.

I'm not allowing v to be omitted, I'm allowing t to be omitted. The "if t is
nil" part of your response is exactly the problem: if t is omitted, it should
act as if it's nil, but currently it doesn't, instead raising an argument type
error.

Regards,
Nathaniel

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] setmetatable({}) causes error

Luiz Henrique de Figueiredo
> I'm not allowing v to be omitted, I'm allowing t to be omitted.

Yes, I meant t, sorry for the typo.

Allowing t to be omitted is confusing because it is in the interest of
the programmer to be explicit about what metatable is being set.

setmetatable(v) is not explicit and seems very confusing to me.

Of course, if you are fine with this, use your patch.
But you don't need to patch the C code. You can it in Lua:

        local old_setmetatable=setmetatable
        function setmetatable(v,t) return old_setmetatable(v,t) end

The code seems useless but the explict argument t implies that it gets
an explict nil if you call setmetatable(v).

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] setmetatable({}) causes error

Nathaniel Musgrove
On Thu, Jul 13, 2017 at 5:47 AM, Luiz Henrique de Figueiredo
<[hidden email]> wrote:
> Allowing t to be omitted is confusing because it is in the interest of
> the programmer to be explicit about what metatable is being set.
>
> setmetatable(v) is not explicit and seems very confusing to me.

There are exactly zero other scenarios in Lua where f(nil) produces different
behavior than f(). The fact that this function is the sole exception to the
rule violates the principle of least astonishment.

If the programmer wishes to use an explicit nil, that should be their choice,
not an arbitrary requirement that exists nowhere else.

Regards,
Nathaniel

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] setmetatable({}) causes error

Luiz Henrique de Figueiredo
> There are exactly zero other scenarios in Lua where f(nil) produces different
> behavior than f().

Try assert(), getmetatable(), ipairs(), pcall(), tonumber(), tostring(), type().

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] setmetatable({}) causes error

Lorenzo Donati-3
In reply to this post by Nathaniel Musgrove
On 13/07/2017 14:54, Nathaniel Musgrove wrote:

> On Thu, Jul 13, 2017 at 5:47 AM, Luiz Henrique de Figueiredo
> <[hidden email]> wrote:
>> Allowing t to be omitted is confusing because it is in the interest of
>> the programmer to be explicit about what metatable is being set.
>>
>> setmetatable(v) is not explicit and seems very confusing to me.
>
> There are exactly zero other scenarios in Lua where f(nil) produces different
> behavior than f(). The fact that this function is the sole exception to the
> rule violates the principle of least astonishment.
>
> If the programmer wishes to use an explicit nil, that should be their choice,
> not an arbitrary requirement that exists nowhere else.
>
> Regards,
> Nathaniel
>
>
I don't agree, sorry.

Usually when you omit an argument and that argument it's interpreted as
nil it is a strong hint that that argument is intended to be optional
and some *senisble* default is to be used.

in setmetatable the t argument is not at all optional, semantically, it
is the very "target" of the operation. It would be very confusing to be
able to omit an argument just as a shortcut for `nil`, when you just
want to change the metatable of `nil`. This goes *against* the principle
of least astonishment, IMO: if I'm setting a new metatable (something
very low level and "risky"), I like to be explicit. What's the point of
having t default to "nil". Note that here `nil` doesn't mean "no
argument", but it means `nil` really, i.e. the type `nil` whose only
value is `nil`.

cheers!

-- Lorenzo


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] setmetatable({}) causes error

Lorenzo Donati-3
On 13/07/2017 15:26, Lorenzo Donati wrote:

> On 13/07/2017 14:54, Nathaniel Musgrove wrote:
>> On Thu, Jul 13, 2017 at 5:47 AM, Luiz Henrique de Figueiredo
>> <[hidden email]> wrote:
>>> Allowing t to be omitted is confusing because it is in the interest of
>>> the programmer to be explicit about what metatable is being set.
>>>
>>> setmetatable(v) is not explicit and seems very confusing to me.
>>
>> There are exactly zero other scenarios in Lua where f(nil) produces
>> different
>> behavior than f(). The fact that this function is the sole exception
>> to the
>> rule violates the principle of least astonishment.
>>
>> If the programmer wishes to use an explicit nil, that should be their
>> choice,
>> not an arbitrary requirement that exists nowhere else.
>>
>> Regards,
>> Nathaniel
>>
>>
> I don't agree, sorry.
>
> Usually when you omit an argument and that argument it's interpreted as
> nil it is a strong hint that that argument is intended to be optional
> and some *senisble* default is to be used.
>
> in setmetatable the t argument is not at all optional, semantically, it
> is the very "target" of the operation. It would be very confusing to be
> able to omit an argument just as a shortcut for `nil`, when you just
> want to change the metatable of `nil`. This goes *against* the principle
> of least astonishment, IMO: if I'm setting a new metatable (something
> very low level and "risky"), I like to be explicit. What's the point of
> having t default to "nil". Note that here `nil` doesn't mean "no
> argument", but it means `nil` really, i.e. the type `nil` whose only
> value is `nil`.
>
> cheers!
>
> -- Lorenzo
>
>
>
Ok, scrap those last lines about setting the metatable for `nil`. I made
a fool of myself by not reading well the original post.

My brain read debug.setmetatable, instead of setmetatable in basic
library and all got mixed up.

Brain fart!

sorry for the noise

-- Lorenzo

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] setmetatable({}) causes error

Pierre Chapuis
In reply to this post by Luiz Henrique de Figueiredo
July 13, 2017 3:16 PM, "Luiz Henrique de Figueiredo" <[hidden email]> wrote:

>> There are exactly zero other scenarios in Lua where f(nil) produces different
>> behavior than f().
>
> Try assert(), getmetatable(), ipairs(), pcall(), tonumber(), tostring(), type().

And of course:

    > select('#', nil, nil, nil)
    3

It's a feature, not a bug.

--
Pierre Chapuis

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] setmetatable({}) causes error

Nathaniel Musgrove
In reply to this post by Luiz Henrique de Figueiredo
On Thu, Jul 13, 2017 at 6:16 AM, Luiz Henrique de Figueiredo
<[hidden email]> wrote:
>> There are exactly zero other scenarios in Lua where f(nil) produces different
>> behavior than f().
>
> Try assert(), getmetatable(), ipairs(), pcall(), tonumber(), tostring(), type().

> select('#')

The difference is that those are degenerate scenarios. There is no reason that
anyone would ever actually write those function calls, because the outcome is
easily predicted, and more importantly useless.

By comparison, setmetatable(t) actually performs a task, one that a programmer
would be entirely reasonable in wanting to perform.

At the end of the day, you say it might be confusing to omit a nil, but at
least when someone says "why doesn't setmetatable(t) raise an error?" I can
explain that omitting an argument is equivalent to explicitly passing nil
(which is a very basic, consistent Lua principle), and passing nil to
setmetatable() has explicitly-defined behavior.

What's confusing is beating your head against a wall for half an hour trying
to figure out why on Earth a function call is raising an error before finally
finding the answer in Lua's _source code_. That was time out of my life that
had no reason at all to be spent, and I would rather that no one else ever
have to do that.

Regards,
Nathaniel

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] setmetatable({}) causes error

Duncan Cross
On Thu, Jul 13, 2017 at 4:22 PM, Nathaniel Musgrove
<[hidden email]> wrote:
> The difference is that those are degenerate scenarios. There is no reason that
> anyone would ever actually write those function calls, because the outcome is
> easily predicted, and more importantly useless.

Well, except that you might do something like tostring(myfunction())
where myfunction() usually returns one value but sometimes returns no
values.

-Duncan

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] setmetatable({}) causes error

Luiz Henrique de Figueiredo
In reply to this post by Nathaniel Musgrove
> What's confusing is beating your head against a wall for half an hour trying
> to figure out why on Earth a function call is raising an error before finally
> finding the answer in Lua's _source code_.

The authoritative answer is given by the manual which says

        setmetatable (table, metatable)
        http://www.lua.org/manual/5.3/manual.html#pdf-setmetatable

Note that the second argument must be present. It is not marked as optional.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] setmetatable({}) causes error

Nathaniel Musgrove
> Well, except that you might do something like tostring(myfunction())
> where myfunction() usually returns one value but sometimes returns no
> values.

In that case, why would you knowingly tostring() a function when one of its
normal results causes an error?

> Note that the second argument must be present. It is not marked as optional.

I concede that much, and I have nothing further to argue. Forgive my
frustration; I've loved using Lua for several years and this is the first time
I've ever come across such an inconsistency. I was very excited to be able to
provide a quality-of-life improvement to my favorite programming language.

Thank you for your prompt and measured responses.

Regards,
Nathaniel

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] setmetatable({}) causes error

Roberto Ierusalimschy
In reply to this post by Nathaniel Musgrove
> What's confusing is beating your head against a wall for half an hour trying
> to figure out why on Earth a function call is raising an error before finally
> finding the answer in Lua's _source code_. That was time out of my life that
> had no reason at all to be spent, and I would rather that no one else ever
> have to do that.

Beating your head against a wall is always confusing...

-- Roberto

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] setmetatable({}) causes error

Viacheslav Usov
In reply to this post by Luiz Henrique de Figueiredo
On Thu, Jul 13, 2017 at 5:28 PM, Luiz Henrique de Figueiredo <[hidden email]> wrote:

> The authoritative answer is given by the manual which says

>         setmetatable (table, metatable)

> Note that the second argument must be present. It is not marked as optional.

This semantics is incompatible with section 3.4.11, which says:

When a function is called, the list of arguments is adjusted to the length of the list of parameters, unless the function is a vararg function, which is indicated by three dots ('...') at the end of its parameter list.

Several lines further in the text, we have an example:

     function f(a, b) end
[...]
     CALL            PARAMETERS
     
     f(3)             a=3, b=nil
This, per the manual, a non-vararg argument, if omitted, is nil. This implies that the call setmetatable(t), which is not vararg, shall be equivalent to setmetabable(t, nil), which has a specified meaning.

I should say that the TNONE vs TNIL difference is very arcane and confusing. And this difference only exists for C functions, and one can make a C function conform with the documented behaviour (3.4.11), or not.

Cheers,
V. 
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] setmetatable({}) causes error

Roberto Ierusalimschy
> > The authoritative answer is given by the manual which says
>
> >         setmetatable (table, metatable)
> >         http://www.lua.org/manual/5.3/manual.html#pdf-setmetatable
>
> > Note that the second argument must be present. It is not marked as
> optional.
>
> This semantics is incompatible with section 3.4.11, which says:
>
> When a function is called, the list of arguments is adjusted to the length
> of the list of parameters, unless the function is a *vararg function*,
> which is indicated by three dots ('...') at the end of its parameter list.

This whole explanation, as you said, is in Section 3.4.11, which is
all about functions defined in Lua. I hope you do not expect that
the previous sentence ("Parameters act as local variables that are
initialized with the argument values") applies also to C functions.
Similarly for the next sentence ("A vararg function [...] collects all
extra arguments and supplies them to the function through a vararg
expression").  So, I see no reason to expect that this particular
sentence would apply to C functions. But we can pretend this was the
source of the problem and try to make that text more clear.

-- Roberto

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] setmetatable({}) causes error

Viacheslav Usov
On Thu, Jul 13, 2017 at 6:12 PM, Roberto Ierusalimschy <[hidden email]> wrote:

> I hope you do not expect that the previous sentence ("Parameters act as local variables that are initialized with the argument values") applies also to C functions.

In fact, at some point back in the past, that is exactly what I expected. I expected that an omitted argument would be seen as nil in a C function. I learned the actual behaviour only when I had to chase some subtle bugs introduced by my expectation. I did not like the experience. That is why I called that arcane and confusing.

Another problem is that your language above implies that we somehow must expect that setmetatable should behave like a "C function" and not a like a Lua function. Why? I do not think "because it is implemented in C is a sensible answer for someone who has no idea about the implementation of Lua. And why, indeed, should we expect that C functions behave differently? The difference is explained very compactly at the end of section 4.3, but it only makes sense for a C programmer, not for a Lua programmer (a pure Lua programmer would not even look there), who still sees there "virtual type LUA_TNONE, which behaves like a nil value". Does that imply that every user of Lua should understand the arcane details of Lua's C language API? I think that would be too much to expect.

> Similarly for the next sentence ("A vararg function [...] collects all extra arguments and supplies them to the function through a vararg expression").  So, I see no reason to expect that this particular sentence would apply to C functions.

In fact, the behaviour of C functions can be most easily understood by assuming they are always vararg. Even then the TNONE/TNIL difference remains, but at least the "adjustment" is taken out.

Cheers,
V.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] setmetatable({}) causes error

nobody
On 2017-07-13 18:32, Viacheslav Usov wrote:

> On Thu, Jul 13, 2017 at 6:12 PM, Roberto Ierusalimschy wrote:
>> I hope you do not expect that the previous sentence ("Parameters
>> act as local variables that are initialized with the argument
>> values") applies also to C functions.
>
> […] we somehow must expect that setmetatable should behave like a "C
> function" and not a like a Lua function. […]
>
>> Similarly for the next sentence ("A vararg function [...] collects
>> all extra arguments and supplies them to the function through a
>> vararg expression").  So, I see no reason to expect that this
>> particular sentence would apply to C functions.
>
> In fact, the behaviour of C functions can be most easily understood
> by assuming they are always vararg. Even then the TNONE/TNIL
> difference remains, but at least the "adjustment" is taken out.

Almost… ^,^  The easiest way to understand C functions from Lua is that
they are always _vararg-only_ functions with no named parameters.  TNONE
doesn't exist on the Lua side (it's an artifact of indexing beyond top
of stack) but can be emulated by checking if the index is bigger than
`select('#',...)`.

So you can actually treat C functions as a feature-limited subset of Lua
functions, which is easy to understand.

-- nobody

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] setmetatable({}) causes error

Soni L.
In reply to this post by Luiz Henrique de Figueiredo


On 2017-07-13 09:47 AM, Luiz Henrique de Figueiredo wrote:

>> I'm not allowing v to be omitted, I'm allowing t to be omitted.
> Yes, I meant t, sorry for the typo.
>
> Allowing t to be omitted is confusing because it is in the interest of
> the programmer to be explicit about what metatable is being set.
>
> setmetatable(v) is not explicit and seems very confusing to me.
>
> Of course, if you are fine with this, use your patch.
> But you don't need to patch the C code. You can it in Lua:
>
> local old_setmetatable=setmetatable
> function setmetatable(v,t) return old_setmetatable(v,t) end
>
> The code seems useless but the explict argument t implies that it gets
> an explict nil if you call setmetatable(v).
>

It doesn't feel confusing to me. Both setmetatable({}) and rawset({}, 1)
feel natural as "unset" operations. (but I can accept that neither are
supported and argue for their support without being completely rude
about it.)

setmetatable(v) with omitted t is like telling it to unset. Other ppl
might expect it to set a new, empty metatable, but the nil/none
semantics of lua (see e.g. load()) tend towards unsetting.

(I'd also argue setmetatable should return 2 values: the table, and the
new metatable. that is, both arguments passed to it.)

--
Disclaimer: these emails may be made public at any given time, with or without reason. If you don't agree with this, DO NOT REPLY.


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] setmetatable({}) causes error

Tobias Kieslich
In reply to this post by Nathaniel Musgrove

Quoting Nathaniel Musgrove <[hidden email]>:

>
> By comparison, setmetatable(t) actually performs a task, one that a  
> programmer
> would be entirely reasonable in wanting to perform.
>
It is very much dependent what is the expected outcome seen from the
programmers perspective. For example:

if a==b then ... end
vs
if a=b  then ... end

are both entirely viable pieces of code but one could be an error if
the it was basically a typo. setmetatable(t) falls into the same
category for me. The error allows me to catch a potential problem much
sooner. I wouldn't even do what Luiz' suggested and overwrite the
function with the same name. Create a function called removemetatable(t)
is much more clear. We do have mandatory code reviews at work and
that made me appreciate clarity in code very much but then again,
it's just an opinion :-P

  -tobbik


12
Loading...