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

Allow intrinsics to have multiple input arguments #9527

Merged

Conversation

stuhood
Copy link
Member

@stuhood stuhood commented Apr 13, 2020

Problem

Intrinsics have been functions from one Value to another Value for a while now, but this does not allow an intrinsic function to depend on multiple Params simultaneously.

Solution

Move the definitions of Intrinsics next to their implementations, and change their signatures to Vec<Value> -> Value to allow for further expansion of inputs.

Finally, to prepare for multi-platform speculation make MultiPlatformExecuteProcess the first intrinsic with multiple arguments by adding a Platform requirement.

…port for intrinsics taking more than one argument.

# Delete this line to force CI to run the JVM tests.
[ci skip-jvm-tests]
# Delete this line to force CI to run the JVM tests.
[ci skip-jvm-tests]
@stuhood stuhood force-pushed the stuhood/multiple-inputs-to-intrinsics branch from e919335 to 3ff8f2f Compare April 13, 2020 06:23
@stuhood
Copy link
Member Author

stuhood commented Apr 13, 2020

The commits are useful to review independently.

intrinsics.insert(
Intrinsic {
product: types.directory_digest,
inputs: vec![types.input_files_content],
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to be able to tie the inputs length to the length on the values vec expected by the intrinsic function. The &args[0], &args[1] look scary in isolation (what if there are 3 input types and the function evolves to only using 2 or needing 4. There is and will be a small enough set of intrinsics that just making sure these align by hand is probably good enough.

Copy link
Member Author

@stuhood stuhood Apr 13, 2020

Choose a reason for hiding this comment

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

There is and will be a small enough set of intrinsics that just making sure these align by hand is probably good enough.

Yea. For better or worse, these have evolved away from a large switch statement and toward dynamic dispatch like this, because it makes the callsite clearer. It's possible that moving back toward the switch statement is feasible, but for now I think that the fact these are now isolated to one file makes this lack of type safety manageable.

@stuhood stuhood merged commit 45bf1da into pantsbuild:master Apr 13, 2020
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