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 remove_if_expression rule #208

Closed
wants to merge 7 commits into from

Conversation

jiwonz
Copy link
Contributor

@jiwonz jiwonz commented Aug 29, 2024

Add remove_if_expression rule which removes luau's if expression syntax from source then translate it into closure and if statement.

Tests are succeeded.

Examples

Input

local a = if true then 1 else 2

Output

local a = (true and {(1)} or {(2)})[1]

@jiwonz jiwonz changed the title Add remove-if-expression rule Add remove-if-expression rule Aug 29, 2024
@jiwonz jiwonz changed the title Add remove-if-expression rule Add remove_if_expression rule Aug 29, 2024
Copy link
Contributor

@jeparlefrancais jeparlefrancais left a comment

Choose a reason for hiding this comment

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

Thank for contributing! I think we could switch the replacement strategy though. Someone in the Roblox open source discord shared this trick where you can do:

local a = if condition() then 1 else 0
local a = (condition() and { 1 } or { 0 })[1]

We should also wrap potential expressions that can return multiple values to avoid creating a bigger table than needed. So ... and functions calls should be wrapped in parentheses!

let result_return = nodes::ReturnStatement::one(if_exp.get_result().clone());
let else_result_return = nodes::ReturnStatement::one(if_exp.get_else_result().clone());
let front_block = nodes::Block::new(vec![], Some(result_return.into()));
let front_branch = nodes::IfBranch::new(if_exp.get_condition().clone(), front_block);
Copy link
Contributor

Choose a reason for hiding this comment

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

You can take advantage that Statement and LastStatement can be made into a Block, most methods will accept Into<Block> (or Into<Statement> or Into<Expression>).

let front_branch = IfBranch::new(if_exp.get_condition().clone(), result_return);

RemoveIfExpression::default(),
assign_if_expression("local a = if true then 1 else 2") => "local a = (function() if true then return 1 else return 2 end end)()",
assign_if_expression_with_elseif("local a = if true then 1 elseif false then 2 else 3") => "local a = (function() if true then return 1 elseif false then return 2 else return 3 end end)()"
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add more tests! For example, we need to treat function calls and ... in a specific way to make sure we are not returning multiple values. For that we need to wrap them in parentheses.

@jiwonz
Copy link
Contributor Author

jiwonz commented Aug 29, 2024

Thank for contributing! I think we could switch the replacement strategy though. Someone in the Roblox open source discord shared this trick where you can do:

local a = if condition() then 1 else 0
local a = (condition() and { 1 } or { 0 })[1]

We should also wrap potential expressions that can return multiple values to avoid creating a bigger table than needed. So ... and functions calls should be wrapped in parentheses!

Hello! Thanks for reviewing! I read your all 3 comments which were really helpful for me to improve my contribution.

but sorry. one thing that I'm not quite understanding is "We should also wrap potential expressions that can return multiple values to avoid creating a bigger table than needed. So ... and functions calls should be wrapped in parentheses!" I'm not sure what ... and function calls are actually meaning.

Can you please provide more example codes so can you help me I understand? Sorry about that.

@jeparlefrancais
Copy link
Contributor

but sorry. one thing that I'm not quite understanding is "We should also wrap potential expressions that can return multiple values to avoid creating a bigger table than needed. So ... and functions calls should be wrapped in parentheses!" I'm not sure what ... and function calls are actually meaning.

Can you please provide more example codes so can you help me I understand? Sorry about that.

Of course 😄 Take this example:

local function oof(...: string)
    return if condition(...) then ... else transform(...)
end
-- converts to
local function oof(...: string)
    return (condition() and { (...) } or { transform(...) })[1]
end
  1. Imagine that oof is called with multiple arguments, ... would expand in the table and put more values than needed.
  2. Imagine that transform(...) returns multiple values, it would also expand in the table and put more values than needed again

The wrapping in parentheses will make the extra values go away.

@jiwonz
Copy link
Contributor Author

jiwonz commented Aug 29, 2024

but sorry. one thing that I'm not quite understanding is "We should also wrap potential expressions that can return multiple values to avoid creating a bigger table than needed. So ... and functions calls should be wrapped in parentheses!" I'm not sure what ... and function calls are actually meaning.
Can you please provide more example codes so can you help me I understand? Sorry about that.

Of course 😄 Take this example:

local function oof(...: string)
    return if condition(...) then ... else transform(...)
end
-- converts to
local function oof(...: string)
    return (condition() and { (...) } or { transform(...) })[1]
end
  1. Imagine that oof is called with multiple arguments, ... would expand in the table and put more values than needed.
  2. Imagine that transform(...) returns multiple values, it would also expand in the table and put more values than needed again

The wrapping in parentheses will make the extra values go away.

Thank you so much! It seems possible to implement easily except for elseif hell... hm

… be changed, so node processor will be rewritten
Fix elseif behavior that doesn't add table expression
Change branches vec to referenced vec in process function
@jiwonz
Copy link
Contributor Author

jiwonz commented Sep 4, 2024

This PR may close #207

@Stefanuk12
Copy link
Contributor

Stefanuk12 commented Sep 7, 2024

