Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handling of LUA_TNIL and LUA_TNONE with boolean parameters #105

Closed
rpatters1 opened this issue Apr 4, 2023 · 18 comments
Closed

Handling of LUA_TNIL and LUA_TNONE with boolean parameters #105

rpatters1 opened this issue Apr 4, 2023 · 18 comments

Comments

@rpatters1
Copy link
Contributor

rpatters1 commented Apr 4, 2023

This a follow-on to issue #82.

Suppose you have this C method registered in Lua:

int my_method(bool arg);

With LB2, any of the following calls in Lua leads to my_method being called with arg == false in C++.

my_method(false)
my_method(nil)
my_method()

I currently have a constructor function with a boolean parameter that is omitted by the Lua code. LB3 is calling the constructor with true rather than false! (!!!!) This is perhaps the most wrong of any possible choice. I see only two possible acceptable behaviors if a boolean parameter is missing or nil:

  1. Pass false to the C function like LB2 does.
  2. Throw (or return) a run-time error.

If I were starting from scratch I might vote for option 2. But my project has been going a long time with the assumption of option 1. Many of the constructors and methods have trailing optional boolean parameters that C++ defaults to false. The option 1 behavior allows those trailing booleans also to be optional in Lua for free.

For my project, switching to LuaBridge3 (which I would very much like to do) is contingent on no breaking changes to the many thousands of lines of code written for Lua on Finale over the last nearly 20 years. Option 2 would be an unacceptable breaking change. I'm a believer in flexibility and options, so perhaps the choice between 1 & 2 should be both well-defined and documented (which I don't think it is at the moment) and selectable by means of a compile-time macro.

@rpatters1 rpatters1 changed the title Handling of LUA_TNIL and LUA_TNONE with boolean values Handling of LUA_TNIL and LUA_TNONE with boolean parameters Apr 4, 2023
@Mellnik
Copy link

Mellnik commented Apr 4, 2023

Why not post it in #82? One could argue that if you don't need to pass anything as parameter then it should be wrapped with std::optional. But I understand backward compatibility. It's up to kunitoki how he wants to handle it.

@rpatters1
Copy link
Contributor Author

rpatters1 commented Apr 4, 2023

I mainly posted a new issue because it is to report the bug that omitting lua boolean parameters causes true to be sent to C++. That's a flat-out bug.

It also appears that at least Stack::get will get a boolean value as true if it is anything other than nil or nothing or false.

@rpatters1
Copy link
Contributor Author

Concerning breaking changes, to me this situation appears similar to what happened with the introduction of integer types in Lua 5.3. By default, Lua 5.3 would emit a runtime error when you assigned a non-integer floating point value to an integer property. (At least, to an integer property created by LuaBridge on a C++ class.)

This was a massive breaking change and a huge barrier to adoption. It is the reason that my project never progressed past 5.2 for 6 years. I recently decided to investigate the issue further and discovered that Lua offers a compile-time macro to truncate floats into integers instead of issuing runtime errors. I am advocating something similar with LuaBridge 3 and omitted booleans, because unless this issue is addressed I can't use LB3. And I really do want to use it.

@kunitoki
Copy link
Owner

kunitoki commented Apr 5, 2023

If you declare the method with bool, but call it without parameters, that should raise a lua error. We should gradually move to use optional for optional parameters.

@rpatters1
Copy link
Contributor Author

If you declare the method with bool, but call it without parameters, that should raise a lua error.

I don't disagree, but it is a breaking change. I think it is important not to let the perfect be the enemy of the good.

@kunitoki
Copy link
Owner

kunitoki commented Apr 5, 2023

The reason i took over luabridge3 was to make things consistent and more aligned with how modern C++ works as well, regardless of breaking changes. This code is more than 10 years old, and what was good 10 years ago, might not be so good nowadays.

@rpatters1
Copy link
Contributor Author

rpatters1 commented Apr 5, 2023

regardless of breaking changes.

