-
-
Notifications
You must be signed in to change notification settings - Fork 12
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
Conversation
There was a problem hiding this 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!
src/rules/remove_if_expression.rs
Outdated
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); |
There was a problem hiding this comment.
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)()" | ||
); |
There was a problem hiding this comment.
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.
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 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
The wrapping in parentheses will make the extra values go away. |
Thank you so much! It seems possible to implement easily except for |
… 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
This PR may close #207 |
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 WhyUsing an
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 |
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 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. Similar to how darklua handles However, I can acknowledge how it may be difficult to know when or how to split the expression up. For example, setting 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. |
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
Output