warn of unused variables

[kostmo]

Closes #863.

Implements unused variable checking for Let, Lambda, and Bind.

Also:

• Now distinguishes between parsing errors and warnings via red vs. yellow squigglies in VS Code.
• Added unit tests

[byorgey]

I think this whole thing can be simplified quite a bit.

1. If the parser does not already give us a SrcLoc for each variable, it absolutely should. We should fix that rather than having a convoluted and error-prone toErrPos function.
2. All the stuff about downward vs upward scopes seems much too complicated, and possibly incorrect. def and bind do not really scope "up" in any way that I can make sense of, they just scope over binds nested further to the right. Note also that it is not allowed to have a def anywhere except at the topmost level, for example it is not allowed to have (def a = ... end; def b = ... end); ... so we don't have to worry about getting defined names from a "nested" bind like in that example. EDIT: this was wrong, sorry! See my later comment.

Note that we already have a traversal fvT defined in Swarm.Language.Syntax which will fetch all the free variables in a term. However, we need something slightly more general here since we need all the free variable occurrences, with their SrcLoc. Actually in the annotate-ast branch I had to generalize fvT to traverse over the Syntax nodes instead of Term, which would be perfect here. Let me see if I can extract something useful out of that as a separate PR.

If we had a way to traverse over free variables with their associated SrcLoc, then in theory this should be as easy as just listing them and turning each one into an unused var warning.

EDIT: sorry, I was confused. It's not free variables we want (i.e. variable occurrences with no binder), but the opposite (variable bindings with no occurrences). It should still be possible to simplify this but I'll have to think about it a bit more.

[kostmo]

def and bind do not really scope "up" in any way that I can make sense of, they just scope over binds nested further to the right.

In my first pass at implementing this, I got the sense that Def and Bind were somehow inherently different than Let and Lambda in the way they would need to be checked, though the "upward" scope idea is half-baked. In reading your comment, maybe "rightward" scope is what I was thinking.

[byorgey]

I got the sense that Def and Bind were somehow inherently different than Let and Lambda in the way they would need to be checked

Of course the scoping rules are a bit different, but fundamentally they should not need to be handled in a different way.

I am imagining something like this (pseudocode):

data VarBinding = VB Var SrcLoc

-- | Takes the set of currently in-scope variables and a term, and returns the set of all variable occurrences in the term.
varOccs :: Map Var VarBinding -> Syntax -> Set VarBinding
varOccs inScope (Syntax l t) = case t of
  TUnit -> S.empty
  ...
  TVar x -> inScope ! x  -- in theory this lookup can't fail but in practice we want to handle it more safely than (!)
  SLam x xloc _ s -> varOccs (M.insert x (VB x xLoc) inScope) s
  SApp s1 s2 -> varOccs inScope s1 `S.union` varOccs inScope s2
  ...
  ... etc.

This is assuming that we enhance the parser to store e.g. the SrcLoc of the variable bound by a lambda in the SLam constructor (the xloc above).

EDIT: then, after getting varOccs, we can get all the variable bindings in the whole term (just using a simple type-based generic traversal, should be one line of code) and delete the set of bindings with occurrences to get the bindings with no occurrences.

EDIT: I removed a case for SDef in the code above, because I don't think we should warn about unused defs. See my comment later.

[byorgey]

@kostmo let me know whether/what kind of additional help you'd like with this.

[byorgey]

Chatting with @xsebek in IRC helped me realize I was wrong about nested def not being allowed. It is allowed, and in fact must be allowed if we want importing to work. HOWEVER, I would argue that we should not warn about unused defs, precisely because def defines global names which can scope past the end of the expression/file containing them. It is going to be very common to define a .sw file full of defs, where some of the defined names are used in others, but many of them are simply there so that you can import the .sw file and use them at the REPL. Hence a normal .sw file is going to be full of "unused" names defined via def and it would be really weird and annoying to warn about them. Just like how Haskell does not warn you about "unused" top-level definitions (unless you make an explicit export list; then it will warn you about names which are not used in the module and also not exported).

let, bind, and lambdas, on the other hand, all define names in a local scope that is easy to determine syntactically. It should be possible to handle them all in essentially the same way, using an approach like the pseudocode I wrote in a comment above.

For more context about the difference between def vs everything else, see also my comment at #997 (comment) .

[kostmo]

@byorgey this is now working for let, lambda, and bind, and unit tests have been added.

[byorgey]

This is looking really nice! Just a few remaining issues + some new edge cases for the test suite.

[kostmo]

All feedback addressed. Added 4 more tests.

[byorgey]

Looks great!

[kostmo]

@Mergifyio refresh

[mergify]

refresh

✅ Pull request refreshed