Skip to content

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
Contributor

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