Rely less on undocumented features

[frenchy64]

We simply guard against nil leaking into merge or select-keys. This does not change semantics.

I caught this with a linter I'm building.

[trptcolin]

I suspect there's lots of other code out there that uses merge on nil too (I've assumed it in a bunch of my client & internal projects) - what's the linter's motivation here? Trying to guarantee a non-nil map comes out of merge?

The current merge implementation is clearly doing a lot of work to handle nils, and it seems unlikely that the language would break backwards compatibility for this. Obviously this isn't a super-big deal, but if everybody has to do this everywhere they use merge, it's going to make code noisier. So I'm curious to hear more about the reasons for this.

[frenchy64]

Hi Colin,

This is my opinionated linter that points out code I would rather not find in production.

Personally I wouldn't trust merge with anything but a map for every argument. I would recommend not passing it nil in production.

It's a subtle issue, the docstring claims it "conj's" on the map argument, but clearly it does not call (conj nil ...) if you pass nil as the first argument. That's too arbitrary for me. I have a whole list of pet peeves, my biases are clearing showing!

This was just a pull request after all, perhaps the devs would like to explain why they accepted it?

I would assume it's to beat the cleverness/brittleness out of the code. The disjunctive conditional makes it interesting that it works at all.

[technomancy]

I am not fond of the fact that Clojure is sloppy about its nil handling, but that's how it is. It's idiomatic in most lisps to conflate nil and empty collections, and going against that is just going to be a constant battle; I don't think it's worth cluttering the codebase for it. I'm certain this is not the only place that exhibits this aspect.

[hypirion]

I don't mind the correctness, and have no issues with this patch.

That being said, I'm not sure that using undocumented features in general is necessarily the problem -- It may be an upstream problem instead (i.e. lack of proper clojure.core documentation). It's a good thing to have a correct program which doesn't rely on undocumented behaviour, but when it hinders readability or code modifications, then perhaps the undocumented behaviour should be documented if it's reasonable.

This is a bit unrelated to the PR, but I think it would be very interesting to see which undocumented features are most frequently used. Perhaps it even can be used as an argument for elaborating undocumented features in clojure.core.

[sdegutis]

Allowing nils as merge arguments seems very idiomatic in Clojure. Conceptually it seems to fit the rest of clojure.core and I've probably relied on it a few times too. I'd agree that a better solution is to define and document this behavior rather than avoid using it.

[frenchy64]

These corner cases are enough to make me avoid passing nil to merge. I hope these don't get documented.

(merge nil)
;=> nil

(merge nil nil nil)
;=> nil

[technomancy]

In the context of the meta-merge function here (and many other places in Clojure), there is no ill effect from propagating nil through to with-meta.

I think it would be great if Clojure became more discerning around its use of nil, but that is something that would have to happen from the top-down. Trying to effect that kind of change in codebases like this one doesn't make sense.

[frenchy64]

Yes, bullying Clojure core into making changes just because of downstream usage is silly.

I think the sloppiness/convenience has its place in Clojure core, especially during dev.

I'm building tools to avoid "buying in" to the sloppiness in important code. It's also troubling to think of someone modifying meta-merge and misinterpreting the invariants at play (there are many implicit ones). So it's also defensive (proposed) change.

[scgilardi]

One succinct option in cases where it matters that the result of a merge not be nil is to add an empty map to the end of the arg list:

(with-meta (merge might-be-nil1 might-be-nil2 {}) {:some :meta})

[hypirion]

Just to clarify, this PR wasn't trying to remove non-nil results from a merge, but rather the fact that the nil values passed to merge (in this specific context) depends on undocumented functionality.