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

Implement support for math.lerp #1608

Merged
merged 3 commits into from
Jan 9, 2025
Merged

Implement support for math.lerp #1608

merged 3 commits into from
Jan 9, 2025

Conversation

zeux
Copy link
Collaborator

@zeux zeux commented Jan 9, 2025

This change implements math.lerp RFC with C function definition, builtin
function, builtin constant folding and type inference, and tests.

The tests validate a few lerp properties by providing counter-examples
for popular lerp implementations; the testing is of course not
exhaustive, as exhaustive testing was done offline using fuzzing.

Type definitions are implemented in a separate commit; to avoid having
to copy the entire surface two more times I've split the string into a minimal
set of components that can be spliced based on existing flags. I would expect
further splits to happen across library boundaries, but I avoided doing that to
simplify the patch. Let me know if you'd rather see this done, or if you'd rather
see the type changes in a separate PR.

Codegen support will be implemented separately: it requires new IR for conditional
selects to represent the desired logic without using a branch.

@@ -235,7 +235,7 @@ static uint8_t getBytecodeConstantTag(Proto* proto, unsigned ki)
return LBC_TYPE_ANY;
}

static void applyBuiltinCall(int bfid, BytecodeTypes& types)
static void applyBuiltinCall(LuauBuiltinFunction bfid, BytecodeTypes& types)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(made this change to get exhaustive switch checks)

@zeux zeux marked this pull request as ready for review January 9, 2025 06:41
Copy link
Collaborator

@vegorov-rbx vegorov-rbx left a comment

Choose a reason for hiding this comment

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

Please remove the unflagged split changes to the definitions.
Splitting like we do for buffer and vector is ok, but splitting them further with a string builder approach isn't great to work with.

I would take duplication over it, however you might wait until after the Friday sync which removes one of the copies.

This change implements math.lerp RFC with C function definition, builtin
function, builtin constant folding and type inference, and tests.

The tests validate a few lerp properties by providing counter-examples
for popular lerp implementations; the testing is of course not
exhaustive, as exhaustive testing was done offline using fuzzing.

Type definitions and codegen support will be implemented separately.
@zeux
Copy link
Collaborator Author

zeux commented Jan 9, 2025

splitting them further with a string builder approach isn't great to work with

I don't quite see why that is the case - I think the approach as implemented in the original commit is a clean way to allow adding members separately in the future. I thought about splitting individual libraries entirely (with declare foo : {} wrapping), but that doesn't solve the problem of excessive duplication (e.g. here math.map & math.lerp are added to the same library, so you'd need to duplicate the entire type surface of math three times).

Additionally, there was a TODO comment "// TODO: there has to be a better way, like splitting up per library" which I recommend removing because after seeing this I decided I might as well do it this way.

Regardless, I've reverted the type changes entirely and I will wait for Luau team to figure this out as well as add math.lerp definition using whatever flagging paradigm they prefer.

@zeux zeux requested a review from vegorov-rbx January 9, 2025 15:45
@vegorov-rbx
Copy link
Collaborator

vegorov-rbx commented Jan 9, 2025

I don't quite see why that is the case - I think the approach as implemented in the original commit is a clean way to allow adding members separately in the future.

Just to explain the reasoning from my side, I dislike how it's inconsistent between old and freshly added members.
It also takes more effort to have consistent ident/formatting while writing extra added definitions out-of-line.

If there is an intent that after unflagging the string will be embedded into the "full" definition for a library, it means that unflagging is not trivial and gets harder to argue for unflagging PR safety internally.
(unflagging of lua_pushcfunction(L, math_lerp, "lerp"); into a new mathlib array entry is also non-trivial, but has been done before, although a different flagging style might be required later [like throwing from the function that is not yet enabled]; not asking for changes here today)

Having parts of the same construct declare class {/} split away from content also decreases readability of a declaration as a whole.

so you'd need to duplicate the entire type surface of math three times

Copy-pasting big chunks of code has never been a problem for me ;)
But it's true that it gets error-prone at certain number of N in 2^N, that is certainly a negative of my direction.

Additionally, there was a TODO comment "// TODO: there has to be a better way, like splitting up per library" which I recommend removing because after seeing this I decided I might as well do it this way.

We have started splits with vector and buffer and hopefully continue with the other parts.
Note that separate strings can still be loaded using multiple calls to loadDefinitions so concatenation is not strictly required. This might be useful in the future to support typechecking configurations which lack a corresponding luaopen_* call at runtime.
I believe having to duplicate smaller blocks will be more manageable than with a full string.

@vegorov-rbx vegorov-rbx merged commit 8a4ef26 into master Jan 9, 2025
8 checks passed
@vegorov-rbx vegorov-rbx deleted the fnlerp branch January 9, 2025 17:42
@zeux
Copy link
Collaborator Author

zeux commented Jan 9, 2025

If there is an intent that after unflagging the string will be embedded into the "full" definition for a library, it means that unflagging is not trivial and gets harder to argue for unflagging PR safety internally.

Yes - that was the intent. I think it should be possible to argue that "foo" + (true ? "bar" : "") can be safely replaced by "foobar".

different flagging style might be required later [like throwing from the function that is not yet enabled];

I only did this this way because of how math.map was implemented fwiw, canonically in the past "throw if not enabled" approach was used more often I believe.

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.

2 participants