Skip to content

Conversation

duncanawoods
Copy link
Contributor

I believe most of the Callable/Handler structs/traits in magic.rs are not doing anything useful. If I am wrong, I will be curious to learn why!

This PR replaces them with a simpler IntoFunction trait which is much less weird and enables calling the boxed closure to without any unnecessary allocation.

@duncanawoods duncanawoods changed the title radically simplify function binding magic as an IntoFunction trait simplify function binding magic as an IntoFunction trait May 6, 2025
@clarkmcc clarkmcc requested review from clarkmcc and Copilot and removed request for clarkmcc May 6, 2025 16:41
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR simplifies function binding in the interpreter by replacing the old Callable/Handler mechanism with a leaner IntoFunction trait that avoids unnecessary allocations.

  • Replaces function call invocations in objects.rs from an indirect call method to a direct closure call.
  • Removes legacy Handler/Callable types and updates the function registry in magic.rs.
  • Adjusts macros and context retrieval in macros.rs and context.rs to support the new trait.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
interpreter/src/objects.rs Updates function invocations and registry get method.
interpreter/src/magic.rs Removes legacy function structures and defines the new IntoFunction.
interpreter/src/macros.rs Revises macro implementations to implement IntoFunction.
interpreter/src/context.rs Updates imports and changes get_function return type accordingly.
Comments suppressed due to low confidence (2)

interpreter/src/macros.rs:66

  • [nitpick] Consider renaming the lambda parameter '_ftx' in the generated closures to a more descriptive name such as 'ctx' to improve readability.
impl<F, $($t,)* R> IntoFunction<($($t,)*)> for F

interpreter/src/context.rs:108

  • Changing the return type from Option<Box> to Option<&Function> may affect consumers that expect an owned instance; please verify that all call sites can work with a borrowed Function.
pub(crate) fn get_function<S>(&self, name: S) -> Option<&Function>

@clarkmcc
Copy link
Collaborator

clarkmcc commented May 6, 2025

Thanks for this! That doesn't surprise me, I ripped a lot of the magic off of axum's implementation back in the day. It was due for a tidy!

@clarkmcc clarkmcc merged commit f65fe4c into cel-rust:master May 6, 2025
1 check passed
@github-actions github-actions bot mentioned this pull request May 6, 2025
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.

2 participants