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

Fix jsonnet-lint erorr when linting importing files and imported files at the same time: #544 #548

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

thombashi
Copy link
Contributor

@thombashi thombashi commented Jul 11, 2021

This PR aims to fix #544.

I also encounter #544 (the same case with #544 (comment))

Please let me know if my understanding is wrong.
The problem seems to be caused by XxxtoAST (ImportAST and SnippetToAST) functions return different Node instances, even with the same arguments.
Specifically, about using the ast.Node cache. ImportAST function (call SnippetToAST function internally) uses ast.Node cache, while a direct call of SnippetToAST function does not use the cache.
So, the return values are different (e.g. LocationRange.File).
This will results missing key at getExprPlaceholder.

To fix this problem, I made changes to SnippetToAST to use the same cache with ImportAST.

After applied the PR:

$ cat foo.jsonnet
local bar = import 'bar.libsonnet';
{}
$ cat bar.libsonnet
{}
$ ./jsonnet-lint foo.jsonnet bar.libsonnet
foo.jsonnet:1:7-35 Unused variable: bar

local bar = import 'bar.libsonnet';


Problems found!
exit status 2

@google-cla google-cla bot added the cla: yes label Jul 11, 2021
@coveralls
Copy link

Coverage Status

Coverage increased (+0.003%) to 67.905% when pulling d9173fc on thombashi:fix_imports_for_lint into 51daeb3 on google:master.

@sbarzowski
Copy link
Collaborator

Thanks for sending this. I think your diagnosis is correct, but the solution causes other problems. I think that now if you use EvaluateAnonymousSnippet multiple times, you'll actually get the first one evaluated every time. The very idea behind the Snippet variants was that it can be used outside of cachable paths. It is supposed to return a new thing each time.

So to fix this I would go a slightly different route. (a) Figure out why returning different root nodes causes issues in the first place. Intuitively it should be slower, but still work. (b) Change the API of the linter to take the filenames and use importAST instead of hacking around the snippets (this is a breaking change, but it's still ok for the linter).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

jsonnet-lint panics on an import that imports the same file
3 participants