Include new `&.` operator as `NilCheck` smell

[backus]

The new &. operator (https://github.com/ruby/ruby/commit/a356fe1c3550892902103f66928426ac8279e072, https://github.com/ruby/ruby/commit/0b7d473734d0dec8520afe7a36540aa1f40d2532, https://github.com/ruby/ruby/commit/837babd56459aafc1232a12fbfa783025d619b98) (similar to ActiveSupport's #try) adds the following syntax:

ruby 2.3

foo = bar&.baz&.qux

which is functionally equivalent to

ruby 2.2

foo = bar.baz.qux unless bar.nil? || bar.baz.nil?

This depends on Parser adding support for this syntax (https://github.com/whitequark/parser/issues/209) but after that this should be possible.

[troessner]

Yep, had that one already on my watch list, thanks for opening up this issue!

[chastell]

Oh yes let’s do it. I read about this first thing today and it completely ruined my (admittedly, extremely first world) Friday morning. W to the T to the F, srsly. </grumble>

[mvz]

Hm .. I'm reluctant to instantly banish an entire Ruby language feature. In most case, this is equivalent to stuff like:

foo = bar.baz.qux if bar && bar.baz

Since we also allow stuff like the following, I don't see the point of jumping on .? just yet.

def foo(bar)
  return unless bar
  bar.quuz
end

I may change my mind, though, since I'm also not very fond of .try, seeing as it's totally overused.

[chastell]

I think we should consider #try a smell as well. It’s not quite the calibre of … rescue nil, but it’s close.

(Interestingly, Rust doesn’t have nils and solves this with option types, but the code out there in the wild ends up littered with .unwrap() calls; at least they blow up on the spot, not somewhere much further away. Crystal does have nil, but as a separate type and you have to explicitly handle it – e.g., you can’t call #odd? on an array of ints and nils. The above seems to be taking Ruby in the exact opposite direction.)

[troessner]

Hach, you're all such grumpy cats, I love this feature!

[Drenmi]

I, too, can be a grumpy cat, and nil is one of my pet peeves--so much so that I gave a talk on it. Needless to say, a resounding plus one on this. 😬

While we're on the subject of this smell detector; I am a big fan of the use of the NodeFinder construct, but it irks me a bit that the smell detector has a direct dependency on them.

How about inverting the dependencies, and perhaps have both depend on some common abstraction. Maybe something like a node finder repo with which node finders can register themselves, and smell detectors can use declaratively?

Then all the serial concatenation of node finder results could be reduced to something like:

node_finders.flat_map(&:smelly_nodes)

[mvz]

If there are node finders that could be used by more than one smell detector, that could be a sensible direction.

[troessner]

Maybe something like a node finder repo with which node finders can register themselves, and smell detectors can use declaratively?

Like the idea a lot but we probably need to prototype this a bit in a pull request to see how this works in practice :)

[Drenmi]

I think @mvz also made a good point:

If there are node finders that could be used by more than one smell detector [...]

If there's no reuse happening, putting in the extra abstraction to enable reuse might be, as they say, Not Worth the Effort™. 😁

[mvz]

It's probably best to move the smell detector implementations to some common form and then go from there. We 'just' have to decide on the common form.

[troessner]

It's probably best to move the smell detector implementations to some common form and then go from there. We 'just' have to decide on the common form.

Couldn't agree more. Right now our smell detectors vastly differ from each other. In my mind they should be responsible for:

• getting the nodes in question
• find the smelly ones
• build up the warning

And they should all have the same structure.

[mvz]

I've moved the discussion on smell detector structure to its own issue. Feel free to continue commenting there.

[mvz]

I'm going to take a stab at this.

[troessner]

I'm going to take a stab at this.

🎉

[mvz]

Stab taken :smile:.

[troessner]

Closing this one with #1026 merged