Looks like there's a lot of diff churn due to automatic formatting.

Yeah apparently I didn't manage to fork off latest or I spent long enough doing so that things have moved on, I'll do some rebasing. In the meantime, the changes in Linter however are all new - I didn't couple it to the existing pass there

OK, I did do some funky reformat for some reason. This should now be rebased, unfortunately I can't run tests even on master locally since updating to latest.

You need to delete .test_modules

Thanks, got it passing now. Some reason had to update the golden test for 1 file I didn't touch

#3808

Just wanted to say I really appreciate you picking this up. I'm sure this will have a hugely positive effect on the codebase I work on (although I guess I won't be able to give it a try until we've released 0.14 and updated at work).

I think this looks good. Have you tried running it against https://github.com/JordanMartinez/purescript-hail-mary? I would like to get an idea of the number of warnings to expect from this overall. You can use --verbose-stats with psa to get a breakdown.

@natefaubion I actually just deleted that repo. When I said there were 381 warnings using my master-compatible package set, I found out that many of them were due to # Type being used instead of Row Type. That made me realize that my branch wasn't including all of my work. So, I just finished updating that fork to point to all the work I did previously via the fix-warnings-ps-master-compatible branch.

Since I'm no Bash-fu person, I found that this code made it possible to install all packages in that set:

sudo apt-get install xclip -y
spago ls packages > example.txt && cat example.txt | sed -r "s/([a-zA-Z0-9-]+).*/\1/" | tr '\n' ' ' | xclip -selection c
spago install <CTRL+SHIFT+V>

The current number of warnings is now 377, but not all of them are the "the inferred kind for X is Y. Consider adding a kind signature" warnings.

I think there are some missing cases, particularly around record puns. When running against the package set, I get warnings for things like children in https://github.com/lumihq/purescript-react-basic/blob/master/src/React/Basic/DOM/Generated.purs#L17481

This is missing a case for Literal, so it does not work it's way into object or array literals.

I did see the (earlier) comment nate, just not got around to running against that compatible package set - I don't think I was aware of it before

@nwolverson I made a change to fix this, but I'm not going to presume by pushing to this branch. psa does not work against master currently due to the stderr/stdout switch. I ran a jq script against the error output.

From that above ps-master-compatible branch:

cd src
spago install
spago build > errors.json

and then to generate a table:

jq -r '.warnings | map(select(.errorCode == "UnusedName") | .moduleName) | group_by(.) | map({moduleName: .[0], count: .|length}) | .[] | "\(.moduleName) \(.count)"' errors.json | sort -k2 -n | column -t -s' ' | tail -r

Or to filter specific modules:

jq -r '.warnings | map(select(.errorCode == "UnusedName" and .moduleName == "Halogen.VDom.DOM"))' errors.json

I've updated a bunch of cases here, there were quite a few issues

• Module self-exports were reporting everything as unused
• Let bindings with patterns let { x } = e were just broken
• Object/array literals as @natefaubion said
• The operator part of binary operator expressions

Looking at that package set now, I'm only finding genuine unused issues - including a surprising number of declarations that are not used or exported :)

Improvements that could be made

• Source span of unused declarations is the whole file, should at least be the decl

btw @JordanMartinez I have a different spago version apparently but I used this

spago list-packages | awk '{print $1}' | xargs spago install

Rebased on master and fixed golden tests

Updated to avoid warning on variables with _ prefix, this is a common pattern from many languages, discussion on #purescript seemed entirely positive in that regard.

Just curious, what sort of situation is that useful in? We tend to prefix our foreign imports with _ and I'd want the use of those to be considered. I can't really think of a reason I'd want to bypass this check personally. I'm sure there is a reason for it, just not one I've encountered myself.

@garyb the change I just pushed only makes that distinction for locally bound identifiers, rather than top level declarations, I believe, so should not affect the foreign import case. For those top-level decls I don't really see any possible reason to distinguish - why would you want to define a function or import a foreign and not use it, I can only imagine it is dead or intended to be used in future, and a warning is appropriate.

The situation tends to be when pattern matching on multiple arguments, it's often clearer to see what is being ignored rather than seeing _ everywhere. Often when using multiple function heads and ignoring certain arguments in some of them, or passing a lambda which happens to ignore an argument - obviously it's a stylistic choice and depends on the code you write, when running these warnings over an actual codebase I came across a bunch of instances where I would best just replace a discarding binding with _, but others in which I felt the name improves clarity.

The leading underscore is a convention in a number of languages/compilers/linters - e.g. Erlang, Haskell (GHC), Rust, TypeScript, Ruby (Rubocop)

Thanks for the explanation!

Just looking through my open prs - the build failure on this is a timeout - I expect only worth kicking it when looking to merge this.

Looks great! I have a few (very minor) comments and then I would be happy to merge this.

I'm going to push a commit updating the changelog and then merge this. Thanks!

FYI, these two error docs currently lead to 404

• https://github.com/purescript/documentation/blob/master/errors/UnusedName.md
• https://github.com/purescript/documentation/blob/master/errors/UnusedDeclaration.md