New Performance/DoubleStartEndWith cop #2590

DNNX · 2016-01-06T10:52:23Z

This is a continuation of #2535 .

This cop checks for double #start_with? or #end_with? calls
separated by ||. In some cases such calls can be replaced
with an single #start_with?/#end_with? call.

  # bad
  str.start_with?("a") || str.start_with?(Some::CONST)
  str.start_with?("a", "b") || str.start_with?("c")
  var1 = ...
  var2 = ...
  str.end_with?(var1) || str.end_with?(var2)

  # good
  str.start_with?("a", Some::CONST)
  str.start_with?("a", "b", "c")
  var1 = ...
  var2 = ...
  str.end_with?(var1, var2)

The corner cases are when arguments to start_with? have side effects,
or when the receiver itself has side effects, or when the receiver
is not a string at all, or when someone monkey-patched String#start_with?

Benchmark results are not too convincing because of a big variance, but they show that the single-call approach is at least not slower that the two-call approach.

prefix1 = "lib/"
prefix2 = "test/"
{
  'First prefix matches' => 'lib/file.rb',
  'Second prefix matches' => 'test/file.rb',
  'None matches' => 'app/models/file.rb'
}.each do |description, str|
  Benchmark.ips do |x|
    puts "==== #{description} ===="
    x.report('two calls') { str.start_with?(prefix1) || str.start_with?(prefix2) }
    x.report('one call')  { str.start_with?(prefix1, prefix2) }
    x.compare!
  end
end

==== First prefix matches ====
Calculating -------------------------------------
           two calls    31.010k i/100ms
            one call    35.180k i/100ms
-------------------------------------------------
           two calls      2.035M (±14.6%) i/s -      9.644M
            one call      2.039M (±15.9%) i/s -      9.569M

Comparison:
            one call:  2038728.5 i/s
           two calls:  2034701.2 i/s - 1.00x slower

==== Second prefix matches ====
Calculating -------------------------------------
           two calls    31.139k i/100ms
            one call    34.518k i/100ms
-------------------------------------------------
           two calls      1.515M (±20.7%) i/s -      6.944M
            one call      1.709M (±19.5%) i/s -      7.836M

Comparison:
            one call:  1708570.7 i/s
           two calls:  1514975.3 i/s - 1.13x slower

==== None matches ====
Calculating -------------------------------------
           two calls    33.038k i/100ms
            one call    36.789k i/100ms
-------------------------------------------------
           two calls      1.471M (±18.9%) i/s -      6.773M
            one call      1.778M (±19.9%) i/s -      8.130M

Comparison:
            one call:  1778026.2 i/s
           two calls:  1471409.3 i/s - 1.21x slower

Wild life occurrences: ManageIQ/manageiq#5558, rails/rails#22374, the aforementioned #2535.

alexdowad · 2016-01-06T11:10:11Z

Nice!! I was thinking of doing this myself, good to see that someone else did it first!

alexdowad · 2016-01-06T11:11:29Z

lib/rubocop/cop/performance/double_start_end_with.rb

+      #   str.end_with?(var1, var2)
+      class DoubleStartEndWith < Cop
+        MSG = 'Use `str.%{method}(x, ..., y, ...)` ' \
+              'instead of `str.%{method}(x, ...) || str.%{method}(y, ...)`.'


Rather than showing x, ... and y, ..., it would be nicer if the message showed the actual arguments.

Good point. I'll fix it.

alexdowad · 2016-01-06T11:22:33Z

One other way this could be improved -- think of things like this:

# this will parse as (or (or something str.start_with?('a')) str.start_with?('b'))...
# in other words the 2 nodes you are interested in will not be nested under the same 'or' node
something || str.start_with?('a') || str.start_with?('b')

# another example:
str.start_with?('a') || foo || bar || str.start_with?('b')

Evaluation is from left to right, so you have to make sure that there is no node with unknown side effects between the 2 matching calls.

DNNX · 2016-01-06T11:29:45Z

something || str.start_with?('a') || str.start_with?('b') - yea, I was thinking about this, but found it kind of complex to implement so I decided to go with more simple implementation which covers most real-life cases (in my experience). I also saw !str.start_with?(a) && !str.start_with?(b) in one open-source project. I can also be simplified, but again, I decided not to go this far. Do you think it should be done in this PR?

Another thing which can potentially be done is we can loosen the requirement of the first argument to the first start_with? call to be pure. As long as all other arguments are pure, the first one can have side effects - the semantics will stay the same.

alexdowad · 2016-01-06T11:45:14Z

Do you think it should be done in this PR?

It's up to you. I am just suggesting ways to make the PR better. If you don't want to go that far in this PR, and the maintainers want to accept it as is, that is fine. It is already an improvement over what we have.

DNNX · 2016-01-06T12:41:23Z

Here is a little summary:

Implemented more intelligent matcher as per @alexdowad's suggestion
Improved offense message format. No more abstract x, ... y, ...; the message now includes the actual code
Dropped the requirement for the arguments of the first start_with? call to be pure
something || str.start_with?('a') || str.start_with?('b') - not going to do this in this PR if you guys don't mind
str.start_with?('a') || foo || bar || str.start_with?('b') - will leave it as well
!str.start_with?(a) && !str.start_with?(b) - not going to do this either (in this PR at least)

If you guys are ok with this, please let me know. I will squash the commits and add an entry to the change log then.

DNNX · 2016-01-07T12:24:13Z

UPD: This PR is ready to merge now, if everyone is ok with the changes.

jonas054 · 2016-01-07T12:40:36Z

