DEV Community

Tomasz Wegrzanowski
Tomasz Wegrzanowski

Posted on

Open Source Adventures: Episode 76: Ameba linter for Crystal

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
Enter fullscreen mode Exit fullscreen mode

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
Enter fullscreen mode Exit fullscreen mode

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
                                                    ^------------^
Enter fullscreen mode Exit fullscreen mode

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
                      ^-----------------------^
Enter fullscreen mode Exit fullscreen mode

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
        ^-----^

Enter fullscreen mode Exit fullscreen mode

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]
Enter fullscreen mode Exit fullscreen mode

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()
Enter fullscreen mode Exit fullscreen mode

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
  ^^
Enter fullscreen mode Exit fullscreen mode

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
Enter fullscreen mode Exit fullscreen mode

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
Enter fullscreen mode Exit fullscreen mode

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"`
Enter fullscreen mode Exit fullscreen mode

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}
                      ^------------------^
Enter fullscreen mode Exit fullscreen mode

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}
                      ^-------------------^
Enter fullscreen mode Exit fullscreen mode

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|
                    ^
Enter fullscreen mode Exit fullscreen mode

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(" ")
                                                              ^--^
Enter fullscreen mode Exit fullscreen mode

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.

Top comments (0)