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

State of flychecking #14

Closed
ljli opened this issue Mar 28, 2017 · 1 comment
Closed

State of flychecking #14

ljli opened this issue Mar 28, 2017 · 1 comment

Comments

@ljli
Copy link
Contributor

ljli commented Mar 28, 2017

We pass the flags --eval, --strict and --show-trace.
We do not only evaluate derivations so --eval is probably a given.
--strict makes sense as a default but should be configurable per buffer since it might reject valid code. error: stack overflow (possible infinite recursion) is currently not parsed and flycheck complains ("Exit code > 0, but no error...").
--show-trace is passed to handle errors in imported files. Without --show-trace only the error of the root cause is shown as if we had evaluated that file. If an error in a different file than the one in the current buffer is reported flycheck gives no indication, that something is wrong in the current buffer. With --show-trace we get a stack of errors with While evaluating <thing> at <loc> at the top, where <thing> is e.g. an attribute in the current file and <loc> is its location. So that info is more useful, but since we only use that, no indication to what actually went wrong and where is given. The user has to scan beginning at <loc> for import statements and then open those files and repeat this recursively till the file with the actual cause is reached. So we want to use the top and the bottom error of the call stack to construct a custom error message which includes what went wrong and in which file. Furthermore we could try to report the error at the location of the actual import statement instead of at the attribute that is bound to the expression where the import occurs ({ not-here = <lots-of-code> \*but here ->*\ import ./file-with-err.nix).
In nixpkgs we usually have package expressions as self contained functions in separate files, which are ultimately pulled into all-packages.nix with callPackage. There are two problems with that model with regards to evaluation checking.
rec { a = b; b = a; } is nonsense and nix-instantiate tells us as much. arg: rec { a = b; b = a; } isn't better, but nix-instantiate says <LAMBDA> and has no objections. Since files containing top level sets are pretty rare compared to files with a top level function, this is not so great. Direct references missing in the lexical scope are detected, though. (arg: { a = b; } is an error, but arg: { a = arg.b; } is fine).
Assume a toy model with the files all-packages.nix (rec { callPackage = ...; a = callPackage ./a.nix {}; callPackage ./b.nix {}), a.nix (args: { attrA = 1; }), b.nix ({ a }: someStuff = a.attrB; }. Looking at b.nix alone we can say it is a valid expression and not much more. Knowing that it should at least work in the context of being imported in all-packages.nix we can see it has a problem (a.nix does not provide attrB). The vast majority of code in nixpkgs follows this model.
In practice that means that flycheck when used on nixpkgs does not report very much of interest beyond plain syntax errors, and --strict does not help much, since it has no effect on functions. I think we should do better.
My first idea would be to evaluate some top level expression which may or may not import the .nix file loaded in the current buffer, along with file itself. With --show-trace we should even get accurate reports for the current buffer if our file is affected. Two concerns here.
Making sure that the expression is evaluated far enough that imports for our file are evaluated. If the top level expression is a set, --strict should do that.
nixpkgs is riddled with platform/architecture/.. dependent assertions. We need to ignore them if they fail. nix-instantiate has a --keep-going flag, but so far it didn't do anything for me.

@matthewbauer
Copy link
Member

So flycheck has recently included its own Nix checker. It's the same idea as what was in this repo but since it's now official, I've decided to remove it from this repo. Any fixes or changes should now be directed to the flycheck repo. #5 remains valid though.

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

No branches or pull requests

2 participants