chore: fix all ameba 1.7.0-dev lint findings - #2171
Merged
Merged
Conversation
Drop the `begin` blocks that span a whole method or block body, which Crystal already handles via inline `rescue`/`ensure`. 14 problems, no behaviour change. First entry off the ameba 1.6.4 -> 1.7.0-dev backlog in `.ameba.yml`.
Drop `return` on expressions that already are the last one evaluated in their method. 10 problems, no behaviour change. Each site was checked to be the final expression of its method, or of an `if`/`unless`/`begin` that is itself the method's last expression. None sits inside a block, where dropping `return` would have changed control flow rather than just style. The early guards in `auth_handler` keep their `return`s.
Of the 6 reports only one was a real dead store: an unreachable `error : Exception? = nil` in `Lease#keepalive_loop`, sitting after a `loop` that has no `break`. The rescue clause below it assigns its own local, so the line did nothing. The other 5 are pre-declarations in front of a `begin`/`ensure`. The analyzer does not model the exception edge into `ensure`, so it reads them as dead even though that is exactly the path that uses them. Crystal already nils an unassigned local inside `ensure`, so dropping the declarations keeps the cleanup behaviour identical. `etcd_spec` also reset `stale_lease` to nil after releasing it, which is what made the earlier assignment look dead. The `ensure` releases best-effort with `rescue nil` and `Channel#close` is idempotent, so the reset only saved a redundant `lease_revoke`. Dropping it lets the rule run with no exclusions.
1.7.0-dev scores complexity differently, which pushed two methods over the limit of 12 without their code changing. `Queue#message_expire_loop` (16) was three sequential `select` blocks plus a branch inlined into one loop. Split each into a `wait_for_*` helper that returns false only where the original broke out on an idle timeout, leaving the loop as a flat sequence of guards. This mirrors how `AMQP::Consumer` already decomposes `deliver_loop`. Down to 7, helpers score 3, 3 and 9. `Link#monitor_consumers` (13) gets its upstream cancel, and the `AMQP::Client::Error` it swallows, extracted into `cancel_upstream_consumer`. Down to 11 -- only one under the limit, so the next branch added there will trip it again. The rule now runs with no exclusions.
All 5 flagged directives were `# ameba:disable Metrics/CyclomaticComplexity` that 1.7.0-dev no longer needs, and all 5 were real -- ameba only hid them while the surrounding files were excluded from the complexity rule itself. They are not redundant because the methods got simple, though. Forcing `MaxComplexity: 1` to print scores puts them at 12, 12, 12, 11 and 11 against a limit of 12, so `mime_type`, `Prometheus#register_routes` and `DeadLettering#route` sit exactly on the line and trip the rule again on the next branch added.
The only `Void` outside a `lib` is in the `on_server_name` monkey-patch, on a line copied verbatim from stdlib `openssl/ssl/context.cr`. `Proc(Void)` -> `Proc(Nil)` is representationally identical -- a proc is two pointers whatever it returns, and Crystal normalises a `Void` proc return to `Nil`, so the cast still matches the `fun`'s declared `fp : Proc(Void)`.
The rule wants `Process.on_terminate` instead of `Signal::HUP/INT/TERM.trap`, but that only buys Windows portability and `Launcher#setup_signal_traps` also traps USR1/USR2 and resets SEGV, so it is Unix-only either way. Adopting it would replace three self-explanatory traps with a `case` on `ExitReason` where reload-on-SIGHUP has to be spelled `.terminal_disconnected?`. Its autofix is also unsafe: it rewrites each trap into its own `Process.on_terminate`, and since every call re-registers all three signals, only the last would survive.
All 5 findings are the same shape: a single log message split across lines with
`\` string continuations, not a multi-statement block. `bindings.cr` already
writes that exact shape as `do`...`end`, so this makes the tree consistent
rather than fighting the convention. Message-only, no semantic change.
The 255 single-line `Log.x { }` calls are untouched -- the rule only fires on
multi-line blocks.
All 6 were real, in two shapes.
The two in `lavinmqperf` mutated a running total inside a format string --
`@io.print "#{count += @connections} connections "` -- and the prints that
follow read the post-increment `count`, so the statement order was load-bearing
but invisible. Hoisting the `+=` above the print keeps the same values.
The four in `observable_spec.cr` were the benign
`register_observer(obs1 = Observer.new { ... })` idiom, split into an
assignment and a call.
`JSON::PullParser#read_object` requires the block to consume each value, but the `else` branch of both `case`es in `User` did nothing, so an unrecognised key left the parser positioned on its value and the next `read_object_key` raised `JSON::ParseException`. `UserStore#load!` re-raises, so a single unknown field makes the broker fail to start. That happens on a downgrade -- a newer LavinMQ writes a field an older one does not know -- or on a hand-edited users.json. The definitions import endpoint is unaffected; it parses users as `JSON::Any`. Found while working through the ameba backlog: `Lint/ElseNil` flags exactly these two `else nil` branches, and deleting them would have cemented the bug.
Four remaining findings, all plain deletions -- a `case`/`if` without an `else` already evaluates to nil, so the branch had no effect. `token_claim.cr` is the only one whose value is used: the `case` is assigned to `username`, and without the `else` it still infers `String?`. The other two findings were the `else nil` branches in `User`, fixed separately in the preceding commit.
Type annotations only, no runtime change. Six were autofixed, three hand-done. Multi-type unions are spelled `(A | B)?` rather than the corrector's `A | B?`. Both are the same type -- `?` binds tighter than `|` and unions are flat -- but `Time::Span | Time::MonthSpan?` reads as though only `MonthSpan` is nilable. The three the corrector skipped had the same shape and needed the same parens.
All 12 were real -- no false positives. In every case a neighbouring rescue in the same block does use its variable, so the unused bindings read as copy-paste symmetry rather than intent. Left alone: a few of these swallow detail worth logging, e.g. the `AuthenticationError` rescue in `clustering/server.cr` logs only "Follower negotiation error". Adding `ex.message` would also satisfy the rule but changes log output, so it is a separate change.
All 79 findings were in `spec/`, none in `src/`, and all the same shape: a JSON
request body assigned to a local and then passed to `http.put`/`http.post`/
`JSON.parse`. Converting `%(...)` to a heredoc changes the string, since
heredocs strip common indentation, so every consumption site was checked first
-- none compares the literal, so the whitespace change is inert.
Bodies keep their internal relative indentation, re-based to statement + 2.
That matches the existing heredoc style in `users_spec.cr` and `config_spec.cr`.
One-line `%({...})` and backslash-split `%()` fragments also satisfy the rule,
and 50 of the 79 would fit on a single line, but mixing the two idioms in the
same files by JSON length is worse than being uniform.
No new `Style/HeredocIndent` findings -- still 32, all pre-existing elsewhere.
All 8 autofixed: `return nil` -> `return` in methods already returning a nilable type, which is the same value. `ip_matcher.cr` also needed a reformat -- its multi-line condition was aligned to the old `return nil unless` width.
All 12 autofixed. Most are `self.new(...)` inside a class method, where bare `new` resolves to the same thing. The only ones that could have changed meaning are `self.size` in `clustering/checksums.cr` and `self.compare_value` in `config.cr`, since a local of the same name would shadow the method. Checked both: no such local exists, they resolve to `Checksums#size` and `Option#compare_value`.
All 32 autofixed, spec-only. Each is a uniform +2 shift of the heredoc body and its closing marker; `<<-` dedents by the minimum indentation, so the strings themselves are unchanged. Counted 32, not the 33 from the original sweep.
All 34 autofixed: `should eq true/false/nil` -> `should be_true/be_false/ be_nil`. No change in strictness -- `be_true` and `be_false` are defined as `eq true` and `eq false`. The loose matchers are `be_truthy`/`be_falsey`, which the corrector does not use.
All 46 autofixed: `{{foo}}` -> `{{ foo }}`. Whitespace inside the delimiters
does not affect macro expansion.
This was the last entry, so the 1.6.4 -> 1.7.0-dev backlog section is gone and
`.ameba.yml` is back to just the rules this project has chosen to configure.
viktorerlingsson
changed the title
|
Reviewed the changes. This is predominantly a mechanical lint cleanup (heredoc conversions in specs,
One finding:
The Per the project rule that bug fixes need regression specs, this behavioral fix should have a spec that imports a user (and a permission) with an unrecognized field and asserts it parses successfully. The rest of the PR is covered by existing specs. |
viktorerlingsson
marked this pull request as ready for review
August 7, 2026 07:51
spuun
approved these changes
Aug 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Clears the lint backlog parked at the bottom of
.ameba.ymlduring the Crystal 1.21 upgrade (#2170). Ameba 1.6.4 doesn't compile on Crystal 1.21, so the shard tracks master (1.7.0-dev), which reported 291 problems across 18 rules -- all pre-existing code the newer ameba flags, none of it 1.21 related.One commit per rule, so this reviews best commit by commit. 17 rules fixed, 1 declined. The backlog section is gone and
.ameba.ymlis back to just the rules this project deliberately configures.Worth a closer look
49de9e697is a real bug fix, not a lint change.Lint/ElseNilflagged theelse nilbranches inUser's JSON parsing, butJSON::PullParser#read_objectrequires the block to consume every value. An unknown key inusers.jsontherefore raisedJSON::ParseException, and sinceUserStore#load!re-raises, the broker fails to start -- on a downgrade, or a hand-edited file. Deleting the line as the rule wanted would have cemented it, so it becamepull.skipwith a regression spec. The definitions import endpoint is unaffected; it parses users asJSON::Any.09e04623afound two perf tools mutating a running total inside a format string ("#{count += @connections} connections "), where the prints that follow read the post-increment value -- statement order was load-bearing but invisible.241b053bedeclinesLint/SignalTrap.Process.on_terminateonly buys Windows portability, andsetup_signal_trapsalso traps USR1/USR2 and resets SEGV, so it's Unix-only either way. Note its autofix is unsafe: it rewrites each trap into its ownProcess.on_terminate, and since every call re-registers all three signals, only the last would survive.Known CI failure:
Compile changed *.cr files independentlyThat job fails on
src/lavinmq/amqp/argument/dead_lettering.cr:This is pre-existing and not caused by this PR. The file references
AMQP::Queuewithout requiringqueue/queue.cr, so it has never compiled on its own. Checking outmain's version of the file and building it standalone fails identically. The job only compiles changed files, and the only change here is deleting a now-redundant# ameba:disable Metrics/CyclomaticComplexitycomment -- which was enough to pull the file into the changed set and surface the latent breakage for the first time.Not fixed here, to keep this PR to lint changes. The require cannot simply be added:
queue/queue.crdoesinclude Argument::DeadLettering, so requiring it back is a cycle that fails on theincludeinstead. It needs either a forward declaration ofQueueindead_lettering.cror moving theTaskalias somewhere both files can see.Test plan
2090 -> 2091 is the new regression spec. The 10 pending are pre-existing and unchanged from
main.🤖 Generated with Claude Code