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

add load_from_std_lib function to Lua state #136

Merged
merged 3 commits into from
Aug 1, 2019

Conversation

azdle
Copy link
Collaborator

@azdle azdle commented Jun 19, 2019

This function allows loading of any portion of the standard libraries
after initial state creation.

Specifically, this is filling a need to load the debug library after an
error is encountered so that a traceback can be generated without
risking exposing the debug library anywhere else in the code.

@kyren
Copy link
Contributor

kyren commented Jul 27, 2019

Sorry for sitting on this for so long...

I've been trying to figure out exactly what I'm going to do re: #116 for a while, and this PR sort of overlaps with it.

I still need to figure out what I'm going to do, and that will affect everything that loads the Lua stdlib, but I don't think that's a valid reason to wait on this PR, since this is still a strict improvement.

Would it be possible to just combine the body here with the code that loads libraries in create_lua? I'd be happy to merge this then.

@azdle
Copy link
Collaborator Author

azdle commented Jul 30, 2019

No worries.

Not sure why I didn't do this in the first place. Force-pushed with the big block of ifs moved out to a common function.

Also, CI seems to be failing on some not totally backwards compatible change to rust in 1.35 -> 1.36 that's causing rustyline to fail to build. s/3.0.0/5.0.0/ for rustyline fixes it. Do you want me to just push that in this PR so it builds? Or do you want that change done somewhere else to keep things clean?

@kyren
Copy link
Contributor

kyren commented Jul 30, 2019

You can go ahead and push that as part of this PR if you don't mind, I don't mind the drive-by change honestly.

@azdle
Copy link
Collaborator Author

azdle commented Jul 31, 2019

There was also a test case failing due to some changes in what errors the compiler produces for broken code. Updated a couple of tests in tests/compile-fail to what 1.36 uses too. Hopefully that's right, it looks like only the current stable is built in CI so that should get the shiny green checkmark here.

src/lua.rs Outdated Show resolved Hide resolved
src/lua.rs Outdated
///
/// This function is unsafe because it can be used to load the `debug` library which can be used
/// to break the safety guarantees provided by rlua.
pub unsafe fn load_from_std_lib(&self, lua_mod: StdLib) -> Result<()> {
Copy link
Contributor

@kyren kyren Aug 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be load_std_lib and also can there be an unsafe_load_std_lib variant that allows debug, similar to new_with_debug?

(new_with_debug is probably going to be renamed to unsafe_new when other things that the debug library are considered unsafe)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

load_std_lib implies to me that it loads the whole std lib. Maybe load_from_std_lib & unsafe_load_from_std_lib? Also any preference on if the first should error if debug is in the list or just ignore it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah you're right, I see your point on load_from_std_lib, that's fine.

The safe version should panic I think if it's asked to load the debug library (that's what new does).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, should have looked at that. 👍

Changed to load_from_std_lib and unsafe_load_from_std_lib. (Also squashed the fixups.)

This function allows loading of any portion of the standard libraries
after initial state creation, except debug for unsafety reasons.
Also added is `unsafe_load_from_std_lib` which *does* also allow loading
the debug library.

Specifically, this is filling a need to load the debug library after an
error is encountered so that a traceback can be generated without
risking exposing the debug library anywhere else in the code.
@azdle
Copy link
Collaborator Author

azdle commented Aug 1, 2019

Had to force-push again because I forgot to cargo fmt and I had an extra blank line.

@kyren
Copy link
Contributor

kyren commented Aug 1, 2019

Awesome work, thank you very much!

@kyren kyren merged commit e7e1b7e into mlua-rs:master Aug 1, 2019
@azdle
Copy link
Collaborator Author

azdle commented Aug 2, 2019 via email

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

Successfully merging this pull request may close these issues.

2 participants