Add 'Style/UnpackFirst' cop #5643

bdewater · 2018-03-07T04:35:27Z

This is a shorter way of getting the data you will need most of the time when unpacking a string. It was introduced in Ruby 2.4: https://ruby-doc.org/core-2.4.0/String.html#method-i-unpack1

While unpack1 is also faster (it skips creating the intermediate array object), I consider it to be more style related like Style/SymbolProc. The bad form runs 3+ million iterations per second on my laptop, which is not a performance bottleneck for most Ruby apps.

Before submitting the PR make sure the following are checked:

Wrote [good commit messages]
Feature branch is up-to-date with master (if not - rebase it).
Squashed related commits together.
Added tests.
Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
The PR relates to only one subject with a clear title
and description in grammatically correct, complete sentences.
Run rake default or rake parallel. It executes all tests and RuboCop for itself, and generates the documentation.

bbatsov · 2018-03-12T06:10:54Z

lib/rubocop/cop/style/unpack.rb

+
+        minimum_target_ruby_version 2.4
+
+        MSG = 'Use `unpack1(%<format>s)` instead of '\


The message might be more informative with the receiver of unpack in it.

bbatsov · 2018-03-12T06:12:32Z

The cop looks good overall. I wonder of the generic name Unpack shouldn't be changed with UnpackFirst, though.

bdewater · 2018-03-12T13:53:17Z

Thanks for the feedback. Agreed on both points and just pushed the changes 😄

Drenmi · 2018-03-12T15:18:43Z

There's one (not very commonly used) syntax that's unaccounted for. Accessing like so:

foo.[](0)

Not sure if it's worth accounting for this in the first version, unless it can be easily handled. 🙂

bdewater · 2018-03-12T15:21:53Z

TIL! I'll look at adding at later today.

rrosenblum · 2018-03-12T19:29:41Z

@Drenmi Other cops like Performance/Sample do not currently support checking the style foo.[](0). I am starting to see that style used more with safe navigation, foo&.[](0). Is it worth adding support for that style? Regardless, it seems like a good candidate for a new style cop.

bdewater · 2018-03-13T21:46:16Z

@Drenmi I added that syntax (and found some others 😄) but was not able to get the message completely right 🤔

rrosenblum · 2018-03-14T13:26:20Z

While unpack1 is also faster, I consider it to be more style related like Style/SymbolProc. The bad form runs 1+ million iterations per second on my laptop, which is not a performance bottleneck for most Ruby apps.

It is a tough call between classifying as style vs performance some times. Like you said, this unlikely to be a performance bottleneck, but there is a clear performance gain in using unpack1. Regardless of how we classify this cop, I encourage you to add an example to https://github.com/JuanitoFatas/fast-ruby

[2] pry(main)> FOO_BAR = 'foo bar'.freeze
[5] pry(main)> Benchmark.ips do |x|
[5] pry(main)*   x.report('unpack[0]') { FOO_BAR.unpack('Z*Z*')[0] }
[5] pry(main)*   x.report('unpack1') { FOO_BAR.unpack1('Z*Z*') }
[5] pry(main)*   x.compare!
[5] pry(main)* end
Warming up --------------------------------------
           unpack[0]   169.518k i/100ms
             unpack1   218.699k i/100ms
Calculating -------------------------------------
           unpack[0]      3.356M (± 5.3%) i/s -     16.782M in   5.015638s
             unpack1      5.533M (± 5.5%) i/s -     27.775M in   5.036752s

Comparison:
             unpack1:  5532904.7 i/s
           unpack[0]:  3355751.7 i/s - 1.65x  slower

rrosenblum · 2018-03-14T13:30:55Z

lib/rubocop/cop/style/unpack_first.rb

+        MSG = 'Use `String#unpack1(%<format>s)` instead of '\
+          '`String#unpack(%<format>s)%<method>s`.'.freeze
+
+        def_node_matcher :unpack_and_first_element?, <<-PATTERN


Thoughts for later: I wonder if we could capture this into a reusable form to make it easier for other cops to also identify that they are grabbing the first element from an array. Performance/Sample also looks for grabbing the first element, but is missing checks for at and take.

rrosenblum · 2018-03-14T13:52:25Z

@bdewater Here is a fix for the messaging, https://gist.github.com/rrosenblum/240d8343c4aa7377a73f5170ddbaf6c3.

bdewater · 2018-03-15T16:02:36Z

@rrosenblum thank you very much! 🙇🏼‍♂️ Learned something new today 😄 I believe this is now ready to be merged.

rrosenblum · 2018-03-15T16:30:09Z

config/enabled.yml

@@ -2010,6 +2010,12 @@ Style/UnneededPercentQ:
  StyleGuide: '#percent-q'
  Enabled: true

+Style/UnpackFirst:
+  Description: >-
+                 Checks for accessing the first element of unpack's result


