Static analysis

PullRequest: truffleruby/561

Author: chrisseaton

.reek.yml

@@ -0,0 +1,49 @@
+detectors:
+  UncommunicativeVariableName:
+    enabled: false  # we're happy with short variable names
+  UncommunicativeParameterName:
+    enabled: false  # we're happy with short parameter names
+  LongParameterList:
+    enabled: false  # we're often meeting an existing Ruby API with fixed parameters
+  UncommunicativeModuleName:
+    enabled: false  # we're often meeting an existing Ruby API with fixed module names
+  UncommunicativeMethodName:
+    enabled: false  # we're often meeting an existing Ruby API with fixed method names
+  BooleanParameter:
+    enabled: false  # we're often meeting an existing Ruby API with boolean parameters
+  TooManyStatements:
+    enabled: false  # yes we often write longer methods
+  FeatureEnvy:
+    enabled: false  # we don't want to add new public methods to Ruby API classes to better encapsulate behaviour
+  UnusedParameters:
+    enabled: false  # primitives mean that the use of parameters can be hidden
+  NilCheck:
+    enabled: false  # our code is often too low level to factor this away
+  ManualDispatch:
+    enabled: false  # manual dispatch is often part of Ruby internal semantics
+  IrresponsibleModule:
+    enabled: false  # we don't comment all modules because we're implementing an existing interface so they do not need a raison d'être
+  ControlParameter:
+    enabled: false  # we're often meeting an existing Ruby API with control parameters
+  DataClump:
+    enabled: false  # we're often meeting an existing Ruby API with 'data clumps'
+  TooManyMethods:
+    enabled: false  # we're often meeting an existing Ruby API with an existing set of methods
+  TooManyConstants:
+    enabled: false  # we're often meeting an existing Ruby API with an existing set of constants
+  UtilityFunction:
+    enabled: false  # we're often meeting an existing Ruby API with utility functions
+  MissingSafeMethod:
+    enabled: false  # we're often meeting an existing Ruby API without safe equivalents, or the safe equivalent is implemented in Java
+  Attribute:
+    enabled: false  # we're often meeting an existing Ruby API with writable attributes
+  InstanceVariableAssumption:
+    enabled: false  # we often set instance variables lazily
+  NestedIterators:
+    enabled: false  # yes we often write nested iterators
+  TooManyInstanceVariables:
+    enabled: false  # yes we sometimes use a lot of instance variables
+  RepeatedConditional:
+    enabled: false  # we don't want to add new public methods to Ruby API classes to better encapsulate behaviour
+  DuplicateMethodCall:
+    enabled: false  # needs investigation - sometimes legitimate due to Ruby semantics, but lots of failures

.rubocop.yml

@@ -11,7 +11,7 @@ AllCops:
 
 # Type 'Style' (166):
 # Supports --auto-correct
-Layout/TrailingBlankLines:
+Style/TrailingBlankLines:
   Description: Checks trailing blank lines and final newline.
   StyleGuide: "#newline-eof"
   Enabled: true
@@ -21,7 +21,7 @@ Layout/TrailingBlankLines:
   - final_blank_line
 
 # Supports --auto-correct
-Layout/TrailingWhitespace:
+Style/TrailingWhitespace:
   Description: Avoid trailing whitespace.
   StyleGuide: "#no-trailing-whitespace"
   Enabled: true

doc/contributor/static-analysis.md

