diff --git a/README.md b/README.md index 88f3e53..85dc607 100644 --- a/README.md +++ b/README.md @@ -73,6 +73,10 @@ server (or that users will lose data). If your server simply does something differently from the reference nREPL implementation, you'll get a warning instead. +Some of the checks send what a particular client sends, the way it sends +it, e.g. `cider.connect` sends the requests CIDER sends while connecting. That way +the report tells you directly whether CIDER will work with your server. + ## Checking Clients If you're working on a client, `proof proxy` can sit between it and a @@ -123,16 +127,17 @@ the replies). `-like jank` turns on all of jank's scenarios at once, and proof is still in its early days. Right now it covers the core of the protocol (`describe`, unknown ops, `eval`, sessions, `stdin` and the wire format), -along with the requests clients send and the server differences clients -have to deal with, and `proof list` will show you all the checks. +what CIDER sends while connecting and evaluating code, the requests +clients send and the server differences clients have to deal with, and +`proof list` will show you all the checks. Here's what's coming next: - checks for `interrupt`, `load-file`, `completions` and `lookup` - robustness checks (malformed messages, fields of the wrong type, clients disconnecting in the middle of an evaluation) -- replaying what real clients send (e.g. when CIDER or Calva connect to a - server) as client profiles +- client profiles for more clients (e.g. Calva, Conjure and + vim-fireplace) - publishing the compatibility matrix somewhere nicer than a CI job summary diff --git a/doc/design.md b/doc/design.md index 613e18d..f836a19 100644 --- a/doc/design.md +++ b/doc/design.md @@ -116,6 +116,24 @@ This split keeps the regular checks simple. A check doesn't have to verify that every response has the right `id`, for instance, as the wire checks take care of that for all of them. +## Client Profiles + +The checks above ask each question in the simplest way possible, which +doesn't tell you much about the requests real clients send. CIDER, for +instance, sends the file, line and column of the code it evaluates, +along with a bunch of options for printing the result (one of them a +nested dict, another an empty list). A server that can't handle any of +those breaks CIDER, even if it passes every other check. + +Client profiles fill that gap. A client profile says what one client +sends in a few situations (e.g. while connecting) and what the client +needs from each reply, along with a link to the client code that needs +it. Each situation becomes a check that sends those requests, the way +the client sends them, and fails at the first reply the client couldn't +use. Replies that only bother the client (e.g. an error the client just +shows to the user) get a note. Right now there's a profile for CIDER, +built from what CIDER sends with its default settings. + ## Strict About the Wire, Relaxed About the Rest proof has its own bencode implementation, as the popular Go libraries @@ -236,7 +254,7 @@ Here's how the codebase is organized: ``` cmd/proof the command-line interface internal/report text and JSON reports, the compatibility matrix -internal/checks the checks, the wire checks, the client rules and a fake server for testing them +internal/checks the checks, the wire checks, the client rules, the client profiles and a fake server for testing them internal/proxy forwarding the traffic between a client and a server internal/serve a server for client tests, which acts like other servers on request internal/clients accepting clients and recording what they say, for proxy and serve @@ -289,10 +307,9 @@ what's planned next: support them) - robustness checks (malformed messages, fields of the wrong type, clients disconnecting in the middle of an evaluation) -- client profiles that replay what specific clients send (e.g. when CIDER - or Calva connect to a server), so a report can tell you - directly whether CIDER will work with your server (`proof proxy` - already sees this traffic, it just doesn't save it yet) +- client profiles for more clients (e.g. Calva, Conjure and + vim-fireplace), and a way to record them with `proof proxy`, which + already sees the traffic but doesn't save it yet - more servers in the compatibility matrix and a proper home for the matrix itself - incorporating [Spec Changes](spec-changes.md) into the spec, so that diff --git a/doc/hacking.md b/doc/hacking.md index 447c9ab..61518dc 100644 --- a/doc/hacking.md +++ b/doc/hacking.md @@ -45,7 +45,7 @@ $ bin/proof run profiles/clojure.toml | `internal/profile` | Loading and validating profiles. | | `internal/server` | Starting servers and figuring out their ports. | | `internal/check` | The checks framework (`Check`, `T`, `Rule`), grading and expected failures. It doesn't know anything about specific ops. `checktest` has helpers for testing checks. | -| `internal/checks` | The checks (`describe.go`, `op.go`, `session.go` and `eval.go`), the wire checks (`wire.go`), the client rules (`client.go`), the links to client and server code (`refs.go`), the fake server used to test all of them (`fake_test.go`) and a scripted client for testing the client rules (`client_test.go`). | +| `internal/checks` | The checks (`describe.go`, `op.go`, `session.go` and `eval.go`), the wire checks (`wire.go`), the client rules (`client.go`), the client profiles (`clients/` and `client_profiles.go`), the links to client and server code (`refs.go`), the fake server used to test all of them (`fake_test.go`) and a scripted client for testing the client rules (`client_test.go`). | | `internal/clients` | Accepting clients, recording what they say and stopping, for `proof proxy` and `proof serve`. | | `internal/proxy` | Forwarding the traffic between a client and a server, for `proof proxy`. | | `internal/serve` | The server behind `proof serve`: its piece of Clojure (`lang.go` and `eval.go`), the scenarios (`scenarios.go`), sessions (`session.go`) and the server itself (`serve.go`). | @@ -225,6 +225,49 @@ to a socket and prints the replies is all you need for that. Every client rule needs a quirk in `clientQuirks` and an entry in the table in `TestClientRulesCatchMistakes` (both in `client_test.go`). +## Adding a Client Profile + +The client profiles live in `internal/checks/clients`, one TOML file per +client, and proof reads them when it starts. Here's a check from +`cider.toml`: + +```toml +[[checks]] +id = "cider.eval" +title = "CIDER can evaluate code from a source buffer" +why = "Evaluating code from a source buffer sends ..." +needs = ["clojure"] + +[[checks.steps]] +send = { op = "clone", client-name = "CIDER", client-version = "2.1.0-snapshot" } +new-session = "repl" +why = "CIDER gives up connecting" +refs = ["nrepl-client.el#L749-L760"] +``` + +Each step sends a request and says what the client needs from the reply +besides `done`: + +| Option | What the reply needs | +|---|---| +| `new-session` | A `new-session`, which later steps can use as `$` and this name (e.g. `session = "$repl"`). | +| `snippet` | The value of this snippet of the server's profile, with all the parts it came in joined together. Its code goes in the request, as it stands for the user's code. | +| `dicts` | These fields have to be dicts (or empty lists), if the reply has them. Each one is a list of keys, e.g. `["versions", "clojure"]`. | + +`why` says what happens in the client when the reply doesn't have what +the step needs, and `refs` point at the client code in question, +starting from the profile's `code` (which is pinned to a commit, just +like the links in `refs.go`). A check that sends code in some language +should list the capability for it in `needs`, unless the client sends +that code to any server (like CIDER's startup code). + +To find out what a client sends, run it through `proof proxy` with `-v`, +which shows every request. Then read the client's code to see what it +does with each reply, and keep only what the client really needs. +`TestClientProfiles` makes sure a profile hangs together, and the fake +server should get a quirk for anything a profile catches that the other +checks don't. + ## Adding a Scenario The scenarios of `proof serve` live in `internal/serve/scenarios.go`. diff --git a/doc/profiles.md b/doc/profiles.md index bddf423..7e67487 100644 --- a/doc/profiles.md +++ b/doc/profiles.md @@ -109,6 +109,7 @@ capability your profile doesn't declare are skipped. | Capability | When to set it | Checks that need it | |---|---|---| | `namespaces` | Your language has a notion of a current namespace that can be set with the `ns` field of a request (like Clojure). | `eval.ns`, `eval.unknown-ns` | +| `clojure` | Your server evaluates Clojure, or a dialect close enough that the code Clojure clients send (e.g. `ns` forms) runs. | `cider.eval` | ## Snippets @@ -119,7 +120,7 @@ snippet is missing, all the checks that need it are skipped. | Snippet | Options | What it should do | Checks that use it | |---|---|---|---| -| `value` | `code`, `value` | evaluate to `value` | `eval.value`, `eval.survives-error`, `eval.ns`, `eval.unknown-ns`, `session.ephemeral`, `session.unknown`, `session.closed` | +| `value` | `code`, `value` | evaluate to `value` | `eval.value`, `eval.survives-error`, `eval.ns`, `eval.unknown-ns`, `session.ephemeral`, `session.unknown`, `session.closed`, `cider.eval`, `cider.repl` | | `stdout` | `code`, `out` | print `out` to the standard output | `eval.stdout`, `eval.stdout-order` | | `stderr` | `code`, `err` | print `err` to the standard error | `eval.stderr` | | `throw` | `code` | raise an error | `eval.error-status`, `eval.error-report`, `eval.survives-error` | diff --git a/doc/usage.md b/doc/usage.md index 4da4c04..bec8bb9 100644 --- a/doc/usage.md +++ b/doc/usage.md @@ -89,6 +89,16 @@ eval see: nrepl/nrepl#147 https://github.com/nrepl/nrepl/issues/147 ``` +The checks in the `cider` group send what CIDER sends while connecting, +evaluating code from a source buffer and evaluating code in its REPL, +with all the extra fields CIDER puts in its requests. If one of them +fails, CIDER won't work properly with your server, and the report says +what CIDER does with the reply it got (e.g. "CIDER gives up +connecting"). The code they evaluate for the user is the `value` +snippet of your profile. Before evaluating code from a source buffer, +CIDER evaluates the buffer's `ns` form, so `cider.eval` also needs the +`clojure` capability. + Each check gets one of the following verdicts: | Verdict | Meaning | diff --git a/internal/checks/all.go b/internal/checks/all.go index e00cfbe..606fddc 100644 --- a/internal/checks/all.go +++ b/internal/checks/all.go @@ -10,5 +10,6 @@ func All() []*check.Check { all = append(all, sessionChecks()...) all = append(all, evalChecks()...) all = append(all, stdinChecks()...) + all = append(all, clientProfileChecks()...) return all } diff --git a/internal/checks/checks_test.go b/internal/checks/checks_test.go index 3d74908..5cd2ddd 100644 --- a/internal/checks/checks_test.go +++ b/internal/checks/checks_test.go @@ -14,7 +14,7 @@ import ( var fakeProfile = &profile.Profile{ Name: "fake", Timeout: time.Second, - Capabilities: map[string]bool{"namespaces": true}, + Capabilities: map[string]bool{"namespaces": true, "clojure": true}, Snippets: map[string]profile.Snippet{ "value": {Code: "value", Value: "3"}, "stdout": {Code: "stdout", Out: "proof"}, @@ -28,9 +28,14 @@ var fakeProfile = &profile.Profile{ } func runFake(t *testing.T, q quirks) map[string]check.Result { + t.Helper() + return runFakeChecks(t, q, All(), WireRules()...) +} + +func runFakeChecks(t *testing.T, q quirks, checks []*check.Check, rules ...*check.Rule) map[string]check.Result { t.Helper() env := &check.Env{Profile: fakeProfile, Addr: startFake(t, q), Settle: 20 * time.Millisecond} - return checktest.ByID(check.Run(env, All(), WireRules())) + return checktest.ByID(check.Run(env, checks, rules)) } func TestWellBehavedServerPassesEverything(t *testing.T) { @@ -47,17 +52,20 @@ func TestChecksCatchMisbehaviour(t *testing.T) { want map[string]check.Verdict }{ {"ops as a list", quirks{opsList: true}, map[string]check.Verdict{"describe.ops-dict": F, "describe.required-ops": S, - "stdin.need-input": S, "stdin.roundtrip": S, "stdin.eof": S}}, + "stdin.need-input": S, "stdin.roundtrip": S, "stdin.eof": S, "cider.connect": F}}, {"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}, 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}}, + "stdin.need-input": S, "stdin.roundtrip": S, "stdin.eof": S, "cider.connect": F}}, {"no unknown-op", quirks{noUnknownOp: true}, map[string]check.Verdict{"op.unknown": F, "op.unknown-echo": W}}, {"no op echo", quirks{noOpEcho: true}, map[string]check.Verdict{"op.unknown-echo": W}}, {"status is a string", quirks{statusString: true}, map[string]check.Verdict{ "op.unknown": F, "op.unknown-echo": F, "wire.status-type": 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}, map[string]check.Verdict{ + "cider.connect": F, "cider.eval": F, "cider.repl": F}}, {"any session accepted", quirks{acceptAnySession: true}, map[string]check.Verdict{"session.unknown": F, "session.closed": F}}, {"shared session state", quirks{sharedState: true}, map[string]check.Verdict{"session.isolated": F}}, {"sessions tied to sockets", quirks{socketSessions: true}, map[string]check.Verdict{"session.across-connections": W}}, @@ -79,7 +87,7 @@ func TestChecksCatchMisbehaviour(t *testing.T) { "wire.id": W, "eval.stdout": F, "eval.stderr": F, "eval.error-report": W}}, {"integer value", quirks{intValue: true}, map[string]check.Verdict{ "wire.field-types": F, "eval.value": F, "eval.survives-error": F, "eval.multiple-forms": F, - "session.ephemeral": F, "session.persistent": F}}, + "session.ephemeral": F, "session.persistent": F, "cider.eval": F, "cider.repl": F}}, {"unsorted keys", quirks{unsortedKeys: true}, map[string]check.Verdict{"wire.canonical": W}}, {"invalid UTF-8", quirks{badUTF8: true}, map[string]check.Verdict{"wire.utf8": W, "eval.stdout": F}}, {"no stdin op", quirks{noStdinOp: true}, map[string]check.Verdict{ diff --git a/internal/checks/client_profiles.go b/internal/checks/client_profiles.go new file mode 100644 index 0000000..8559051 --- /dev/null +++ b/internal/checks/client_profiles.go @@ -0,0 +1,190 @@ +package checks + +import ( + "embed" + "fmt" + "slices" + "strings" + "sync" + + "github.com/BurntSushi/toml" + "github.com/nrepl/proof/internal/check" + "github.com/nrepl/proof/nrepl" +) + +// Client profiles live in clients/, one per client. Each of their checks +// sends the requests a client sends in some situation (e.g. while +// connecting), the way the client sends them, and fails at the first +// reply the client couldn't use. +// +//go:embed clients/*.toml +var clientFiles embed.FS + +type clientProfile struct { + Name string `toml:"name"` + // Code is where links to the client's code start, at the commit the + // profile was written against. + Code string `toml:"code"` + Checks []clientCheck `toml:"checks"` +} + +type clientCheck struct { + ID string `toml:"id"` + Title string `toml:"title"` + Why string `toml:"why"` + // Needs are the capabilities the server's profile has to declare, + // e.g. "clojure" for checks that send Clojure code. + Needs []string `toml:"needs"` + Steps []clientStep `toml:"steps"` +} + +// clientStep is a request and what the client needs from its reply, +// besides done. +type clientStep struct { + // Send is the request. A string starting with $ stands for the + // session an earlier step got. + Send request `toml:"send"` + // Snippet names a snippet of the server's profile whose code goes in + // the request, and whose value the reply has to have. It stands for + // the user's code. + Snippet string `toml:"snippet"` + // NewSession names the session the reply has to hand back. + NewSession string `toml:"new-session"` + // Dicts are fields that have to be dicts if the reply has them, each + // a path of keys (e.g. ["versions", "clojure"]). An empty list will do + // too, as clients can't tell the two apart. + Dicts [][]string `toml:"dicts"` + // Why says what happens in the client when the reply doesn't have + // what the step needs, and Refs point at the code that needs it. + Why string `toml:"why"` + Refs []string `toml:"refs"` +} + +// request is a request as it is, whatever fields the client puts in it. +type request map[string]any + +func (r *request) UnmarshalTOML(data any) error { + m, ok := data.(map[string]any) + if !ok { + return fmt.Errorf("a request has to be a table, not %T", data) + } + *r = m + return nil +} + +// clientProfiles reads the client profiles, which are part of proof, so a +// broken one is a bug the tests catch. +var clientProfiles = sync.OnceValue(func() []clientProfile { + files, err := clientFiles.ReadDir("clients") + if err != nil { + panic(err) + } + var profiles []clientProfile + for _, f := range files { + var p clientProfile + md, err := toml.DecodeFS(clientFiles, "clients/"+f.Name(), &p) + if err != nil { + panic(fmt.Sprintf("client profile %s: %v", f.Name(), err)) + } + if undecoded := md.Undecoded(); len(undecoded) > 0 { + panic(fmt.Sprintf("client profile %s: unknown keys %v", f.Name(), undecoded)) + } + profiles = append(profiles, p) + } + return profiles +}) + +func clientProfileChecks() []*check.Check { + var checks []*check.Check + for _, p := range clientProfiles() { + for _, c := range p.Checks { + cc := &check.Check{ID: c.ID, Title: c.Title, Severity: check.Fail, Why: c.Why, Needs: c.Needs, Run: c.replay} + for _, s := range c.Steps { + for _, path := range s.Refs { + if r := ref(p.Name+" "+path, p.Code+path); !slices.Contains(cc.Refs, r) { + cc.Refs = append(cc.Refs, r) + } + } + if s.Snippet != "" && !slices.Contains(cc.Snippets, s.Snippet) { + cc.Snippets = append(cc.Snippets, s.Snippet) + } + // Like every check that clones, so a broken clone doesn't + // make each of them wait for it. Other failures (e.g. of + // describe) still show, as they're what the client runs into. + if s.NewSession != "" { + cc.Requires = []string{"session.clone"} + } + } + checks = append(checks, cc) + } + } + return checks +} + +func (c clientCheck) replay(t *check.T) { + conn := t.Connect() + sessions := map[string]string{} + for i, s := range c.Steps { + req := nrepl.Message{} + for k, v := range s.Send { + if name, ok := v.(string); ok && strings.HasPrefix(name, "$") { + v = sessions[name[1:]] + } + req[k] = v + } + var want string + if s.Snippet != "" { + sn := t.Snippet(s.Snippet) + req["code"], want = sn.Code, sn.Value + } + step := fmt.Sprintf("step %d (%s)", i+1, req.Str("op")) + resp := t.Request(conn, req) + if resp.HasStatus("error") || resp.HasStatus("eval-error") { + t.Notef("%s got status %v", step, resp.Status()) + } + if s.NewSession != "" { + id := resp.Str("new-session") + if id == "" { + t.Stopf("%s got no new-session, so %s", step, s.Why) + } + sessions[s.NewSession] = id + } + if s.Snippet != "" { + switch got := strings.Join(resp.Values(), ""); got { + case want: + case "": + t.Stopf("%s gave no value, so %s", step, s.Why) + default: + t.Stopf("%s gave the value %q instead of %q, so %s", step, got, want, s.Why) + } + } + for _, path := range s.Dicts { + if v := nonDictField(resp, path); v != nil { + t.Stopf("%s has %s that is %s, not a dict, so %s", step, strings.Join(path, "."), typeName(v), s.Why) + } + } + } +} + +// nonDictField returns the field at path in a reply (e.g. versions, then +// clojure) unless it's a dict, an empty list or missing. +func nonDictField(resp nrepl.Response, path []string) any { + v := resp.Get(path[0]) + for _, key := range path[1:] { + d, ok := v.(map[string]any) + if !ok { + // The parent's problem, if any. + return nil + } + v = d[key] + } + switch v := v.(type) { + case nil, map[string]any: + return nil + case []any: + if len(v) == 0 { + return nil + } + } + return v +} diff --git a/internal/checks/client_profiles_test.go b/internal/checks/client_profiles_test.go new file mode 100644 index 0000000..8beeb9e --- /dev/null +++ b/internal/checks/client_profiles_test.go @@ -0,0 +1,92 @@ +package checks + +import ( + "regexp" + "strings" + "testing" + + "github.com/nrepl/proof/internal/check" + "github.com/nrepl/proof/nrepl" +) + +// The client profiles are part of proof, so they have to make sense +// before they ever reach a server. +func TestClientProfiles(t *testing.T) { + ids := map[string]bool{} + for _, c := range All() { + if ids[c.ID] { + t.Errorf("two checks are called %s", c.ID) + } + ids[c.ID] = true + } + pinned := regexp.MustCompile(`^https://github\.com/[^/]+/[^/]+/blob/[0-9a-f]{40}/`) + // The links in refs.go to the same clients. + bases := map[string]string{"CIDER": ciderBase} + for _, p := range clientProfiles() { + if p.Name == "" || !pinned.MatchString(p.Code) { + t.Errorf("%q needs a name and links pinned to a commit, got %q", p.Name, p.Code) + } + if base, ok := bases[p.Name]; ok && p.Code != base { + t.Errorf("%s links to %s, but refs.go to %s", p.Name, p.Code, base) + } + for _, c := range p.Checks { + if c.ID == "" || c.Title == "" || c.Why == "" || len(c.Steps) == 0 { + t.Errorf("%s: a check needs an id, a title, a why and steps", p.Name) + } + sessions := map[string]bool{} + for i, s := range c.Steps { + if s.Send["op"] == nil || len(s.Refs) == 0 { + t.Errorf("%s step %d: needs an op and a link to the client's code", c.ID, i+1) + } + if (s.NewSession != "" || s.Snippet != "" || len(s.Dicts) > 0) && s.Why == "" { + t.Errorf("%s step %d: doesn't say what happens to the client without what it needs", c.ID, i+1) + } + for _, v := range s.Send { + if name, ok := v.(string); ok && strings.HasPrefix(name, "$") && !sessions[name[1:]] { + t.Errorf("%s step %d: uses %s before a step gets it", c.ID, i+1, name) + } + } + if s.NewSession != "" { + sessions[s.NewSession] = true + } + } + } + } +} + +// Replies the client can live with still get a note when they say +// something went wrong. +func TestClientChecksNoteErrors(t *testing.T) { + c := clientCheck{ID: "test.errors", Title: "test", Why: "test", Steps: []clientStep{ + {Send: map[string]any{"op": "clone"}, NewSession: "s", Why: "nothing works"}, + {Send: map[string]any{"op": "eval", "code": "throw", "session": "$s"}}, + }} + r := runFakeChecks(t, quirks{}, []*check.Check{{ID: c.ID, Title: c.Title, Run: c.replay}})[c.ID] + if r.Verdict != check.Pass || len(r.Notes) != 1 || !strings.Contains(r.Notes[0], "step 2 (eval) got status [eval-error done]") { + t.Errorf("got %s with notes %q", r.Verdict, r.Notes) + } +} + +func TestNonDictField(t *testing.T) { + cases := []struct { + path []string + reply nrepl.Message + bad bool + }{ + {[]string{"versions"}, nrepl.Message{"versions": map[string]any{}}, false}, + {[]string{"versions"}, nrepl.Message{}, false}, + // Clients decode an empty list and an empty dict the same way. + {[]string{"aux"}, nrepl.Message{"aux": []any{}}, false}, + {[]string{"aux"}, nrepl.Message{"aux": []any{"current-ns"}}, true}, + {[]string{"versions", "clojure"}, nrepl.Message{"versions": map[string]any{"clojure": map[string]any{}}}, false}, + {[]string{"versions", "clojure"}, nrepl.Message{"versions": map[string]any{"clojure": "1.12.6"}}, true}, + {[]string{"versions", "clojure"}, nrepl.Message{"versions": map[string]any{}}, false}, + // That's for versions itself to report. + {[]string{"versions", "clojure"}, nrepl.Message{"versions": "1.12.6"}, false}, + } + for _, c := range cases { + if got := nonDictField(nrepl.Response{Messages: []nrepl.Message{c.reply}}, c.path); (got != nil) != c.bad { + t.Errorf("%v in %v: got %v", c.path, c.reply, got) + } + } +} diff --git a/internal/checks/clients/cider.toml b/internal/checks/clients/cider.toml new file mode 100644 index 0000000..12fe83e --- /dev/null +++ b/internal/checks/clients/cider.toml @@ -0,0 +1,118 @@ +# What CIDER sends while connecting to a server without cider-nrepl, and +# when it evaluates code, with its default settings. The links point at +# the code that needs what the steps check. +name = "CIDER" +code = "https://github.com/clojure-emacs/cider/blob/9e049baa1c2c136724d7538b1df6898ee77de6e7/lisp/" + +# CIDER sends its startup code to any server, so this check runs on servers +# for other languages too. +[[checks]] +id = "cider.connect" +title = "CIDER can connect" +why = "CIDER clones two sessions, asks for describe and evaluates its startup code before it shows a prompt, so a server that gets any of it wrong leaves CIDER users without a REPL." + +[[checks.steps]] +send = { op = "clone", client-name = "CIDER", client-version = "2.1.0-snapshot" } +new-session = "repl" +why = "CIDER gives up connecting" +refs = ["nrepl-client.el#L749-L760"] + +[[checks.steps]] +send = { op = "clone", client-name = "CIDER", client-version = "2.1.0-snapshot" } +new-session = "tooling" +why = "CIDER gives up connecting" +refs = ["nrepl-client.el#L749-L760"] + +# The REPL's banner looks up the versions of Clojure, nREPL and Java. +[[checks.steps]] +send = { op = "describe", session = "$repl" } +dicts = [["ops"], ["aux"], ["versions"], ["versions", "clojure"], ["versions", "nrepl"], ["versions", "java"]] +why = "CIDER fails whenever it looks something up in it" +refs = ["nrepl-client.el#L233-L242", "cider-session.el#L220-L242", "cider-repl.el#L361-L366"] + +# The REPL shows its prompt once the startup code is done. An eval error is +# fine: CIDER shows it and carries on. +[[checks.steps]] +refs = ["cider-repl.el#L287-L312"] + +[checks.steps.send] +op = "eval" +session = "$repl" +code = "(when-let [requires (resolve 'clojure.main/repl-requires)]\n (clojure.core/apply clojure.core/require @requires))" +file = "*cider-repl ~/project:localhost:7888(clj)*" +line = 6 +column = 1 +"nrepl.middleware.print/stream?" = "1" +"nrepl.middleware.print/print" = "cider.nrepl.pprint/clojure-pprint" +"nrepl.middleware.print/quota" = 1048576 +"nrepl.middleware.print/buffer-size" = 4096 +"nrepl.middleware.print/options" = { right-margin = 70 } +content-type = "true" +inhibit-cider-middleware = "true" + +[[checks]] +id = "cider.eval" +title = "CIDER can evaluate code from a source buffer" +why = "Evaluating code from a source buffer sends the buffer's ns form first, and then the code with its file, line and column and options for printing the value, so a server that can't handle either breaks evaluation in CIDER's source buffers." +needs = ["clojure"] + +[[checks.steps]] +send = { op = "clone", client-name = "CIDER", client-version = "2.1.0-snapshot" } +new-session = "repl" +why = "CIDER gives up connecting" +refs = ["nrepl-client.el#L749-L760"] + +# The first eval in a buffer makes sure its namespace exists, and gives up +# if that takes more than 30 seconds. Its reply isn't checked. +[[checks.steps]] +send = { op = "eval", session = "$repl", code = "(ns proof.cider)\n" } +refs = ["cider-eval.el#L800-L819"] + +[[checks.steps]] +snippet = "value" +why = "users don't see the result of their code" +refs = ["cider-eval.el#L851-L914", "cider-eval.el#L571-L576"] + +[checks.steps.send] +op = "eval" +session = "$repl" +ns = "proof.cider" +file = "/home/user/project/src/proof/cider.clj" +line = 3 +column = 1 +content-type = "true" +"nrepl.middleware.print/print" = "cider.nrepl.pprint/pr" +# An empty list is false to nREPL. +"nrepl.middleware.print/stream?" = [] +"nrepl.middleware.print/quota" = 1048576 + +[[checks]] +id = "cider.repl" +title = "CIDER's REPL can evaluate code" +why = "The REPL sends the code with its line and column and options for pretty-printing and streaming the value, so a server that can't handle them breaks CIDER's REPL." + +[[checks.steps]] +send = { op = "clone", client-name = "CIDER", client-version = "2.1.0-snapshot" } +new-session = "repl" +why = "CIDER gives up connecting" +refs = ["nrepl-client.el#L749-L760"] + +[[checks.steps]] +snippet = "value" +why = "users don't see the result of their code" +refs = ["cider-repl.el#L1390-L1429", "cider-repl.el#L1353-L1354"] + +# The namespace is user until a reply says otherwise. +[checks.steps.send] +op = "eval" +session = "$repl" +ns = "user" +file = "*cider-repl ~/project:localhost:7888(clj)*" +line = 7 +column = 7 +"nrepl.middleware.print/stream?" = "1" +"nrepl.middleware.print/print" = "cider.nrepl.pprint/clojure-pprint" +"nrepl.middleware.print/quota" = 1048576 +"nrepl.middleware.print/buffer-size" = 4096 +"nrepl.middleware.print/options" = { right-margin = 70 } +content-type = "true" diff --git a/internal/checks/eval.go b/internal/checks/eval.go index b6b1fce..11c11f0 100644 --- a/internal/checks/eval.go +++ b/internal/checks/eval.go @@ -99,8 +99,8 @@ func evalChecks() []*check.Check { ID: "eval.error-report", Title: "A failed eval explains itself in err and ex", Severity: check.Warn, - Why: "err is what users see in the REPL, and CIDER's synchronous requests look at ex and err to decide whether a request failed.", - Refs: []check.Ref{ciderEvalError, nreplEvalError}, + Why: "err is what users see when an eval fails, as clients show it like any other error output, and ex is what nREPL sends to say what was thrown.", + Refs: []check.Ref{ciderStderr, nreplEvalError}, Snippets: []string{"throw"}, Requires: []string{"session.clone"}, Run: func(t *check.T) { diff --git a/internal/checks/fake_test.go b/internal/checks/fake_test.go index b20d454..d5ecc72 100644 --- a/internal/checks/fake_test.go +++ b/internal/checks/fake_test.go @@ -50,6 +50,7 @@ type quirks struct { needInputDone bool // need-input comes with done eofError bool // an empty stdin fails the read dropStdin bool // stdin input never reaches the read + flatFields bool // a request with a dict or list in it kills the connection } type fakeSession struct { @@ -67,6 +68,8 @@ type fakeServer struct { sessions map[string]*fakeSession nextID int shared *fakeSession + // namespaces are user and the ones ns forms created. + namespaces map[string]bool } func startFake(t *testing.T, q quirks) string { @@ -75,7 +78,7 @@ func startFake(t *testing.T, q quirks) string { if err != nil { t.Fatal(err) } - s := &fakeServer{q: q, ln: ln, sessions: map[string]*fakeSession{}, shared: newFakeSession()} + s := &fakeServer{q: q, ln: ln, sessions: map[string]*fakeSession{}, shared: newFakeSession(), namespaces: map[string]bool{"user": true}} t.Cleanup(func() { ln.Close() }) go func() { for { @@ -161,6 +164,14 @@ func (s *fakeServer) session(req nrepl.Message, local map[string]bool) (*fakeSes func (s *fakeServer) handle(c net.Conn, req nrepl.Message, local map[string]bool) bool { q := s.q done := []any{"done"} + if q.flatFields { + for _, v := range req { + switch v.(type) { + case map[string]any, []any: + return false + } + } + } switch op := req.Str("op"); op { case "describe": if q.crashOnDescribe { @@ -193,8 +204,13 @@ func (s *fakeServer) handle(c net.Conn, req nrepl.Message, local map[string]bool } if q.unsortedKeys { // Hand-encoded with "status" before "id". + var session string + if sess := req.Str("session"); sess != "" { + session = "7:session" + strconv.Itoa(len(sess)) + ":" + sess + } c.Write([]byte("d6:statusl4:donee2:id" + strconv.Itoa(len(req.Str("id"))) + ":" + req.Str("id") + - "3:opsd8:describede4:evalde5:clonede5:closede5:stdindee8:versionsd4:faked14:version-string3:1.0eee")) + "3:opsd8:describede4:evalde5:clonede5:closede5:stdindee" + session + + "8:versionsd4:faked14:version-string3:1.0eee")) return true } s.send(c, req, fields) @@ -282,7 +298,13 @@ func (s *fakeServer) eval(c net.Conn, req nrepl.Message, local map[string]bool) s.send(c, req, map[string]any{"status": status}) return } - if ns := req.Str("ns"); ns != "" && ns != "user" && !q.nsFallback { + s.mu.Lock() + if name, ok := strings.CutPrefix(code, "(ns "); ok { + s.namespaces[strings.TrimRight(name, ") \n")] = true + } + known := s.namespaces[req.Str("ns")] + s.mu.Unlock() + if ns := req.Str("ns"); ns != "" && !known && !q.nsFallback { s.send(c, req, map[string]any{"status": []any{"error", "namespace-not-found", "done"}}) return } diff --git a/internal/checks/refs.go b/internal/checks/refs.go index cd69c69..f7e556b 100644 --- a/internal/checks/refs.go +++ b/internal/checks/refs.go @@ -31,8 +31,9 @@ func issue(repo string, n int) check.Ref { var ( ciderOpSupported = ref("CIDER nrepl-op-supported-p", ciderBase+"nrepl-client.el#L233-L237") ciderClone = ref("CIDER clones a main and a tooling session on connect", ciderBase+"nrepl-client.el#L740-L760") - ciderPayloadCond = ref("CIDER reads value/out/err as mutually exclusive", ciderBase+"nrepl-client.el#L875-L887") + ciderPayloadCond = ref("CIDER reads value/out/err as mutually exclusive", ciderBase+"nrepl-client.el#L875-L888") ciderEvalError = ref("CIDER eval-error handling", ciderBase+"nrepl-client.el#L898-L899") + ciderStderr = ref("CIDER shows err as error output", ciderBase+"nrepl-client.el#L887-L888") ciderDone = ref("CIDER completes requests on done", ciderBase+"nrepl-client.el#L900-L902") ciderNsNotFound = ref("CIDER namespace-not-found handling", ciderBase+"nrepl-client.el#L945") ciderRuntime = ref("CIDER runtime detection from versions", ciderBase+"cider-session.el#L218-L291") diff --git a/profiles/babashka.toml b/profiles/babashka.toml index a190601..eaea45e 100644 --- a/profiles/babashka.toml +++ b/profiles/babashka.toml @@ -9,6 +9,7 @@ startup-timeout = "30s" [capabilities] namespaces = true +clojure = true [snippets.value] code = "(+ 1 2)" diff --git a/profiles/basilisp.toml b/profiles/basilisp.toml index a789bd6..53fb6cc 100644 --- a/profiles/basilisp.toml +++ b/profiles/basilisp.toml @@ -9,6 +9,7 @@ startup-timeout = "30s" [capabilities] namespaces = true +clojure = true [snippets.value] code = "(+ 1 2)" diff --git a/profiles/clojure-clr.toml b/profiles/clojure-clr.toml index bcde1a5..8cb3e0d 100644 --- a/profiles/clojure-clr.toml +++ b/profiles/clojure-clr.toml @@ -18,6 +18,7 @@ startup-timeout = "180s" [capabilities] namespaces = true +clojure = true [snippets.value] code = "(+ 1 2)" diff --git a/profiles/clojure.toml b/profiles/clojure.toml index 646f407..6559075 100644 --- a/profiles/clojure.toml +++ b/profiles/clojure.toml @@ -9,6 +9,7 @@ startup-timeout = "120s" [capabilities] namespaces = true +clojure = true [snippets.value] code = "(+ 1 2)" diff --git a/profiles/jank.toml b/profiles/jank.toml index 25ed640..3abd2bb 100644 --- a/profiles/jank.toml +++ b/profiles/jank.toml @@ -10,6 +10,7 @@ startup-timeout = "120s" [capabilities] namespaces = true +clojure = true [snippets.value] code = "(+ 1 2)"