oppiliappan / statix

lints and suggestions for the nix programming language
https://git.peppe.rs/languages/statix/about
MIT License
557 stars 21 forks source link

Invalid eta-reduction suggestions #4

Closed danth closed 2 years ago

danth commented 2 years ago
[W07] Warning: This function expression is eta reducible
    ╭─[./modules/syncthing.nix:86:13]
    │
 86 │       (map (folder: nameValuePair folder.path folder) foldersOnThisDevice);
    ·             ────────────────────┬───────────────────  
    ·                                 ╰───────────────────── Found eta-reduction: nameValuePair folder.path
────╯

This warning is a false-positive because folder would now be undefined.

Statix should ensure that any removed function arguments do not occur in the remaining expression before it suggests eta-reduction.

danth commented 2 years ago

By the way, is there some documentation on how lints are written? I'm willing to contribute, but would need to understand how this AST code works:

https://github.com/nerdypepper/statix/blob/aa1a85527613ca8cabd5b7134ab46833564439c7/lib/src/lints/eta_reduction.rs#L21-L31

oppiliappan commented 2 years ago

This is a great catch, thanks for the report.

I'm willing to contribute, but would need to understand how this AST code works:

Absolutely. Please run cargo doc --open to keep library documentation handy. rnix-parser is evolving fast and the online documentation is often ahead by a bit. Here is a rough guide, which I should probably include as a document in the repo later:

  1. The #[lint(...)] macro tells you everything you need to know about the lint, the most important piece of metadata here is the match_with attribute. This value describes the kind of node or token we wish to match against. If that node is seen in the source, this lint's validation function is called.

  2. The validate function accepts a SyntaxElement. Everything inside the validate function simply describes the kind of node we are looking for. It might seem a little strange that we have access to only one node, but in reality, the entire AST is available because of rowan's api (node.parents() gives us access to parent nodes for example). To fill in this function, all you need to figure out is how to traverse the AST and assert some conditions. To make it easier to visualize the tree, please visit https://cstea-nix.peppe.rs. It uses WASM to run rnix-parser in the browser, you will get a solid idea of what Nix CSTs look like. I also make heavy use of the if-chain crate to write cascading assertions without nesting. I know that this para seems very hand-wavey, but I think reading a simple lint such as unquoted_splice.rs will give you a good idea about the types and function calls. rnix::types is where all the magic happens, do take a look at that. The rowan crate is also very important to understand the idea of SyntaxNodes, SyntaxTokens and SyntaxElements.

  3. Return values: The return value is an optional report. If the lint does not want to raise an error, just return None. A Report is a collection of Diagnostics. A Diagnostic is a combination of a range (TextRange) and a message (String), and optionally, a fix for this diagnostic (Suggestion). Most lints just return a Report with a single Diagnostic. I will not go into detail about Suggestions just yet.

oppiliappan commented 2 years ago

Fixed in 48c5727