technomancy/slamhound — pull request #53 by guns (2013-11-04 03:13:15+00:00)
Title: New ns form parser for more reliable disambiguation

Hi @technomancy,

This is a large changeset, so I am submitting this as a pull request.

Slamhound is one of my favorite Clojure tools, and one I would like to
see more widely used. Unfortunately, aside from the very large problem
of issue #19, there have been numerous issues related to disambiguation
that I think discourages users.

#24, #41, and #50 are some closed issues of this type. #26 and #27 are
still open.

In each of the addressed changes, the old ns form is used to divine user
preferences when disambiguating between candidate namespaces. However,
because `asplode/asplode` simply disassembled the old ns without parsing
it, matching old :require clauses was done with structural pattern
matching.

Namespace reference clauses like :require and :use are very flexible
and support prefix lists, multiple keyval pairs per clause, multiple
entries per namespace, and trailing flags. Pattern matching against
these clauses is therefore difficult and undependable.

I added `regrow/expand-libs` in f1f10ad7408cb80f27fc8ac64e9b48eb730a2322
as an attempt toward homogenizing libspecs for better pattern matching,
but private attempts at extending this function convinced me that it is
a dead end.

This patch series introduces a proper ns parser. The general algorithm
is the same as before, but disambiguation is now very dependable.

Here are the three main steps in light of the new parser:

### 1. asplode

`asplode/asplode` parses ns references into the following map:

```clojure
;; #'slam.hound.asplode/empty-ns-references
{:import #{} ; #{class-sym}
:require #{} ; #{ns-sym}
:alias {} ; {ns-sym ns-sym}
:refer {} ; {ns-sym #{var-sym}}
:xrefer #{} ; #{ns-sym} - exclusively referred namespaces
:refer-all #{} ; #{ns-sym}
:exclude {} ; {ns-sym #{var-sym}}
:rename {} ; {ns-sym {var-sym var-sym}}
:reload false ; true/false
:reload-all false ; true/false
:verbose false ; true/false
:load nil ; a seq of file paths
:gen-class nil ; a seq of option pairs, possibly empty
}
```

Parsing follows the logic of `clojure.core/import` and
`clojure.core/load-libs`, so all documented ways of writing ns
references are supported.

### 2. regrow

Candidate generation has not changed, except now `regrow/candidates`
returns a set of class or ns symbols that correspond to the given
:failure-type, instead of a seq of literal libspec forms
(`'([my.ns :as my] [my.other.ns :as my])`).

Since we now have the old ns as real data, `regrow/disambiguate` can do
its work more accurately, with some new enhancements:

- Imports from the old ns form are preferred (solving issue #26)

- If a missing symbol is capitalized and the old ns has a referred var
with the same name, the search order is set to `[:refer :import]` to
give it a chance to find the capitalized var (solving issue #27)

- Candidates that match vars that were excluded directly either via

`(:use [my.ns :exclude […]])`,

or implicitly excluded from `clojure.core` via

`(:refer-clojure :only […])`,

are now disjoined from the set of candidates.

The predicate functions for sorting candidates have been changed to
ranking functions that return numbers in order to express non-binary
preferences.

This is a nice way to reimplement pull request #24, which ranked vars
that were explicitly referred higher than vars that were implicitly
referred via :use or :refer :all.

I initially planned to support addition and exclusion of candidates
based on the :rename option of :use, :refer-clojure, and the
undocumented :refer clause, but I dropped it since I have never actually
seen it in use. It is easy to add in the future if a user requests it.

### 3. stitch

If the regrow step successfully augments the ns-map with a new :import,
:alias, :refer, or :refer-all, the stitch-up function now has the
responsibility of emitting a real ns-form from data.

This allows the easy generation of compact libspecs such as:

```clojure
(ns my.ns
"Docstring"
{:comment "Now supports re-emitting ns metadata!"}
(:require [my.bar :as bar :refer :all]
[my.foo :as foo :refer [a b c]]
:reload-all :verbose))
```

Notice that multiple options per spec, the flags on :require, and the
metadata map after the docstring can all be emitted.

### Changelist

Here is a high level list of all enhancements and changes for reference:

- New comprehensive ns parser

- Metadata maps in ns forms are parsed and re-emitted

- The :require flags :reload, :reload-all, and :verbose are preserved
and re-emitted

- Multiple :key value options are emitted per libspec; i.e. no separate
:as and :refer vectors.

- Refer candidates are now subject to :exclude rules from the old ns

- Excluding vars in `clojure.core` also excludes vars from `cljs.core`.
See 5e7f292d38d53bd43bac7c146d4eefb1f90d352f for more details

- The relatively expensive dynamic class search function
`regrow/ns-import-candidates` is only invoked when the faster static
class name search fails

- Disambiguation of candidates based on presence in the old ns-map now
works perfectly. This includes :imports, as well as capitalized vars
that shadow class names. Addresses #26 and #27.

- Any mass refers from the old-ns that match a top candidate during
regrow are carried over to the new ns. The only change from #50 is
that now bare `(:use my.ns)` statements also trigger this behavior.

This can be restricted again to only `[… :refer :all]` if desired.

- `cond->` and `as->` from Clojure 1.5 are backported to
`slam.hound.future` for convenience. They should be removed when
upgrading the Clojure dependency

- Unit tests for all new functions. Overall test coverage has increased,
with no known regressions.

### Final notes

The Master once said,

Programmers know the benefits of everything and the tradeoffs of
nothing.

Indeed. As this is a large patchset, I am obligated to state the
tradeoffs.

Here is what we gain:

- Candidate disambiguation works with all valid ns forms (and many
implicitly valid forms)

- The ns data used for disambiguation are maps, sets, and booleans, not
sequences of irregular Clojure forms.

The disambiguation functions in `regrow` are correspondingly easier to
write and understand.

- Better separation of concerns: ns parsing is done entirely in
`asplode`, and form construction entirely within `stitch`.

Currently, ns forms are parsed somewhat in all three namespaces, and
form construction is handled by both `regrow` and `stitch`

And here are the costs:

- +187 lines of code and comments in `slam.hound.asplode`

The old `asplode` is an easy to grok two-function namespace that
disassembles ns forms.

The new `asplode` is 12 functions + 2 constants that handle all
variations of namespace reference clauses.

- +51 lines of code and comments in `slam.hound.stitch`

~20 lines of form collapsing and sorting code is replaced by ~70 lines
of code generation from namespace data.

The new `stitch` functions have complete control over the format of
the emitted code.

Unsurprisingly, I think increasing the reliability and capability of the
disambiguation process is worth the moderate complexity of the ns parser
(which will likely go untouched until the ns macro is amended).

Slamhound is great when it DWIM, but it is infuriating when it refuses
to DWIM, even if I write exactly what I want in the ns form. These
changes are primarily directed at avoiding the latter scenario.

As always, I am eager to discuss these changes and make adjustments, so
please comment at your convenience.

guns