Make orchard.namespace/classpath-namespaces resilient to faulty ns declarations

[mk]

Closes #158

Before submitting a PR make sure the following things have been done:

• The commits are consistent with our contribution guidelines
• You've added tests to cover your change(s)
• All tests are passing
• The new code is not generating reflection warnings
• You've updated the changelog (if adding/changing user-visible functionality)

[vemv]

I think the proposal SGTM, thanks!

Perhaps we were intentionally encouraging that namespaces would be correct at every moment - it's part of CIDER's approach to have loadable code that can be run (and not only parsed).

But it's always possible that some 3rd party library could accidentally include some faulty unused namespace, which isn't something users can easily fix. So it's more friendly to give users something usable, than not.

[vemv]

(ns foo (:requires [clojure.string])) is a also a good example to be covered.

Accordingly, probably a try/catch would be better than the proposed approach - there can be few reasons why a ns parsing could fail.

[mk]

The error I've reported in #158 does not occur here, but it originates here, see:

orchard/src/orchard/namespace.clj

Lines 121 to 123 in c0e24f7

	(pmap jvm-clojure-resource-name->ns-name)
	(filter identity)
	(sort)))

What happens without my change is that we try to sort and hence compare a seq like (:requires [clojure.string]) with symbols and that fails.

I do think a try/catch could be good & helpful in addition to that, but then I'd place it way further up the stack, possibly in cider-nrepl in analyze-stacktrace https://github.com/clojure-emacs/cider-nrepl/blob/5c0f21197fcccb1b2ca67054cab1dcc8a6af2c7f/src/cider/nrepl/middleware/stacktrace.clj#L216

Btw, re-reading the issue I'm noticing now I should have pointed out the stacktrace in https://gist.github.com/mk/bd82092048d733ef3de0e312898f47b4 more promintely.

[mk]

Perhaps we were intentionally encouraging that namespaces would be correct at every moment - it's part of CIDER's approach to have loadable code that can be run (and not only parsed).

Looking at the tests for read-namespace makes me think it's built to namespaces that cannot be read:

orchard/test/orchard/namespace_test.clj

Lines 53 to 61 in c0e24f7

	(testing "of an unparsable file"
	(spit url "(]$@(")
	(is (nil? (sut/read-namespace uri))))
	(testing "of non-list tokens"
	(spit url "these are (still) tokens")
	(is (nil? (sut/read-namespace uri))))
	(testing "when tokens precede the ns form"
	(spit url "there [is a] (ns here) after all")
	(is (= 'here (sut/read-namespace uri))))

But it's always possible that some 3rd party library could accidentally include some faulty unused namespace, which isn't something users can easily fix. So it's more friendly to give users something usable, than not.

I ran into two cases on two different project in the past week were this assumption broke. It was faulty code under our control but we didn't notice what was causing the issue or where to look for an error.

In addition to this change I think it makes sense to be more defensive in cider-nrepl and catch exceptions thrown in the stacktrace handling and warn the user about what's wrong. Let me know if you think this is a good idea and I could take a look at that as a follow-up.

[vemv]

Thanks for the insights!

In addition to this chance I think it makes sense to be more defensive in cider-nrepl

I also think so, and would prefer that we think that we handle all missing edge cases in a single PR - else it's just "git churn" that we're creating (i.e. make a change, then undo it to do something more comprehensive).

[mk]

I also think so, and would prefer that we think that we handle all missing edge cases in a single PR - else it's just "git churn" that we're creating (i.e. make a change, then undo it to do something more comprehensive).

I do think this change is good as it improves things for an issue we've experienced and I don't think I'd undo it after adding catching to cider-nrepl. I think it's also in line with the current behaviour of returning nil for namespaces that cannot be read. Those improvements will be also be a larger change with a bigger surface area. Also I can't say when I'd get around to it or if the end result will be something I'd be confident in proposing without introducing breakage.

[vemv]

Alright, respected, I'll give a look at the problem as I find a chance asap.

[jackrusher]

Delaying this PR to wait for the final comprehensive fix reminds me of:
https://en.wikipedia.org/wiki/Perfect_is_the_enemy_of_good

[vemv]

It's pretty safe to assume that most OSS maintainers are already familiar with various truisms (and their applicability to specific scenarios).

[bbatsov]

So, are we going to do something with this PR or not? I'm favor of just merging it, given that a comprehensive fix is unlikely to happen any time soon.

[vemv]

So, are we going to do something with this PR or not?

I checked out this PR again the other day and it seemed fine to me with no future tweaks needed, since we can rely on the compiler for checking unloadable code.

[bbatsov]

Good. I'm planning a new CIDER release (perhaps as soon as this week) and it'd be nice if we manage to squeeze in recent Orchard updates there.