Breaking changes are a big deal. In putting all this work into LB3, surely you would prefer to see wider rather than narrow adoption. Breaking changes are a major impediment to adoption. Your statement is pretty much the definition of making the perfect be the enemy of the good: almost never a good choice.

And consider that the older = value will always be the easier choice for optional parameters. The vast, vast majority of C++ functions use them over std::optional and will for the foreseeable future. By sticking to your policy, you are telling users of LB3 they either have to modify 3rd party libraries to use std::optional instead, or they have to laboriously comb through their code and replace the registrations of such functions with proxy functions. There has to be an easier way. Neither of the others is going to happen.

All that said, I think we can agree that the current behavior of sending true to C++ constructors when a boolean parameter is omitted is clearly and obviously the wrong choice. That's the main fix I need at the moment. I'll take either runtime error or false.

I may be prepared to eat the breaking change for omitted boolean params. Indeed, you will find that I complained about this from the opposite side in the LB2 repo, but since no fix would ever be forthcoming, I embraced the behavior. Now I'll have to de-embrace it and go find and fix any documentation I wrote that allowed it.

@rpatters1
Copy link
Contributor Author

rpatters1 commented Apr 5, 2023

Here's a test case:

class foo
{
   bool val_;
public:
   foo(bool param) : val_(param) {}
   bool val() const { return val_; }
};

   luabridge::getGlobalNamespace(L)
      .beginNamespace("foobar")
         .beginClass<foo>("foo")
            .addConstructor<void(*)(bool)>()
            .addFunction("val", &foo::val)
         .endClass()
      .endNamespace();
local foo = foobar.foo()
print (foo:val()) -- > prints "true"

In this case foo:val() should either print "false", or foobar.foo() should throw a runtime error. On LB2 it is the former behavior. (foo:val() returns false.)

@kunitoki
Copy link
Owner

kunitoki commented Apr 5, 2023

Some design decisions in LB2 are also questionable. Breaking changes are surely annoying, but all changes come at a cost, if not, this would be LB 2.0.1 not 3.0.0.

Btw, thanks for the repro case.

@rpatters1
Copy link
Contributor Author

If only there were a way to treat an old-style optional parameter as std::optional in LB3. It's cumbersome to have to write proxies for them. I wonder if it would be possible to flag them when they are registered.

@kunitoki
Copy link
Owner

kunitoki commented Apr 5, 2023

The idiomatic way to pass optionals in C++ is std::optional nowadays.

@kunitoki
Copy link
Owner

kunitoki commented Apr 5, 2023

Here's a test case:

Pushed a fix to master

@rpatters1
Copy link
Contributor Author

rpatters1 commented Apr 5, 2023

The idiomatic way to pass optionals in C++ is std::optional nowadays.

So say many C++ purists, but it will never be true. = value optionals are part of the language, and they will never go away. There are 40 years of legacy C++ code written with = value and more being added every day. I would bet there is more added every day now in 2023 than there is std::optional being written.

The syntax of = value is much less cumbersome and easier to read, and all but the most dedicated c++ purists will continue to use it.

@kunitoki
Copy link
Owner

kunitoki commented Apr 5, 2023

default arguments != optional

they serve different purposes.

@rpatters1
Copy link
Contributor Author

I agree. They serve different purposes C++. The issue is, how to port a function with default arguments into Lua with LB3. What I wish for is a way we could mark the default arguments to be treated as std::optional inside LB. This would allow Lua programs to call the same way C++ functions do.

I realize it can be done with proxy functions. I spent a couple of hours this morning trying to write a generic proxy function to let default arguments be optional in Lua, but even with ChatGPT helping I couldn't get it to work. I've discovered that when I ask ChatGPT to do something that isn't really possible I get wrong code from it. 🤣

@rpatters1
Copy link
Contributor Author

My code is working now, so I will close this issue. We still have #82 to track the larger question.

@kunitoki
Copy link
Owner

kunitoki commented Apr 6, 2023

I usually wrap the methods i can't change with lambdas.

@rpatters1
Copy link
Contributor Author

Yes, what I was thinking about was whether is a way to make that wrapping process less cumbersome. So far I haven't com up with one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants