Git Product home page Git Product logo

pippi's Introduction

Pippi

Build Status

Pippi is a utility for finding suboptimal Ruby class API usage.

Consider this little array:

[1, 2, 3]

Now suppose we want to find the first element in that array that's greater than one. We can use Array#select, which returns another Array, and then use Array#first:

[1, 2, 3].select { |x| x > 1 }.first

Of course that's terribly inefficient. Since we only need one element we don't need to select all elements that match the predicate. We should use Array#detect instead:

[1, 2, 3].detect { |x| x > 1 }

A change like this is a small optimization, but they can add up. More importantly, they communicate the intent of the programmer; the use of Array#detect makes it clear that we're just looking for the first item to match the predicate.

This sort of thing can be be found during a code review, or maybe when you're just poking around the code. But why not have a tool find it instead? Thus, pippi. Pippi observes code while it's running - by hooking into your test suite execution - and reports misuse of class-level APIs.

There are many nifty Ruby static analysis tools - flay, reek, flog, etc. This is not like those. It doesn't parse source code; it doesn't examine an abstract syntax tree or even sequences of MRI instructions. So it cannot find the types of issues that those tools can find. Instead, it's focused on runtime analysis; that is, method calls and method call sequences.

Here's an important caveat: pippi is not, and more importantly cannot, be free of false positives. That's because of the halting problem. Pippi finds suboptimal API usage based on data flows as driven by a project's test suite. There may be alternate data flows where this API usage is correct. For example, in the code below, if rand < 0.5 is true, then the Array will be mutated and the program cannot correctly be simplified by replacing "select followed by first" with "detect":

x = [1, 2, 3].select { |y| y > 1 }
x.reject! { |y| y > 2 } if rand < 0.5
x.first

There are various techniques that eliminate many of these false positives. For example, after flagging an issue, pippi watches subsequent method invocations and if those indicate the initial problem report was in error it'll remove the problem from the report.

Pippi is entirely dependent on the test suite to execute code in order to find problems. If a project's test code coverage is small, pippi probably won't find much.

Here's how pippi stacks up using the Aaron Quint Ruby Performance Character Profiles system:

  • Specificity - very specific, finds actual detailed usages of bad code
  • Impact - very impactful, slows things down lots
  • Difficulty of Operator Use - easy to install, just a new gemfile entry
  • Readability - results are easy to read
  • Realtimedness - finds stuff right away
  • Special Abilities - ?

Finally, why "pippi"? Because Pippi Longstocking was a Thing-Finder, and pippi finds things.

Usage

Rails with test-unit

  • Add gem 'pippi' to the test group in your project's Gemfile
  • Add this to test_helper.rb just before the require 'rails/test_help' line
if ENV['USE_PIPPI'].present?
  Pippi::AutoRunner.new(:checkset => ENV['PIPPI_CHECKSET'] || "basic")
  # you can also pass in an IO:
  # Pippi::AutoRunner.new(:checkset => "basic", :io => $stdout)
end
  • Run it:
USE_PIPPI=true bundle exec rake test:units && cat log/pippi.log
  • You can also select a different checkset:
USE_PIPPI=true PIPPI_CHECKSET=rails bundle exec rake test:units && cat log/pippi.log
  • And you can run multiple checksets:
USE_PIPPI=true PIPPI_CHECKSET=basic,rails bundle exec rake test:units && cat log/pippi.log

Here's a demo Rails application.

Rails with rspec

  • Add gem 'pippi' to the test group in your project's Gemfile
  • Add this to spec/spec_helper.rb or spec/rails_helper.rb, just below the require 'rspec/rails' line (if there is one):
if ENV['USE_PIPPI'].present?
  require 'pippi'
  Pippi::AutoRunner.new(:checkset => ENV['PIPPI_CHECKSET'] || "basic")
end
  • Run it:
USE_PIPPI=true bundle exec rake spec && cat log/pippi.log

As part of a continuous integration job

Dan Kohn suggests you could use something like:

if grep -v gem < log/pippi.log; then echo "$(wc -l < log/pippi.log) Pippi flaws found" && false; else echo 'No pippi flaws found'; fi

From the command line:

Assuming you're using bundler:

# Add this to your project's Gemfile:
gem 'pippi'
# Run 'bundle', see some output
# To run a particular check:
bundle exec pippi tmp/tmpfile.rb MapFollowedByFlatten Foo.new.bar out.txt
# Or to run all the basic Pippi checks on your code and exercise it with MyClass.new.exercise_some_code:
bundle exec ruby -rpippi/auto_runner -e "MyClass.new.exercise_some_code"

Checksets

Pippi has the concept of "checksets" which are, well, sets of checks. The current checksets are listed below.

basic

ReverseFollowedByEach

Don't use reverse followed by each; use reverse_each instead

For example, rather than doing this:

[1,2,3].reverse.each {|x| x+1 }

Instead, consider doing this:

[1,2,3].reverse_each {|x| x+1 }

SelectFollowedByAny

Don't use select followed by any?; use any? with a block instead

For example, rather than doing this:

[1,2,3].select {|x| x > 1 }.any?

Instead, consider doing this:

[1,2,3].any? {|x| x > 1 }

SelectFollowedByEmpty

Don't use select followed by empty?; use none? instead

For example, rather than doing this:

[1,2,3].select {|x| x > 1 }.empty?

Instead, consider doing this:

[1,2,3].none? {|x| x > 1 }

SelectFollowedByFirst

Don't use select followed by first; use detect instead

For example, rather than doing this:

[1,2,3].select {|x| x > 1 }.first

Instead, consider doing this:

[1,2,3].detect {|x| x > 1 }

SelectFollowedByNone

Don't use select followed by none?; use none? with a block instead

For example, rather than doing this:

[1,2,3].select {|x| x > 1 }.none?

Instead, consider doing this:

[1,2,3].none? {|x| x > 1 }

SelectFollowedBySelect

Don't use consecutive select blocks; use a single select instead

For example, rather than doing this:

[1,2,3].select {|x| x > 1 }.select {|x| x > 2 }

Instead, consider doing this:

[1,2,3].select {|x| x > 2 }

SelectFollowedBySize

Don't use select followed by size; use count instead

For example, rather than doing this:

[1,2,3].select {|x| x > 1 }.size

Instead, consider doing this:

[1,2,3].count {|x| x > 1 }

buggy

AssertWithNil

Don't use assert_equal with nil as a first argument; use assert_nil instead

For example, rather than doing this:

x = nil ; assert_equal(nil, x)

Instead, consider doing this:

x = nil ; assert_nil(x)

MapFollowedByFlatten

Don't use map followed by flatten(1); use flat_map instead

For example, rather than doing this:

[1,2,3].map {|x| [x,x+1] }.flatten(1)

Instead, consider doing this:

[1,2,3].flat_map {|x| [x, x+1]}

rails

StripFollowedByEmpty

Don't use String#strip followed by empty?; use String#blank? instead

For example, rather than doing this:

'   '.strip.empty?

Instead, consider doing this:

'   '.blank?

Ideas for other problems to detect:

# unnecessary assignment since String#strip! mutates receiver
# wrong
x = x.strip!
# right
x.strip!

# Use Pathname
# wrong
File.read(File.join(Rails.root, "config", "database.yml")
# right
Rails.root.join("config", "database.yml").read

# Use Kernel#tap
# wrong
x = [1,2]
x << 3
return x
# right
[1,2].tap {|y| y << 3 }


# Rails checks

# No need to call to_i on ActiveRecord::Base methods passed to route generators
# wrong
product_path(@product.to_i)
# right
product_path(@product)

# something with replacing x.map.compact with x.select.map

Here are some things that Pippi is not well suited for

"Use self.new vs MyClass.new"

This is not a good fit for Pippi because it involves a receiver usage that can be detected with static analysis.

wrong:

class Foo
  def self.bar
    Foo.new
  end
end

right:

class Foo
  def self.bar
    self.new
  end
end

Proxying certain String instance methods

You might wonder why Pippi "rails" checkset doesn't have the rule "replace "foobar".gsub(/foo/, '') with "foobar".remove(/foo/)". That's because of the behavior of global variables such as $&. This behavior is nicely explained by Frederick Cheung on this StackOverflow comment. It's also broken down by David Black here and by Aaron Patterson and others here. Due to the issue explained there, Pippi's technique of prepending a proxy method breaks code that's further downstream when used with the block form of gsub.

TODO

  • Clean up this initial hacked out metaprogramming
  • Finish refactoring duplicated code into MethodSequenceChecker

Developing

To see teacher output for a file tmp/baz.rb:

rm -f pippi_debug.log ; PIPPI_DEBUG=1 bundle exec pippi tmp/baz.rb DebugCheck Foo.new.bar tmp/out.txt ; cat pippi_debug.log

When trying to find issues in a project:

# in project directory (e.g., aasm)
rm -rf pippi_debug.log pippi.log .bundle/gems/pippi-0.0.1/ .bundle/cache/pippi-0.0.1.gem .bundle/specifications/pippi-0.0.1.gemspec && bundle update pippi --local && PIPPI_DEBUG=1 bundle exec ruby -rpippi/auto_runner -e "puts 'hi'" && grep -C 5 BOOM pippi_debug.log
# or to run some specs with pippi watching:
rm -rf pippi_debug.log pippi.log .bundle/gems/pippi-0.0.1/ .bundle/cache/pippi-0.0.1.gem .bundle/specifications/pippi-0.0.1.gemspec && bundle update pippi --local && PIPPI_DEBUG=1 bundle exec ruby -rpippi/auto_runner -Ispec spec/unit/*.rb

How to do a release

  • Bump version number
  • Move anything from 'training' to 'buggy' or elsewhere
  • Tie off Changelog notes
  • Regenerate docs with pippi:generate_docs, copy and paste that into README
  • Commit, push
  • Tag the release (e.g., git tag -a v0.0.8 -m 'v0.0.8' && git push origin v0.0.8)
  • bundle exec gem build pippi.gemspec
  • gem push pippi-x.gem
  • Update pippi_demo

Credits

pippi's People

Contributors

aprescott avatar edelgado avatar hdabrows avatar jbodah avatar jonatack avatar nepalez avatar olleolleolle avatar spickermann avatar tcopeland avatar tommotorefi avatar

Stargazers

 avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar

Watchers

 avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar

pippi's Issues

pippi 0.0.11 causing numerous rspec failures

I've been using pippi 0.0.6 with no problems, but 0.0.11 is causing numerous RSpec failures. Frustratingly, the error is only on my CircleCI server (running Ubuntu), but not on my OS X dev machine, making it hard to diagnose the problem. It appears, though, that pippi is grabbing tons of memory when CircleCI has a 4 GB limit. I have:

# spec/spec_helper.rb
if ENV['USE_PIPPI'].present?
  require 'pippi'
  Pippi::AutoRunner.new(checkset: ENV['PIPPI_CHECKSET'] || 'basic')
end

I'm invoking RSpec with:

USE_PIPPI=1 bundle exec rspec --color --require spec_helper --format documentation spec

method `size' not defined in Pippi::Checks::SelectFollowedBySize::MySize (NameError)

I'm getting the following :( -

→ USE_PIPPI=true rspec
Run options:
  include {:focus=>true}
  exclude {:js=>true}
/usr/local/lib/ruby/gems/2.1.0/gems/pippi-0.0.3/lib/pippi/checks/check.rb:33:in `remove_method': method `size' not defined in Pippi::Checks::SelectFollowedBySize::MySize (NameError)
    from /usr/local/lib/ruby/gems/2.1.0/gems/pippi-0.0.3/lib/pippi/checks/check.rb:33:in `block (2 levels) in its_ok_watcher_proc'
    from /usr/local/lib/ruby/gems/2.1.0/gems/pippi-0.0.3/lib/pippi/checks/check.rb:33:in `instance_eval'
    from /usr/local/lib/ruby/gems/2.1.0/gems/pippi-0.0.3/lib/pippi/checks/check.rb:33:in `block in its_ok_watcher_proc'
    from /usr/local/Cellar/ruby/2.1.3_1/lib/ruby/gems/2.1.0/gems/rspec-core-3.1.7/lib/rspec/core/world.rb:24:in `block in initialize'
    from /usr/local/Cellar/ruby/2.1.3_1/lib/ruby/gems/2.1.0/gems/rspec-core-3.1.7/lib/rspec/core/example_group.rb:366:in `yield'
    from /usr/local/Cellar/ruby/2.1.3_1/lib/ruby/gems/2.1.0/gems/rspec-core-3.1.7/lib/rspec/core/example_group.rb:366:in `filtered_examples'
    from /usr/local/Cellar/ruby/2.1.3_1/lib/ruby/gems/2.1.0/gems/rspec-core-3.1.7/lib/rspec/core/world.rb:95:in `block in example_count'
    from /usr/local/Cellar/ruby/2.1.3_1/lib/ruby/gems/2.1.0/gems/rspec-core-3.1.7/lib/rspec/core/world.rb:95:in `each'
    from /usr/local/Cellar/ruby/2.1.3_1/lib/ruby/gems/2.1.0/gems/rspec-core-3.1.7/lib/rspec/core/world.rb:95:in `inject'
    from /usr/local/Cellar/ruby/2.1.3_1/lib/ruby/gems/2.1.0/gems/rspec-core-3.1.7/lib/rspec/core/world.rb:95:in `example_count'
    from /usr/local/Cellar/ruby/2.1.3_1/lib/ruby/gems/2.1.0/gems/rspec-core-3.1.7/lib/rspec/core/world.rb:129:in `announce_filters'
    from /usr/local/Cellar/ruby/2.1.3_1/lib/ruby/gems/2.1.0/gems/rspec-core-3.1.7/lib/rspec/core/runner.rb:97:in `setup'
    from /usr/local/Cellar/ruby/2.1.3_1/lib/ruby/gems/2.1.0/gems/rspec-core-3.1.7/lib/rspec/core/runner.rb:84:in `run'
    from /usr/local/Cellar/ruby/2.1.3_1/lib/ruby/gems/2.1.0/gems/rspec-core-3.1.7/lib/rspec/core/runner.rb:69:in `run'
    from /usr/local/Cellar/ruby/2.1.3_1/lib/ruby/gems/2.1.0/gems/rspec-core-3.1.7/lib/rspec/core/runner.rb:37:in `invoke'
    from /usr/local/lib/ruby/gems/2.1.0/gems/rspec-core-3.1.7/exe/rspec:4:in `<top (required)>'
    from /usr/local/bin/rspec:23:in `load'
    from /usr/local/bin/rspec:23:in `<main>'

Code breaking side effects

I can't get pippi running with integration tests.

I get errors as in #5, but I'm using Minitest, not RSpec. The error occurs when remove_method is called in check.rb:33. triggered by the call to map! in ActionDispatch::Journey::Router.get_routes_as_head. I don't know why the proc is even called, since method_name is :size and there's no explicit call to size...

I tried to find a minimal example that triggers this error, but was unsuccessful. Instead I encountered another issue. When I use [].select { |r| r }.map! { |r| r }, the code returns an enumerator instead of an array if pippi is enabled. There seems to be something completely wrong with the handling of map!.

Log file improvements

Planning to create a PR for this over the next couple days but want to log it

Using pippi within my gem causes issues logging to log/pippi.log since my gem doesn't have a log directory. Several things I'd like to see:

  1. Don't fail if log directory doesn't exist
  2. Print to stdout by default
  3. Accept a log file path as as option

Name

Hi Tom,

i assume you are not familiar with german. :)

Here is a translation page: http://www.dict.cc/?s=pipi

Just want to let you know. Its kind of strange to insert this into my Gemfile :)

contributing

Hey, @tcopeland!

You made fantastic work and I want to help, but developing section on readme pointing to files that didn't exist. Can you plz remove tmp/.gitignore and change line tmp/ from main gitignore file to:

tmp/*
!tmp/baz.rb

and another files if they needed in same way as tmp/baz.rb.

Thx!

TypeError: singleton can't be dumped

Hi,

first of all: Thanks for this library :)

While trying pippi on one of our projects I discovered several TypeErrors after activating pippi in tests.

A simple reproduction:

require 'bundler/setup'
require 'minitest/autorun'
require 'pippi'

Pippi::AutoRunner.new(:checkset => ENV['PIPPI_CHECKSET'] || "basic")

class MarshalDumpTest < Minitest::Test
  def test_dump_broken
    Marshal.dump([].select(&:odd?))
  end

  def test_dump_works
    Marshal.dump([].select(&:odd?).dup)
  end
end

Output

Run options: --seed 39192

# Running:

E.

Finished in 0.002342s, 853.9608 runs/s, 0.0000 assertions/s.

  1) Error:
MarshalDumpTest#test_dump_broken:
TypeError: singleton can't be dumped
    test/marshal_dump_test.rb:9:in `dump'
    test/marshal_dump_test.rb:9:in `test_dump_broken'

2 runs, 0 assertions, 0 failures, 1 errors, 0 skips

Any pointers how to solve this issue?

Rails direction

I would suggest adding the following Rails and RSpec directions to the Readme:

Please Pippi near the top of spec/spec_helper.rb. You can call it with:

if ENV['USE_PIPPI'].present?
  require 'pippi'
  Pippi::AutoRunner.new(:checkset => ENV['PIPPI_CHECKSET'] || "basic")
end

This will prevent it from being loaded except when you preface a run with USE_PIPPI=true.

RSpec

How would I invoke pippi when running an RSpec test?

Pippi should default to stdout

Also, if Pippi defaulted to stdout instead of writing to a log file, I could more easily integrate it into our continuous integration server.

Pippi should ignore gems directories

Hi, pippi is now working for me, but it would be great if it offered to ignore code from gems. In fact, this would be helpful as the default option. My pippi.log reads:

/Users/dan/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/activesupport-4.1.7/lib/active_support/callbacks.rb,ReverseFollowedByEach,559
/Users/dan/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/rack-1.5.2/lib/rack/builder.rb,ReverseFollowedByEach,134
/Users/dan/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/rspec-core-3.1.7/lib/rspec/core/world.rb,SelectFollowedBySize,95
/Users/dan/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/rspec-core-3.1.7/lib/rspec/core/flat_map.rb,ReverseFollowedByEach,7
/Users/dan/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/faker-1.4.3/lib/faker/bitcoin.rb,ReverseFollowedByEach,23
/Users/dan/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/activesupport-4.1.7/lib/active_support/callbacks.rb,ReverseFollowedByEach,511
/Users/dan/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/activemodel-4.1.7/lib/active_model/attribute_methods.rb,ReverseFollowedByEach,362
/Users/dan/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/cancancan-1.9.2/lib/cancan/model_adapters/active_record_adapter.rb,SelectFollowedBySize,20
/Users/dan/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/cancancan-1.9.2/lib/cancan/model_adapters/active_record_adapter.rb,ReverseFollowedByEach,24
/Users/dan/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/rspec-mocks-3.1.3/lib/rspec/mocks/space.rb,ReverseFollowedByEach,74
/Users/dan/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/cancancan-1.9.2/lib/cancan/model_adapters/active_record_adapter.rb,SelectFollowedByFirst,20
/Users/dan/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/cancancan-1.9.2/lib/cancan/model_adapters/active_record_adapter.rb,SelectFollowedByFirst,22
/Users/dan/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/paperclip-4.2.0/lib/paperclip/interpolations.rb,ReverseFollowedByEach,32
/Users/dan/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/chronic-0.10.2/lib/chronic/handler.rb,SelectFollowedBySize,66
/Users/dan/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/chronic-0.10.2/lib/chronic/handler.rb,SelectFollowedBySize,48
/Users/dan/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/chronic-0.10.2/lib/chronic/handler.rb,SelectFollowedBySize,57
/Users/dan/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/chronic-0.10.2/lib/chronic/handlers.rb,SelectFollowedBySize,146
/Users/dan/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/actionpack-4.1.7/lib/action_controller/metal.rb,ReverseFollowedByEach,38
/Users/dan/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/sprockets-2.11.3/lib/sprockets/asset_attributes.rb,ReverseFollowedByEach,72
/Users/dan/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/activesupport-4.1.7/lib/active_support/rescuable.rb,ReverseFollowedByEach,88

Database transactions change using rspec

Adding the four lines suggested to rails_helper.rb and running USE_PIPPI=1 bundle exec ./bin/rake causes numerous tests to fail in our otherwise passing test suite.

All these tests are related to duplicate records clashes, or return values not being empty, all of which point to transactions breaking, or the database cleaner gem not firing correctly.

We are running with rspec 3.2, database cleaner (set up as Avdi does in his blog post) and pippi 0.0.12

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.