Tomasz Węgrzanowski's Blog

Open Source Adventures: Episode 76: Ameba linter for Crystal

Tomasz Węgrzanowski's photo
Tomasz Węgrzanowski
·

5 min read

Ameba is a linter for Crystal.

I wrote a bit of Crystal code recently, so let's see how it goes.

I'll run it in both default settings and with --all. Hopefully there will be few false positives with default settings.

Shebang issue

ameba was ignoring Crystal scripts starting with #!/usr/bin/env crystal - it was only looking at files with .cr extension. This is not Crystal's main use case, but it's a valid use case.

Crystal's VSCode extension has the same issue.

The issue can be solved with .ameba.yml config file.

crystal-z3 and Lint/UselessAssign

In default settings it finds one issue:

$ ameba
Inspecting 25 files

......................F..

src/z3/api.cr:89:7
[W] Lint/UselessAssign: Useless assignment to variable `var`
> var = LibZ3.mk_const(Context, name_sym, sort)
  ^-^

Finished in 223.71 milliseconds
25 inspected, 1 failure

It doesn't show any context, but it's from this method:

def mk_const(name, sort)
  name_sym = LibZ3.mk_string_symbol(Context, name)
  var = LibZ3.mk_const(Context, name_sym, sort)
end

That's definitely real.

crystal-z3 with --all and Lint/ComparisonToBoolean

This generates a huge number of Lint/ComparisonToBoolean issues, some examples here:

spec/bool_spec.cr:10:6 [Correctable]
[W] Lint/ComparisonToBoolean: Comparison to a boolean is pointless
> [a ==  true, b ==  true, c == (a & b)].should have_solution({c =>  true})
   ^--------^
spec/model_spec.cr:15:19 [Correctable]
[W] Lint/ComparisonToBoolean: Comparison to a boolean is pointless
> solver.assert a == true
                ^-------^
[W] Lint/ComparisonToBoolean: Comparison to a boolean is pointless
> raise Z3::Exception.new("Cannot evaluate") unless result == true
                                                    ^------------^

Well, crystal-z3 overrides Z3::BoolExpr#== so semantically this lint rule is a miss, but I cannot blame ameba for not supporting such unusual code.

I'm not sure this is a good rule in general. As long as x can be anything else than a boolean, x == true is absolutely not the same thing as x.

Thue Interptetter and Performance/CompactAfterMap

There are two issues it found with Thue interpretter from the series:

./episode-65-crystal-thue-randomized-finite-automaton/thue_rfa.cr:162:27
[C] Performance/CompactAfterMap: Use `compact_map {...}` instead of `map {...}.compact`
> next_tries = active.map{|t| t[char]}.compact
                      ^-----------------------^

compact_map is a decent suggestion, as these merged common operations tend to be more performant, but methods like that keep getting added every minor version of every language, it's hard to remember which language has which combinations.

./episode-65-crystal-thue-randomized-finite-automaton/thue_rfa.cr:191:13
[W] Lint/UselessAssign: Useless assignment to variable `line_no`
> line, line_no = lines.shift
        ^-----^

I don't like this one. Useless single variable assignment can be removed, so that makes sense, but in array destructuring, the alternatives are ugly code like one of:

line, _ = lines.shift
line, _line_no = lines.shift
line = lines.shift[0]

I'd rather stick with the original.

But that's not specific to ameba, most linters' "Useless assignment to variable" check don't have exception for multiple assignment.

For a note, if it was a named tuple, we could do some equivalent of (JavaScript here):

let {line} = lines.shift()

For destructuring a hash / named tuple / object, there's no reason to include these extras.

Thue Interpretter and --all

As we're not using --all, false positives are expected. Here's one example, there's more cases like that:

./episode-65-crystal-thue-randomized-finite-automaton/thue_rfa.cr:221:5 [Correctable]
[C] Style/GuardClause: Use a guard clause (`return unless debug`) instead of wrapping the code inside a conditional expression.
> if debug
  ^^

So linter wants us to replace this:

  def run(debug)
    @state = @initial
    if debug
      @rules.each do |rule|
        STDERR.puts rule
      end
    end

    while match = @rfa.not_nil!.random_match(@state)
      rule = match[:rule]
      idx = match[:idx]
      if debug
        STDERR.puts "Applying rule #{rule} at #{idx} to #{@state.inspect}"
      end
      @state = rule.apply(@state, idx)
    end

    if debug
      STDERR.puts "No more matches. Final state: #{@state.inspect}"
    end
  end

With this:

  def run(debug)
    @state = @initial
    if debug
      @rules.each do |rule|
        STDERR.puts rule
      end
    end

    while match = @rfa.not_nil!.random_match(@state)
      rule = match[:rule]
      idx = match[:idx]
      if debug
        STDERR.puts "Applying rule #{rule} at #{idx} to #{@state.inspect}"
      end
      @state = rule.apply(@state, idx)
    end

    return unless debug
    STDERR.puts "No more matches. Final state: #{@state.inspect}"
  end

That would not be an improvement. Guard clauses at the beginning of a method or loop body are common pattern, but putting them after the main body is weird.

Open Source Adventures and Style/VerboseBlock

To get ameba to see all the files I need to run it with weird:

$ ameba `git grep -l '#!/usr/bin/env crystal'` `git ls "*.cr"`

Here are some of the suggestions:

episode-65/minesweeper.cr:40:25 [Correctable]
[C] Style/VerboseBlock: Use short block notation instead: `map(&.ite(1, 0))`
> neighbourhood(x, y).map{|v| v.ite(1, 0)}.reduce{|a,b| a+b}
                      ^------------------^

That's Crystal code not directly doable in Ruby. It's a reasonable suggestion.

episode-70/aquarium:17:25 [Correctable]
[C] Style/VerboseBlock: Use short block notation instead: `map(&.[2..].chars)`
> @board = lines[2..].map{|l| l[2..].chars}
                      ^-------------------^

This feels like maybe a bit much, but maybe I could get used to it.

episode-72/dominosa:65:23
[W] Lint/UnusedArgument: Unused argument `type`. If it's necessary, use `_` as an argument name to indicate that it won't be used.
> @dominos.each do |type, vars|
                    ^

This one can use .each_value, but just like with multiple assignment, I don't like the suggestion of @dominos.each do |_type, vars| or @dominos.each do |_, vars|.

episode-68/switches:50:67 [Correctable]
[C] Performance/ChainedCallWithNoBang: Use bang method variant `sort!` after chained `select` call
> puts @nodes.select{|n| model[@switches[n]].to_s == "true" }.sort.join(" ")
                                                              ^--^

I see why it makes sense for performance, but .sort! in the middle of a chain is so weird, I'd rather not.

There weren't any new types of suggestions with --all.

Should you use ameba?

Overall it seems like a decent linter, and defaults don't generate too many false positives.

It doesn't do any kind of deep analysis. For example I know I had some if checks that are always true due to type issue (Z3 results LBool enum, not Bool, so result is always true), but ameba never picked that up, as that doesn't show in a purely syntactic check.

Coming next

OK, that's really enough Crystal for now. In the next episode we'll try another technology as promised.

crystal