Check what clients need from completions and lookups

CHANGELOG.md

@@ -32,3 +32,5 @@ Initial release.
   scenarios for nREPL's interrupt bugs in `proof serve`.
 - [#12](https://github.com/nrepl/proof/pull/12): Checks for `load-file`, and
   `load-file` in `proof serve`.
+- [#13](https://github.com/nrepl/proof/pull/13): Checks for completions and
+  lookups, and nREPL's `completions` and `lookup` in `proof serve`.

PLAN.md

@@ -101,14 +101,17 @@ lookups and completions, plus requests that go wrong.
 
 ### Lookups and Completions
 
-- Checks for nREPL's `lookup` and `completions` and for cider-nrepl's
-  `info`, `complete` and `eldoc`, for servers that list them in
-  `describe`. nREPL has the former, Basilisp the latter and Babashka
-  both. CIDER asks for cider-nrepl's ops by their `cider/` names, falls
-  back to the old names and then to nREPL's ops, and the checks should
-  do the same.
-- Client profile checks for the completion and documentation requests of
-  CIDER and Calva.
+- Done: checks for nREPL's `completions` and `lookup` and cider-nrepl's
+  `complete`, `info` and `eldoc`, for servers that list them, sending
+  CIDER's `prefix` and `sym` and the `symbol` of Calva, Conjure and
+  vim-fireplace. The checks of `completions` and `lookup` skip on
+  servers that have `complete` and `info`, as clients use those then.
+  Babashka, Basilisp and ClojureCLR don't say `no-info` for a symbol
+  that doesn't exist, Basilisp counts columns from 0 and ClojureCLR's
+  `file` isn't a URI, which Calva needs to jump to a definition.
+- Done: `proof serve` has nREPL's `completions` and `lookup`.
+- Not done: scenarios for cider-nrepl's ops in `proof serve`, so `-like`
+  could act like Babashka, Basilisp and ClojureCLR there too.
 - [spec#9](https://github.com/nrepl/spec.nrepl.org/pull/9) (`arglists-str`
   and `no-info`) should be settled by then.
 

README.md

@@ -131,14 +131,14 @@ the replies). `-like jank` turns on all of jank's scenarios at once, and
 ## Status
 
 proof is still in its early days. Right now it covers the core of the
-protocol (`describe`, unknown ops, `eval`, sessions, `stdin`, `interrupt`, `load-file` and the wire format),
+protocol (`describe`, unknown ops, `eval`, sessions, `stdin`, `interrupt`, `load-file`, completions, lookups and the wire format),
 what CIDER, Calva, Conjure, vim-fireplace, REPLy and neorepl send while
 connecting, evaluating code and loading files, the requests
 clients send and the server differences clients have to deal with, and
 `proof list` will show you all the checks.
 
-Checks for lookups, completions and malformed requests are coming next,
-along with the first release.
+Checks for malformed requests are coming next, along with the first
+release.
 [PLAN.md](PLAN.md) has all the details.
 
 ## Why "proof"?

doc/design.md

@@ -193,8 +193,9 @@ into, and the tests make sure it stays that way. proof runs its own
 checks against every scenario, and a scenario has to get the same
 verdicts as the servers it names. `-like` turns on all the scenarios of a
 server, and together they have to get that server's column of the matrix
-(apart from `eval.no-code`, which no client sends, and the checks that
-come down to the server's language).
+(apart from `eval.no-code`, which no client sends, the checks that come
+down to the server's language and those of completions and lookups,
+which are nREPL's in proof serve).
 
 The grading works the other way around here. proof can't see what a
 client shows its users, so the client's own tests have to do that. Most
@@ -306,6 +307,7 @@ to have a single test suite that every server can be checked against.
 ## Future Plans
 
 At this point proof covers the core of the protocol (`describe`, unknown
-ops, sessions, `eval`, `stdin`, `interrupt`, `load-file` and the wire format), the requests of
+ops, sessions, `eval`, `stdin`, `interrupt`, `load-file`, completions,
+lookups and the wire format), the requests of
 clients and the server differences clients have to deal with.
 [PLAN.md](../PLAN.md) has what's planned next.

doc/hacking.md

@@ -134,8 +134,8 @@ Here's what each of its fields means:
 | `Requires` | The checks this check depends on. Everything that creates a session requires `session.clone`, and everything that looks at the response of `describe` requires `describe.reply`. |
 
 Checks are grouped by area in `describe.go`, `op.go`, `session.go`,
-`eval.go`, `stdin.go` and `interrupt.go`, and they run in the order
-listed in `all.go`. If you're adding
+`eval.go`, `stdin.go`, `interrupt.go`, `loadfile.go`, `completion.go`
+and `lookup.go`, and they run in the order listed in `all.go`. If you're adding
 a new area, put its checks in a new file and add them to `all.go`.
 
 In `Run` a check talks to the server via `t`:

doc/profiles.md

@@ -65,6 +65,13 @@ eof = "nil"
 code = '(do (print "proof") (flush) (Thread/sleep 60000))'
 out = "proof"
 
+[snippets.completion]
+code = "printl"
+value = "println"
+
+[snippets.lookup]
+code = "println"
+
 [expected-failures]
 "eval.multiple-forms" = "only the last value is returned, see #42"
 ```
@@ -139,6 +146,8 @@ snippet is missing, all the checks that need it are skipped.
 | `multiple` | `code`, `values` | consist of several forms that evaluate to `values` (in order) | `eval.multiple-forms` |
 | `read-line` | `code`, `value`, `eof` | read a line from stdin, evaluating to `value` when the line is `proof` and to `eof` at the end of input | `stdin.need-input`, `stdin.roundtrip`, `stdin.eof` |
 | `sleep` | `code`, `out` | print `out` and then keep running for longer than the `timeout` (e.g. a minute), so there's something to interrupt | all the `interrupt.*` checks |
+| `completion` | `code`, `value` | `code` is the start of a name, which completes to `value` | all the `completion.*` checks |
+| `lookup` | `code` | `code` is the name of a function | `lookup.lookup`, `lookup.info`, `lookup.eldoc` |
 
 A few tips for writing snippets:
 
@@ -166,6 +175,9 @@ A few tips for writing snippets:
 
 - proof evaluates the `use` of `session-state` in a new session as well,
   where it must not return `value` (an error is fine).
+- The checks of completions and lookups evaluate `value` first, and send
+  the `ns` that comes with its value (if any) along with their requests,
+  just like clients send the namespace of the code they complete.
 - proof interrupts `sleep` as soon as its `out` arrives, so make sure
   your server sends the output right away (in Clojure that takes a
   `flush`, as there's no newline).

doc/spec-changes.md

@@ -252,6 +252,22 @@ checks `lookup-error`), so omitting `info` works for it. Conjure's
 `(or msg.info msg)` returns the whole message. The spec should adopt
 `no-info`.
 
+cider-nrepl's `info` (which the spec doesn't have, but Babashka,
+Basilisp and ClojureCLR do) is a different matter. CIDER treats any
+reply without `no-info` as the info about a symbol it found, so a plain
+`done` for a symbol that doesn't exist breaks its docs and jumping to a
+definition. Babashka, Basilisp and ClojureCLR all do that today.
+
+### Completions and Lookups
+
+The spec has `completions` and `lookup`, but it doesn't say that the
+completions should come in one message along with `done` (Conjure reads
+them from the message with `done`, and Calva from the first message of
+the reply to `complete`), that each candidate is a dict with a
+`candidate`, that `line` and `column` in `info` are integers counting
+from 1 (Conjure moves the cursor to `column - 1`), or that `file` is a
+URI (Calva can't jump to a plain path).
+
 ### `status` is a Set, but Some Clients Compare Lists
 
 `nrepl.cmdline` expects exactly `["done" "session-idle"]` when

doc/usage.md

@@ -390,6 +390,8 @@ servers. There are a couple of exceptions. No scenario covers
 `eval.no-code`, as clients always send some code, and the code is still
 Clojure, even with `-like dialtone`. So the checks the profiles of
 dialtone and repartee skip for their languages (e.g. `eval.ns`) pass.
+And the completions and lookups of `proof serve` are always nREPL's
+(`completions` and `lookup`), whatever server it acts like.
 
 Most scenarios change only how the replies look on the wire, not what a
 user should end up seeing. Evaluating `(println "hi") (+ 1 2)` should
@@ -428,7 +430,9 @@ gets a read error. That's enough for CIDER, Calva, Conjure and
 vim-fireplace to connect and work, and it's all you need for checking
 output, values, errors, input and interrupts. `load-file` evaluates
 what's in the file the same way, and like nREPL it sends only the value
-of the last form and stops at the first error.
+of the last form and stops at the first error. `completions` and
+`lookup` know the functions in the table above and whatever the code
+defines.
 
 When you stop it, `proof serve` checks the requests your client sent,
 just like `proof proxy` does, with the same report, exit codes and

internal/checks/all.go

@@ -12,6 +12,8 @@ func All() []*check.Check {
 	all = append(all, stdinChecks()...)
 	all = append(all, interruptChecks()...)
 	all = append(all, loadFileChecks()...)
+	all = append(all, completionChecks()...)
+	all = append(all, lookupChecks()...)
 	all = append(all, clientProfileChecks()...)
 	return all
 }

internal/checks/checks_test.go

@@ -27,6 +27,8 @@ var fakeProfile = &profile.Profile{
 		"multiple":      {Code: "1 2", Values: []string{"1", "2"}},
 		"read-line":     {Code: "read", Value: `"proof"`, Eof: "nil"},
 		"sleep":         {Code: "sleep", Out: "proof"},
+		"completion":    {Code: "pro", Value: "proof"},
+		"lookup":        {Code: "proof"},
 	},
 }
 
@@ -41,8 +43,30 @@ func runFakeChecks(t *testing.T, q quirks, checks []*check.Check, rules ...*chec
 	return checktest.ByID(check.Run(env, checks, rules))
 }
 
+// skipped is what checks get from a server that doesn't have their op.
+func skipped(checks ...[]*check.Check) map[string]check.Verdict {
+	v := map[string]check.Verdict{}
+	for _, cs := range checks {
+		for _, c := range cs {
+			v[c.ID] = check.Skipped
+		}
+	}
+	return v
+}
+
+var (
+	// The fake has both cider-nrepl's complete and info and nREPL's
+	// completions and lookup, which clients only use without the former.
+	fallbacks = map[string]check.Verdict{"completion.completions": check.Skipped, "lookup.lookup": check.Skipped,
+		"lookup.lookup-unknown": check.Skipped}
+	// With nREPL's ops alone, it's the other way around.
+	nreplLookups = checktest.Merged(skipped(completionChecks(), lookupChecks()), map[string]check.Verdict{
+		"completion.completions": check.Pass, "lookup.lookup": check.Pass, "lookup.lookup-unknown": check.Pass})
+)
+
 func TestWellBehavedServerPassesEverything(t *testing.T) {
-	checktest.Verdicts(t, runFake(t, quirks{}), nil)
+	checktest.Verdicts(t, runFake(t, quirks{}), fallbacks)
+	checktest.Verdicts(t, runFake(t, quirks{nreplLookups: true}), nreplLookups)
 }
 
 // Each misbehaviour must produce exactly the listed verdicts, and every
@@ -52,15 +76,9 @@ func TestChecksCatchMisbehaviour(t *testing.T) {
 	// What every client but REPLy goes through first.
 	connects := map[string]check.Verdict{"cider.connect": F, "calva.connect": F, "conjure.connect": F, "fireplace.connect": F,
 		"neorepl.connect": F}
-	// The interrupt checks, which can't tell whether the server has the op.
-	noInterrupts := map[string]check.Verdict{}
-	for _, c := range interruptChecks() {
-		noInterrupts[c.ID] = S
-	}
-	noLoadFile := map[string]check.Verdict{}
-	for _, c := range loadFileChecks() {
-		noLoadFile[c.ID] = S
-	}
+	// The checks that can't tell whether the server has their op.
+	noInterrupts, noLoadFile := skipped(interruptChecks()), skipped(loadFileChecks())
+	noLookups := skipped(completionChecks(), lookupChecks())
 	// What CIDER, Calva and Conjure send to load a file.
 	clientsLoad := map[string]check.Verdict{"cider.load-file": F, "calva.load-file": F, "conjure.load-file": F}
 	cases := []struct {
@@ -69,11 +87,11 @@ func TestChecksCatchMisbehaviour(t *testing.T) {
 		want map[string]check.Verdict
 	}{
 		// Conjure does without the features that need ops.
-		{"ops as a list", quirks{opsList: true}, checktest.Merged(connects, noInterrupts, noLoadFile, map[string]check.Verdict{"describe.ops-dict": F,
+		{"ops as a list", quirks{opsList: true}, checktest.Merged(connects, noInterrupts, noLoadFile, noLookups, map[string]check.Verdict{"describe.ops-dict": F,
 			"describe.required-ops": S, "stdin.need-input": S, "stdin.roundtrip": S, "stdin.eof": S, "conjure.connect": check.Pass})},
 		{"clone not advertised", quirks{noClone: true}, map[string]check.Verdict{"describe.required-ops": F}},
 		{"no versions", quirks{noVersions: true}, map[string]check.Verdict{"describe.versions": W}},
-		{"describe kills the connection", quirks{crashOnDescribe: true}, checktest.Merged(connects, noInterrupts, noLoadFile, map[string]check.Verdict{
+		{"describe kills the connection", quirks{crashOnDescribe: true}, checktest.Merged(connects, noInterrupts, noLoadFile, noLookups, map[string]check.Verdict{
 			"describe.reply": F, "describe.ops-dict": S, "describe.required-ops": S, "describe.versions": S,
 			"stdin.need-input": S, "stdin.roundtrip": S, "stdin.eof": S})},
 		{"no unknown-op", quirks{noUnknownOp: true}, map[string]check.Verdict{"op.unknown": F, "op.unknown-echo": W}},
@@ -83,7 +101,8 @@ func TestChecksCatchMisbehaviour(t *testing.T) {
 			"op.unknown": F, "op.unknown-echo": F, "wire.status-type": F, "conjure.connect": F}},
 		{"no session-closed", quirks{noSessionClosed: true}, map[string]check.Verdict{"session.close": F}},
 		// Only clients put dicts and lists in their requests.
-		{"flat fields only", quirks{flatFields: true}, checktest.Merged(clientsLoad, map[string]check.Verdict{
+		// Conjure's complete has extra-metadata, a list.
+		{"flat fields only", quirks{flatFields: true}, checktest.Merged(clientsLoad, map[string]check.Verdict{"completion.complete-symbol": F,
 			"cider.connect": F, "cider.eval": F, "cider.repl": F, "calva.connect": F, "calva.eval": F, "conjure.eval": F,
 			"neorepl.eval": F})},
 		{"any session accepted", quirks{acceptAnySession: true}, map[string]check.Verdict{"session.unknown": F, "session.closed": F}},
@@ -145,11 +164,34 @@ func TestChecksCatchMisbehaviour(t *testing.T) {
 		{"no load-file op", quirks{noLoadFileOp: true}, checktest.Merged(noLoadFile, clientsLoad)},
 		{"load-file without a value", quirks{loadFileNoValue: true}, checktest.Merged(clientsLoad,
 			map[string]check.Verdict{"load-file.value": F})},
+		{"nREPL's lookups", quirks{nreplLookups: true}, nreplLookups},
+		{"completions before done", quirks{completionsLate: true}, map[string]check.Verdict{"completion.complete-symbol": F}},
+		{"completions before done, nREPL's", quirks{completionsLate: true, nreplLookups: true}, checktest.Merged(nreplLookups,
+			map[string]check.Verdict{"completion.completions": F})},
+		{"string candidates", quirks{stringCandidates: true}, map[string]check.Verdict{
+			"completion.complete": F, "completion.complete-symbol": F}},
+		{"string candidates, nREPL's", quirks{stringCandidates: true, nreplLookups: true}, checktest.Merged(nreplLookups,
+			map[string]check.Verdict{"completion.completions": F})},
+		{"complete takes only a prefix", quirks{completePrefix: true}, map[string]check.Verdict{"completion.complete-symbol": F}},
+		{"empty info for nothing", quirks{emptyLookupInfo: true, nreplLookups: true}, checktest.Merged(nreplLookups,
+			map[string]check.Verdict{"lookup.lookup-unknown": F})},
+		{"line as a string", quirks{lineString: true}, map[string]check.Verdict{"lookup.info": F}},
+		{"line as a string, nREPL's", quirks{lineString: true, nreplLookups: true}, checktest.Merged(nreplLookups,
+			map[string]check.Verdict{"lookup.lookup": F})},
+		{"columns from 0", quirks{columnZero: true}, map[string]check.Verdict{"lookup.info": F}},
+		{"no no-info", quirks{noNoInfo: true}, map[string]check.Verdict{"lookup.info-unknown": F}},
+		{"eldoc of strings", quirks{eldocStrings: true}, map[string]check.Verdict{"lookup.eldoc": F}},
+		// Calva can't jump to a path.
+		{"file as a path", quirks{pathFile: true}, map[string]check.Verdict{"lookup.info": F}},
+		// CIDER finds them, but Calva, Conjure and vim-fireplace don't, and
+		// Conjure falls back to completions and lookup.
+		{"cider/ ops alone", quirks{ciderPrefixed: true}, map[string]check.Verdict{"completion.complete-symbol": S,
+			"completion.completions": check.Pass, "lookup.lookup": check.Pass, "lookup.lookup-unknown": check.Pass}},
 	}
 	for _, c := range cases {
 		t.Run(c.name, func(t *testing.T) {
 			t.Parallel()
-			checktest.Verdicts(t, runFake(t, c.q), c.want)
+			checktest.Verdicts(t, runFake(t, c.q), checktest.Merged(fallbacks, c.want))
 		})
 	}
 }