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

RFC: Allow xpcall error handlers to yield #71

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ccuser44
Copy link

@ccuser44 ccuser44 commented Nov 9, 2024

RFC: Allow xpcall error handlers to yield

Rendered

@bradsharp
Copy link
Contributor

bradsharp commented Nov 11, 2024

An additional drawback is that a number of users use xpcall to prevent yielding of code. That said, this can be reimplemented with:

local function noyield(f, ...)
  local t = coroutine.create(f)
  local r = table.pack(coroutine.resume(t, ...))
  if coroutine.status(t) == "suspended" then
    coroutine.close(t)
    error("Method is not allowed to yield")
  end
  return table.unpack(r)
end

@ccuser44
Copy link
Author

An additional drawback is that a number of users use xpcall to prevent yielding of code. That said, this can be reimplemented with:

local function noyield(f, ...)
  local t = coroutine.create(f)
  local r = table.pack(coroutine.resume(t, ...))
  if coroutine.status(t) == "suspended" then
    coroutine.close(t)
    error("Method is not allowed to yield")
  end
  return table.unpack(r)
end

I see, that may be a problem. However I still absolutely think that the xpcall error handler should be able to yield.
First of all this behavior is totally undocumented. Relying on it is hacky and unreliable too as the yield behavior of both xpcall (in Luau) and pcall (before Luau when ypcall was deprecated).
Another is parity with vanilla Lua, the current behavior only partially mirrors that of vanilla Lua.
Also the debug semantics of not allowing the error handler to yield are quite un-user friendly.

@bradsharp
Copy link
Contributor

I'd be in favor of this but we need to carefully consider how we'd roll this out without impacting users.

@vegorov-rbx
Copy link
Contributor

I don't think it will be "Probably very minor though.", seems like a major increase in complexity of the VM where the whole Lua 5.2 implementation uses data structures incompatible with Luau VM.

Given limited use case and already existing yielding limitations, I don't think it will be worth adding this. Also don't want to accept an RFC to later revert as unimplementable.

Just because compatibility box in the docs in incorrect, doesn't mean we have to change Luau, we can just note the difference in the documentation correctly.

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

Successfully merging this pull request may close these issues.

3 participants