unpack and unpack1 should be surrounded by back-ticks (`).

rrosenblum · 2018-03-15T16:30:59Z

lib/rubocop/cop/style/unpack_first.rb

+module RuboCop
+  module Cop
+    module Style
+      # This cop checks for accessing the first element of *unpack* which can


Use back-ticks instead of asterisks around unpack and unpack1.

rrosenblum · 2018-03-15T16:34:12Z

lib/rubocop/cop/style/unpack_first.rb

+
+        def autocorrect(node)
+          lambda do |corrector|
+            corrector.remove(node.loc.dot) if node.method_name == :first


There are no tests for the auto-correction of take, slice, and at. I think the auto-correction is going to produce incorrect code when one of those is used.

bbatsov · 2018-03-18T02:32:44Z

lib/rubocop/cop/style/unpack_first.rb

+
+        minimum_target_ruby_version 2.4
+
+        MSG = 'Use `String#unpack1(%<format>s)` instead of '\


Why not <%format>s.unpack1? That reads better to me. Same for the other part of the message.

Because format is the argument to unpack(1), not the string to be unpacked.

I'm happy to leave the String prefix off, but I thought that was the suggestion you made in your earlier comment.

I meant I wanted the message to say something like Use foo.unpack1instead offoo...`. When using the actual code that you're checking in the messages they are easier to understand.

Ok, I updated the message to show this.

This is a shorter way of getting the data you will need most of the time when unpacking a string. While unpack1 is also faster (it skips creating the intermediate array object), I consider it to be more style related like Style/SymbolProc. The bad form runs 3+ million iterations per second, which is not a performance bottleneck for most Ruby apps.

greysteil · 2018-03-22T23:22:26Z

Guys, so sorry for the cross-reference spam above. Only realised it was happening today (was due to a change we made last week) and fixed it immediately in dependabot/dependabot-core@2bd343b. Unfortunately there's no way for us to delete the old cross-references, so they'll stay there as a reminder of our mistake 😞

bbatsov · 2018-03-23T02:31:15Z

@greysteil No worries!

bdewater force-pushed the unpack-cop branch 5 times, most recently from db347b0 to 9664d49 Compare March 9, 2018 15:24

bbatsov reviewed Mar 12, 2018

View reviewed changes

bdewater force-pushed the unpack-cop branch 2 times, most recently from b13f0a2 to 01e22fe Compare March 12, 2018 13:45

bdewater changed the title ~~Add 'Style/Unpack' cop~~ Add 'Style/UnpackFirst' cop Mar 12, 2018

bdewater force-pushed the unpack-cop branch from 01e22fe to 08c8785 Compare March 12, 2018 14:38

bdewater force-pushed the unpack-cop branch from 08c8785 to 31314e3 Compare March 13, 2018 21:45

bdewater force-pushed the unpack-cop branch from 31314e3 to 0cbe0f0 Compare March 13, 2018 21:47

rrosenblum reviewed Mar 14, 2018

View reviewed changes

bdewater force-pushed the unpack-cop branch 2 times, most recently from 215a9ec to 3e3cd67 Compare March 15, 2018 16:01

rrosenblum suggested changes Mar 15, 2018

View reviewed changes

bdewater force-pushed the unpack-cop branch 2 times, most recently from 400b052 to 2caa223 Compare March 15, 2018 19:56

bdewater mentioned this pull request Mar 17, 2018

Add String#unpack1 vs String#unpack[0] fastruby/fast-ruby#148

Merged

bbatsov reviewed Mar 18, 2018

View reviewed changes

dependencies bot mentioned this pull request Mar 22, 2018

Update rubocop in / from 0.52.1 to 0.54.0 chauncey-garrett/dotfiles#106

Open

bdewater mentioned this pull request Apr 8, 2018

Add a new cop Performance/UnneededSort #5753

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add 'Style/UnpackFirst' cop #5643

Add 'Style/UnpackFirst' cop #5643

bdewater commented Mar 7, 2018 •

edited

Loading

bbatsov Mar 12, 2018

bbatsov commented Mar 12, 2018

bdewater commented Mar 12, 2018

Drenmi commented Mar 12, 2018

bdewater commented Mar 12, 2018

rrosenblum commented Mar 12, 2018

bdewater commented Mar 13, 2018

rrosenblum commented Mar 14, 2018

rrosenblum Mar 14, 2018

rrosenblum commented Mar 14, 2018

bdewater commented Mar 15, 2018

rrosenblum Mar 15, 2018

rrosenblum Mar 15, 2018

rrosenblum Mar 15, 2018

bbatsov Mar 18, 2018

bdewater Mar 18, 2018

bbatsov Mar 18, 2018

bdewater Mar 18, 2018

greysteil commented Mar 22, 2018

bbatsov commented Mar 23, 2018


		minimum_target_ruby_version 2.4

		MSG = 'Use `unpack1(%<format>s)` instead of '\


		minimum_target_ruby_version 2.4

		MSG = 'Use `String#unpack1(%<format>s)` instead of '\

Add 'Style/UnpackFirst' cop #5643

Add 'Style/UnpackFirst' cop #5643

Conversation

bdewater commented Mar 7, 2018 • edited Loading

Choose a reason for hiding this comment

bbatsov commented Mar 12, 2018

bdewater commented Mar 12, 2018

Drenmi commented Mar 12, 2018

bdewater commented Mar 12, 2018

rrosenblum commented Mar 12, 2018

bdewater commented Mar 13, 2018

rrosenblum commented Mar 14, 2018

Choose a reason for hiding this comment

rrosenblum commented Mar 14, 2018

bdewater commented Mar 15, 2018

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

greysteil commented Mar 22, 2018

bbatsov commented Mar 23, 2018

bdewater commented Mar 7, 2018 •

edited

Loading