@@ -0,0 +1,111 @@
+# Static Analysis of TruffleRuby
+
+We apply static analysis to the production code that we maintain -
+`lib/truffle`, `lib/cext`, `src/annotations`, `src/launcher`, `src/main`,
+`src/services` (some parts of static analysis are also applied to other files,
+but these are the key ones).
+
+## C
+
+The [Clang Static Analyzer](https://clang-analyzer.llvm.org) finds bugs in C
+code. We occasionally run this tool locally but only take its output as a
+suggestion. We use a default configuration.
+
+```
+$ scan-build --use-analyzer `which clang` -analyze-headers clang -c --std=c99 -Ilib/cext/include src/main/c/cext/ruby.c src/main/c/truffleposix/truffleposix.c
+$ scan-view ...as instructed by scan-build...
+```
+
+## Java
+
+### DSL usage
+
+We have a tool to check that some use of our internal annotations and the
+Truffle DSL are correct. Passing this is enforced in our CI gate.
+
+```
+$ jt check_dsl_usage
+```
+
+### CheckStyle
+
+[CheckStyle](http://checkstyle.sourceforge.net) enforces a Java style guide.
+Passing CheckStyle is enforced in our CI gate.
+
+```
+$ mx checkstyle
+```
+
+### FindBugs
+
+[FindBugs](http://findbugs.sourceforge.net) looks for potential Java
+programming errors. We run it with the default Graal project configuration.
+Passing FindBugs is enforced in our CI gate.
+
+```
+$ mx findbugs
+```
+
+## Ruby
+
+### Rubocop
+
+[Rubocop](https://github.com/rubocop-hq/rubocop) enforces a Ruby style guide.
+It's configured in `.rubocop.yml`, and can be run locally as `jt rubocop`.
+Passing Rubocop is enforced in our CI gate.
+
+```
+$ jt rubocop
+```
+
+### Fasterer
+
+[Fasterer](https://github.com/DamirSvrtan/fasterer) looks for potential
+performance improvements. We occasionally run this tool locally but only take
+its output as a suggestion. We use a default configuration.
+
+```
+$ gem install fasterer
+$ fasterer lib/truffle lib/cext src/main
+```
+
+### Reek
+
+[Reek](https://github.com/troessner/reek) looks for patterns of Ruby code
+which could be improved - generally around making it simpler and more clear.
+We occasionally run this tool locally but only take its output as a
+suggestion. We disable a lot of the defaults in `.reek.yml`, either because
+we're implementing a set API, because we're doing something low-level or
+outside normal Ruby semantics, or for performance reasons.
+
+```
+$ gem install reek
+$ reek lib/truffle lib/cext src/main
+```
+
+### Flog
+
+[Flog](http://ruby.sadi.st/Flog.html) lists methods by complexity. You can
+check that your methods do not appear near the top of this list. We
+occasionally run this tool locally but only take its output as a suggestion.
+
+```
+$ gem install flog
+$ flog -m -t 10 lib/truffle lib/cext src/main
+```
+
+### Flay
+
+[Flay](http://ruby.sadi.st/Flay.html) finds similar or identical code, which
+could potentially be factored out. We occasionally run this tool locally but
+only take its output as a suggestion.
+
+```
+$ gem install flay
+$ flay lib/truffle lib/cext src/main
+```
+
+### Tools we don't use
+
+* [Brakeman](https://github.com/presidentbeef/brakeman) appears to only be
+  applicable to Rails applications.

lib/truffle/bigdecimal.rb

@@ -101,42 +101,36 @@ def coerce(other)
     [BigDecimal(other, 20), self]
   end
 
-  # TODO (pitr 28-may-2015): compare with pure Java versions
   def >(other)
-    unless comp = (self <=> other)
-      return false if nan? || (BigDecimal === other && other.nan?)
-      raise ArgumentError, "comparison of #{self.class} with #{other.class}"
-    end
-
-    comp > 0
+    comp_helper(other, :>)
   end
 
   def >=(other)
-    unless comp = (self <=> other)
-      return false if nan? || (BigDecimal === other && other.nan?)
-      raise ArgumentError, "comparison of #{self.class} with #{other.class}"
-    end
-
-    comp >= 0
+    comp_helper(other, :>=)
   end
 
   def <(other)
-    unless comp = (self <=> other)
-      return false if nan? || (BigDecimal === other && other.nan?)
-      raise ArgumentError, "comparison of #{self.class} with #{other.class}"
-    end
-
-    comp < 0
+    comp_helper(other, :<)
   end
 
   def <=(other)
-    unless comp = (self <=> other)
-      return false if nan? || (BigDecimal === other && other.nan?)
-      raise ArgumentError, "comparison of #{self.class} with #{other.class}"
-    end
+    comp_helper(other, :<=)
+  end
 
-    comp <= 0
+  def comp_helper(other, op)
+    comp = (self <=> other)
+    if comp
+      # cmp >= 0, comp < 0, etc
+      comp.send(op, 0)
+    else
+      if nan? || (BigDecimal === other && other.nan?)
+        return false
+      else
+        raise ArgumentError, "comparison of #{self.class} with #{other.class}"
+      end
+    end
   end
+  private :comp_helper
 
   def nonzero?
     zero? ? nil : self

lib/truffle/date.rb

@@ -258,8 +258,7 @@ class Infinity < Numeric # :nodoc:
 
     def initialize(d=1) @d = d <=> 0 end
 
-    def d() @d end
-
+    attr_reader :d
     protected :d
 
     def zero? () false end
@@ -1144,7 +1143,7 @@ def initialize(ajd=0, of=0, sg=ITALY)
   end
 
   # Get the date as an Astronomical Julian Day Number.
-  def ajd() @ajd end
+  attr_reader :ajd
 
   # Get the date as an Astronomical Modified Julian Day Number.
   def amjd() ajd_to_amjd(@ajd) end

lib/truffle/date/delta.rb

@@ -175,7 +175,7 @@ class << self; alias_method :mins, :minutes end
     class << self; alias_method :secs, :seconds end
 
     def self.parse(str)
-      d = begin (@@pa ||= Parser.new).parse(str)
+      d = begin (@pa ||= Parser.new).parse(str)
           rescue Racc::ParseError
             raise ArgumentError, 'syntax error'
           end
@@ -187,7 +187,7 @@ def self.diff(d1, d2) new(d1.ajd - d2.ajd) end
     class << self
 
       def once(*ids) # :nodoc: -- restricted
-        for id in ids
+        ids.each do |id|
           module_eval <<-"end;"
             alias_method :__#{id.object_id}__, :#{id}
             private :__#{id.object_id}__
@@ -206,8 +206,7 @@ def dhms() self.class.delta_to_dhms(@delta) end
 
     once :dhms
 
-    def delta() @delta end
-
+    attr_reader :delta
     protected :delta
 
     def years() dhms[0] end

lib/truffle/rbconfig-for-mkmf.rb

@@ -109,5 +109,5 @@
 mkconfig['TRY_LINK'] = "#{cc} -o conftest $(INCFLAGS) $(CPPFLAGS) #{cflags_for_try_link} #{cext_dir}/ruby.o #{cext_dir}/sulongmock.o $(src) $(LIBPATH) $(LDFLAGS) $(ARCH_FLAG) $(LOCAL_LIBS) $(LIBS)"
 
 %w[COMPILE_C COMPILE_CXX TRY_LINK].each do |key|
-  expanded[key] = mkconfig[key].gsub(/\$\((\w+)\)/) { expanded.fetch($1, $&) }
+  expanded[key] = mkconfig[key].gsub(/\$\((\w+)\)/) { expanded.fetch($1) { $& } }
 end

lib/truffle/rubygems/extra_executables_installer.rb

@@ -18,11 +18,11 @@ def remove_hooks(install_hook, uninstall_hook)
 
     private
 
-    def with_bin_dir(obj, new_bin_dir, &block)
+    def with_bin_dir(obj, new_bin_dir)
       old_bin_dir = obj.instance_variable_get :@bin_dir
       obj.instance_variable_set :@bin_dir, new_bin_dir
       begin
-        block.call
+        yield
       ensure
         obj.instance_variable_set :@bin_dir, old_bin_dir
       end