👍 The changes look good.

bbatsov · 2016-01-07T14:45:49Z

lib/rubocop/cop/performance/double_start_end_with.rb

+              'instead of `%{original_code}`.'
+
+        def on_or(node)
+          receiver, method, args1, args2 = two_start_end_with_calls(node)


args1 and args2 are not very descriptive variable names.

bbatsov · 2016-01-07T14:51:24Z

spec/rubocop/cop/performance/double_start_end_with_spec.rb

+  context 'two #start_with? calls' do
+    context 'with the same receiver' do
+      context 'all parameters of the second call are pure' do
+        let(:source) { 'x.start_with?(a, b) || x.start_with?("c", D)' }


As someone who does a bit of FP on the side, I think the usage of the term "pure" here is confusing. Basically you don' t want a function call as an argument, right?

No -- "pure" means "pure", as in "has no side effects when evaluated". See the definition in Node.

Yes that's right.

Well, strictly speaking we can't allow local variables and constants too. There might be some pathalogical cases when .to_str have side effects:

class Evil def initialize @state = 0 end def to_str @state = (@state + 1) % 4 @state.to_s end end EVIL = Evil.new p "1".start_with?(EVIL, EVIL) || "1".start_with?(EVIL) # => true p "1".start_with?(EVIL, EVIL, EVIL) # => false

pure doesn't mean "has not side effect when evaluated". It means that the same input always maps to the same output, that the only input is parameters (no global, instance vars, etc) and that they are no side effects. I'll check the definition.

Sounds good to me. @alexdowad what do you think?

Hmm. Seems equally confusing with pure?, to be honest.

All the nodes you're checking seem to me like value nodes (and the complex nodes also reduce down to values). In general there are no method invocation checks and I really think that many people associate the term pure with functions and methods. I'm open to other naming suggestions, but I certainly think we can do better than the current name. I was recently reminded that I didn't like this name when I originally saw it, but the commit was a small part of a huge PR and I was too tired to go into relatively small details. Anyways, guess this is not related to the current PR.

Is #no_side_effects? too wordy?

My main issue is that such names imply we're dealing with something executable. I'd expect this to apply to send nodes, not to nodes in general. Naming is so hard...

bbatsov · 2016-01-07T17:23:37Z

Rebase this and I'll have it merged.

This cop checks for double `#start_with?` or `#end_with?` calls separated by `||`. In some cases such calls can be replaced with an single `#start_with?`/`#end_with?` call. ```ruby # bad str.start_with?("a") || str.start_with?(Some::CONST) str.start_with?("a", "b") || str.start_with?("c") var1 = ... var2 = ... str.end_with?(var1) || str.end_with?(var2) # good str.start_with?("a", Some::CONST) || str.start_with?(Some::CONST) str.start_with?("a", "b", "c") var1 = ... var2 = ... str.end_with?(var1, var2) ``` The corner cases are when arguments of the second `start_with?` call have side effects, or when the receiver itself has side effects, or when the receiver is not a string at all, or when someone monkey-patched `String#start_with?`

DNNX · 2016-01-07T18:57:44Z

Rebased it. CI green, no conflicts (yet).

New Performance/DoubleStartEndWith cop

alexdowad reviewed Jan 6, 2016
View reviewed changes

DNNX force-pushed the double-start-end-with branch from 9ab0eaa to 1bb60bb Compare January 7, 2016 11:30

bbatsov reviewed Jan 7, 2016
View reviewed changes

DNNX force-pushed the double-start-end-with branch from 1bb60bb to 2f42e87 Compare January 7, 2016 14:50

bbatsov reviewed Jan 7, 2016
View reviewed changes

DNNX force-pushed the double-start-end-with branch from 2f42e87 to 494a434 Compare January 7, 2016 18:34

bbatsov added a commit that referenced this pull request Jan 7, 2016

Merge pull request #2590 from DNNX/double-start-end-with

fc588a3

New Performance/DoubleStartEndWith cop

bbatsov merged commit fc588a3 into rubocop:master Jan 7, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Performance/DoubleStartEndWith cop #2590

New Performance/DoubleStartEndWith cop #2590

DNNX commented Jan 6, 2016

alexdowad commented Jan 6, 2016

alexdowad Jan 6, 2016

DNNX Jan 6, 2016

alexdowad commented Jan 6, 2016

DNNX commented Jan 6, 2016

alexdowad commented Jan 6, 2016

DNNX commented Jan 6, 2016

DNNX commented Jan 7, 2016

jonas054 commented Jan 7, 2016

bbatsov Jan 7, 2016

DNNX Jan 7, 2016

bbatsov Jan 7, 2016

alexdowad Jan 7, 2016

DNNX Jan 7, 2016

DNNX Jan 7, 2016

bbatsov Jan 7, 2016

DNNX Jan 7, 2016

alexdowad Jan 7, 2016

bbatsov Jan 7, 2016

alexdowad Jan 7, 2016

bbatsov Jan 7, 2016

bbatsov commented Jan 7, 2016

DNNX commented Jan 7, 2016

New Performance/DoubleStartEndWith cop #2590

New Performance/DoubleStartEndWith cop #2590

Conversation

DNNX commented Jan 6, 2016

alexdowad commented Jan 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexdowad commented Jan 6, 2016

DNNX commented Jan 6, 2016

alexdowad commented Jan 6, 2016

DNNX commented Jan 6, 2016

DNNX commented Jan 7, 2016

jonas054 commented Jan 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbatsov commented Jan 7, 2016

DNNX commented Jan 7, 2016