Carpet

Carpet

3M Downloads

Bug: Scarpet `run('command')` function does not return any values

Oxtaly opened this issue ยท 3 comments

commented

Description

The run('command') function never (except in a syntax exception error) returns the command's values, and always returns [0, [], null]

Evidence/steps to reproduce:

Test 1: /script run run('execute if entity @s')

Expected output: [1, [Test passed, count: 1], null]
image
(Ran with minecraft 1.20.1, fabric 0.16.9, carpet v1.4.112+v230608)

Observed output: [0. [], null]
image
(Ran with minecraft 1.21.4, fabric 0.16.9, carpet v1.4.161+v241203)

Test 2: /script run run('execute unless entity @s')

Expected output: [0, [], Test failed, count: 1]
image
(Ran with minecraft 1.20.1, fabric 0.16.9, carpet v1.4.112+v230608)

Observed output: [0. [], null]
image
(Ran with minecraft 1.21.4, fabric 0.16.9, carpet v1.4.161+v241203)

Guessed Source

Thank you to @dmunozv04 for finding the source here;

We believe that due to the new way commands are handled, the current implementation of the run('command') function returns too early before the command is ever executed, provided it isn't a syntax error, which does give the correct output (see below)
/script run run ('exec')
image

Attached is an image of the implementation with an additional oversight highlighted, where the command success value is always set to 0, regardless of the actual command execution
image

commented

Ok, I did some digging and figured out the problem

The command execution changed so that when you run a command while you are handling a command, the requested command gets put on the pile allowing the current command to finish.
Its a design choice but a way cleaner path to execute nested commands.

This impacts situations like
/script run run('setblock ....')
cause setblock will be run in a context of running the 'script' command

It has nothing to do with threading.

The issue can be mitigated by not running commands from commands, for instance this works:

/script run schedule(1, _() -> print(player(), run('/fill 200 100 200 201 101 201 stone')))

as well as calling run() in tick event or anywhere else but /script run/invoke

commented

Hey there, my deepest apologies for the delayed response;

I had missed the fact that this bug would only occur if it was called from another command, so this problem scope is more limited than I first thought, thank you for the added explanation and details;

I believe keeping run_cb() (possibly renamed to run_async() as you mentioned?) could still be good for usages in command ran contexts (notably custom script commands) without the need for a workaround or places where retrieving the first error message is necessary (although that would be quite a rare edge case)

(edited, had not seen the new commits, thank you for fixing the return code issue and updating the documentation)

commented

I've reopened the pull request (#2036) with run_async(expr, callback) + documentation and small documentation changes/precisions for run(expr), but if run_async isn't something you're looking to add into scarpet feel free to close both this issue and PR ๐Ÿ‘