In my opinion (as stated in #212), this is how the transformation should happen:

local maxValue = if a > b then a else b

goes to

local maxValue
if a > b then
    maxValue = a
else
    maxValue = b
end

Why

Using an if saves a few instructions and is truer to what it actually Luau compiles the statement to.

function anon_0(??)
    1     GETIMPORT   R2 1      ; a
    2     GETIMPORT   R3 3      ; b
    3     JUMPIFNOTLT R3 R2 L0
    4     NEWTABLE    R1 0 1
    5     GETIMPORT   R2 1      ; a
    6     SETLIST     R1 R2 1   ; 1
    7     JUMPIF      R1 L1
    8  L0 NEWTABLE    R1 0 1
    9     GETIMPORT   R2 3      ; b
    10    SETLIST     R1 R2 1   ; 1
    11 L1 GETTABLEN   R0 R1 1
    12    GETIMPORT   R1 5      ; print
    13    MOVE        R2 R0
    14    CALL        R1 1 0
    15    RETURN      R0 0
end

function anon_1(??)
    1     LOADNIL     R0
    2     GETIMPORT   R1 1      ; a
    3     GETIMPORT   R2 3      ; b
    4     JUMPIFNOTLT R2 R1 L0
    5     GETIMPORT   R0 1      ; a
    6     JUMP        L1
    7  L0 GETIMPORT   R0 3      ; b
    8  L1 GETIMPORT   R1 5      ; print
    9     MOVE        R2 R0
    10    CALL        R1 1 0
    11    RETURN      R0 0
end

function anon_2(??)
    1     GETIMPORT   R1 1      ; a
    2     GETIMPORT   R2 3      ; b
    3     JUMPIFNOTLT R2 R1 L0
    4     GETIMPORT   R0 1      ; a
    5     JUMP        L1
    6  L0 GETIMPORT   R0 3      ; b
    7  L1 GETIMPORT   R1 5      ; print
    8     MOVE        R2 R0
    9     CALL        R1 1 0
    10    RETURN      R0 0
end

Samples (in order)

local maxValue = ((a > b) and {(a)} or {(b)})[1]
print(maxValue)
local maxValue
if a > b then
    maxValue = a
else
    maxValue = b
end
print(maxValue)
local maxValue = if a > b then a else b
print(maxValue)

@jiwonz
Copy link
Contributor Author

jiwonz commented Sep 7, 2024

In my opinion (as stated in #212), this is how the transformation should happen:

local maxValue = if a > b then a else b

goes to

local maxValue
if a > b then
    maxValue = a
else
    maxValue = b
end

Why

Using an if saves a few instructions and is truer to what it actually Luau compiles the statement to.

function anon_0(??)
    1     GETIMPORT   R2 1      ; a
    2     GETIMPORT   R3 3      ; b
    3     JUMPIFNOTLT R3 R2 L0
    4     NEWTABLE    R1 0 1
    5     GETIMPORT   R2 1      ; a
    6     SETLIST     R1 R2 1   ; 1
    7     JUMPIF      R1 L1
    8  L0 NEWTABLE    R1 0 1
    9     GETIMPORT   R2 3      ; b
    10    SETLIST     R1 R2 1   ; 1
    11 L1 GETTABLEN   R0 R1 1
    12    GETIMPORT   R1 5      ; print
    13    MOVE        R2 R0
    14    CALL        R1 1 0
    15    RETURN      R0 0
end

function anon_1(??)
    1     LOADNIL     R0
    2     GETIMPORT   R1 1      ; a
    3     GETIMPORT   R2 3      ; b
    4     JUMPIFNOTLT R2 R1 L0
    5     GETIMPORT   R0 1      ; a
    6     JUMP        L1
    7  L0 GETIMPORT   R0 3      ; b
    8  L1 GETIMPORT   R1 5      ; print
    9     MOVE        R2 R0
    10    CALL        R1 1 0
    11    RETURN      R0 0
end

function anon_2(??)
    1     GETIMPORT   R1 1      ; a
    2     GETIMPORT   R2 3      ; b
    3     JUMPIFNOTLT R2 R1 L0
    4     GETIMPORT   R0 1      ; a
    5     JUMP        L1
    6  L0 GETIMPORT   R0 3      ; b
    7  L1 GETIMPORT   R1 5      ; print
    8     MOVE        R2 R0
    9     CALL        R1 1 0
    10    RETURN      R0 0
end

Samples (in order)

local maxValue = ((a > b) and {(a)} or {(b)})[1]
print(maxValue)
local maxValue
if a > b then
    maxValue = a
else
    maxValue = b
end
print(maxValue)
local maxValue = if a > b then a else b
print(maxValue)

It is not that easy to convert if expression into if statement since the case like this code above would be a nightmare of side effects

local c = ...
local a, c = sideEffect() + if condition then compute(c) else default(), resetC()

BTW, it was my first idea and the second idea was using closures then the final idea is using tables and and/or operators

@Stefanuk12
Copy link
Contributor

I'm on mobile but I believe that

local c = ...
local a, c = sideEffect() + if condition then compute(c) else default(), resetC()

compiles back to

local c = ...
local a = sideEffect() + if condition then compute(c) else default()
local c = resetC()

where the results of sideEffect and the if expr are stored in separate registers before added up so it's basically equal to

local c = ...
local a = sideEffect()
a += if condition then compute(c) else default()
local c = resetC()

Looking at the bytecode, it's executed as expected in order from left to right. sideEffect is callled then stored and finally added to the result of the if expr. Then, resetC is called.

Similar to how darklua handles a[foo()] += 1, a temporary variable could be made that computes the final value.

However, I can acknowledge how it may be difficult to know when or how to split the expression up. For example, setting a equal to the result then setting a again but with the if expr result.

I'm not too experienced with these things but Rust should make it easier since you can match Expr and make sure all cases are covered.

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.

3 participants