google / go-jsonnet

Apache License 2.0
1.61k stars 230 forks source link

Static import analysis #326

Closed sh0rez closed 4 years ago

sh0rez commented 4 years ago

Hi!

In an effort to build a static import analysis tool while creating Tanka (https://github.com/grafana/tanka), we have been using some dirty-patching on jsonnet to speed up analysis speed.

Basically we are implementing a custom Importer that traces its way through the Imports to build a list of transient dependencies after all.

A full evaluation of jsonnet is not required here, which means we abort early:

https://github.com/sh0rez/go-jsonnet/blob/2787aa93adcc2f2b04e915a8f5523d589ead2ef3/vm.go#L202-L218

However, this patch is definitely not the right way, however I am not familiar enough with the jsonnet codebase to do a better one.

I have seen the efforts in 21c00f1b9ebc856aabf72e2bdb818e1f5b3c11f7 as well, however it is not obvious to me whether I can make use of this.

@sbarzowski Can you provide some details on this, whether I can use the efforts from 21c00f1b9ebc856aabf72e2bdb818e1f5b3c11f7 for my use-case or how to a better patch into go-jsonnet

sparkprime commented 4 years ago

You mean a file-level dependency graph? Should be easy as we disallow computing the filenames. I believe this is helped by Stan's recent change for exporting the logic for finding files.

sh0rez commented 4 years ago

Finished the issue now, accidentally opened it without a description

sh0rez commented 4 years ago

We use this in Tanka to say whether a Kubernetes environment imports a specific library which in turn allows us to figure out which envs to re-apply once we modified a lib

sparkprime commented 4 years ago

Have you considered using bazel to track the dependencies and re-execution of things? It would probably be possible to write something like gazelle to manage the bazel definitions through analysis of jsonnet files.

sparkprime commented 4 years ago

Probably overkill to add that as a dependency to your system though.

sh0rez commented 4 years ago

Yes, I believe bazel would be overkill, especially as the tool is already working. What I just need after all is a way abort the evaluation just after all imports are resolved ... So the importer would have been called on all imports and that's it.

Do you know a good way to get there? Or an alternative approach?

sparkprime commented 4 years ago

You wouldn't try to do a real evaluation, you'd do a static analysis which boils down to just whether any import construct exists in the file, then recursively handling whatever file it referred to.

sh0rez commented 4 years ago

I feel like it should be possible to re-use already existing code from this repository to avoid having to rewrite the parsing of jsonnet, etc, but do not really see a way to do so with the currently exported functions.

In other words .. how to resolve the imports without evaluating?

sbarzowski commented 4 years ago

In short: you can use vm.ImportAST and vm.SnippetToAST to get the Abstract Syntax Tree. Then all you need to do is find imports in it and repeat recursively. My guess is that it should be like 15-30 lines in total

If you want I can write you a more detailed response with examples, but tomorrow.

sbarzowski commented 4 years ago

Ah, I've just noticed that this function https://github.com/google/go-jsonnet/blob/master/internal/parser/context.go#L353 is not exported. This is going to be needed to make it as concise as I said before. I guess we can expose it.

sbarzowski commented 4 years ago

BTW in a PR I submitted today, there is a fragment which does something very similar to what you describe (i.e. it collects imports recursively): https://github.com/google/go-jsonnet/pull/325/files#diff-80f9fdcc81aed508ba78619f9dd78083R100

sh0rez commented 4 years ago

Ah, I've just noticed that this function https://github.com/google/go-jsonnet/blob/master/internal/parser/context.go#L353 is not exported. This is going to be needed to make it as concise as I said before. I guess we can expose it.

Got this working so far, but you are right, using this function would make it much nicer

sh0rez commented 4 years ago

Hey @sbarzowski, I have been able to implement my tool using the example from your PR and I can confirm it is working out nicely (https://github.com/grafana/tanka/pull/84)! We still need to expose this function, submitted a PR for this as well.

Thanks for your help!