Clojure

This ticket is an excellent example of a terrible ticket.

1) It does not lead with a problem statement. The title takes the form akin to "I want X". It proposes a tactic.

2) The description is woefully inadequate. Here too, no problem statement. Descriptions should point to any discussions, but discussions are long and rambling, and no substitute for a succinct problem statement in the description. Descriptions need to be maintained, with the current strategy and name of best patch.

3) No resolution strategy. Just patches attached. How is a reviewer supposed to assess your patch if you don't even bother stating what the problem is and what your approach will be in fixing it, how that approach does fix it, and what if any tradeoffs are being made?

4) The change being requested would be a breaking change. That should be made extremely clear in the description, and doubles the threshold for quality of motivation and implementation.

5) Any breaking change would have to carefully enumerate what would break and when, what the migration strategy would be etc.

6) The patch breaks the compiler. If you don't understand how the compiler works, and why features are there, you shouldn't submit patches that alter it. The only assessments made in comments are 'it works for me', which, while useful anecdotes, are insufficient.

While the ticket itself was bad, the uncritical rallying behind it was more troubling. This is not how Clojure was built, and will not be how it is maintained.

In the end, the ticket proposed a tactic, and that tactic has been rejected. Read safety has been addressed by other means.

https://github.com/clojure/clojure/commit/974a64c06917861b7557fd73154254195b2de548

I'm bumping the Priority field from its default value to "Major" to reflect recent events. (I personally feel "blocker" would be more appropriate, or at least "critical", but I don't want to step on any toes.)

Also removed my original didn't-fix-enough-things patch from Jan 30 2013.

clj-1153-patch-v2.txt dated Feb 2 2013 is functionally identical to Chas Emerick's CLJ-1185.patch dated Feb 1 2013, and no retesting is needed by anyone because of it.

The only change I have made to his patch is: the doc string for read-eval now says its default value is false.

Thanks to all that have tested the patch! Looks like we've made some progress. I'm going to post to the main Clojure ML now and hopefully get more yay/nay votes.

I ran the Sonian test base against this patch. I wouldn't say our test cases are comprehensive, but they're not trivial. We talk to databases through JDBC, and we deal with lots of different data types (BigInts, Strings, Dates, and such).

I had to make a few small changes to our code base, because we rely pretty heavily on print-dup to serialize non-user originated data. We have to add corresponding binding blocks when reading to set read-eval to true. Other than that, the tests all passed.

+1

New patch attached, {{CLJ-1185.patch}}.

This patch eliminates the use of *print-dup* in the compiler entirely. read-string continues to be used to support values that can only be represented via tagged reader literals; I don't think there's any way around that, since the mapping between particular data readers and values' types are buried in individual print-method implementations.

All tests pass (including cases like (eval (list + 1 2 3)) as discussed in the ML thread referenced in the issue description), and a variety of sanity checks around evaluation and compilation tooling (e.g. AOT, nREPL, introspection utilities in Leiningen/Reply/Counterclockwise) all work as expected.

Of course, if this is applied, then any usage of read over trusted content containing #= forms will need to be done with *read-eval* bound to true.

To aid in testing, a Clojure build (1.5.0-RC4) + this patch is available here:

[com.cemerick/clojure "1.5.0-SNAPSHOT"]

Note that you'll have to exclude the standard Clojure dependency from your project in order for this one to take precedence; you can do this by adding

:exclusions [org.clojure/clojure]

to your project.clj.

Fourthed. I hope folks will pitch in and help deal with any breakage.

Thirded.

For what it's worth, on the topic of breaking changes I'd much rather my old code break than continue to work with an unnoticed security hole.

Seconded!

It's worth noting that there was a breakin to rubygems.org over the weekend that was caused by what amounts to this same issue, but with YAML: https://news.ycombinator.com/item?id=5141138

The current default is both dangerous and undocumented; this needs to change.