add rpc interface for dependency resolution #288
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bkettle
force-pushed
the
bk/dependency-resolution
branch
from
55ba075 to
e07e4c0
Compare
|
Backwards compatibility summary: |
aryx
requested changes
Sep 4, 2024
| @@ -1797,12 +1799,44 @@ type output_format = [ | |||
| | Emacs | |||
| ] | |||
|
|
|||
| type manifest = { | |||
There was a problem hiding this comment.
Did you look at the types in OSS/interfaces/Input_to_core.atd?
They look very similar.
There was a problem hiding this comment.
There was also the prelimiary work by @mmcqd in OSS/target/Lockfile_xxx.ml
There was a problem hiding this comment.
We probably want to factorize some of those types at some points.
There was a problem hiding this comment.
I updated this type so that it is now essentially identical to both manifest_target in that file and to manifest in Target.ml. The type is still duplicated here because I wasn't aware of a better way to structure it, though. But hopefully this interface type will only be around for a few months until we get SSC into ocaml entirely
Anyway, curious to hear what you think of this new type and if it resolves your prior concerns
semgrep_output_v1.atd
Outdated
| ?git_ref: string option | ||
| ?git_ref: string option; | ||
| (* ID is used to associate dependencies with each other via depends_on relationships *) | ||
| ?id: string option; |
There was a problem hiding this comment.
Is there any way we can use a more precise type for those id? Like we did for rule_id in this file so that we have stronger typechecking and don't messup a rule ID with another kind of ID.
This is the identifier for what here?
There was a problem hiding this comment.
I've updated the types to no longer use these IDs. Instead, the new version uses the existing but unised children field (of type dependency_child list) to encode relationships between dependencies. Ultimately the ID system felt like it required too much out-of-band knowledge about the other FoundDependency items, but I was going back and forth between these options for a while so would be curious to hear your thoughts on this new approach.
semgrep_output_v1.atd
Outdated
|
|
||
| type dependency_relationships | ||
| <python decorator="dataclass(frozen=True)"> = { | ||
| dep_id: string; |
There was a problem hiding this comment.
maybe define a dep_id = string wrap (like for rule_id) and explain clearly what is a dependency ID, maybe with an example.
bkettle
force-pushed
the
bk/dependency-resolution
branch
from
62a4c11 to
524a6ac
Compare
bkettle
force-pushed
the
bk/dependency-resolution
branch
from
90bd998 to
55095c4
Compare
aryx
requested changes
Sep 19, 2024
semgrep_output_v1.atd
Outdated
| @@ -1817,6 +1862,7 @@ type function_call | |||
| the RPC pipe. | |||
| *) | |||
| | CallValidate of fpath | |||
| | CallResolveDependencies of resolve_dependencies_params | |||
There was a problem hiding this comment.
why introduce an intermediate type with a record with just one field?
Why not simpler | CallResolveDependencies of manifest list ?
semgrep_output_v1.atd
Outdated
| @@ -1827,4 +1873,5 @@ type function_return | |||
| | RetContributions of contributions | |||
| | RetFormatter of string | |||
| | RetValidate of bool | |||
| | RetResolveDependencies of resolve_dependencies_return | |||
There was a problem hiding this comment.
Same, I don't think you need the extra intermediate record with just one field ...
aryx
approved these changes
Sep 19, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds the RPC types that will be necessary to perform dependency resolution in OCaml. These changes comprise new types to represent a resolution request, response, and resolved relationships, and an additional optional
idfield onfound_dependencythat is used to associate a dependency with its dependency relationships.make setup && maketo update the generated code after editing a.atdfile (TODO: have a CI check)Closes SC-1732.