Open Source Adventures: Episode 76: Ameba linter for Crystal
Tags: crystal
Published: 2022-07-18 20:30:09

[Ameba](https://github.com/crystal-ameba/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:

```crystal
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:

```crystal
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):

```javascript
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:

```crystal
  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:


```crystal
  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.