b228758f

Report <> and ++ used to concatenate strings or text

Ships `HsNoSemigroupOnText`, the sixth rule.

## What it reports

A concatenation with a string literal as one of its operands, `<>` or `++`.
Both operators, because `++` is otherwise the way around the rule and
concatenates the same things, and they run together since the fix is the same
list either way: `"at " ++ s <> "."` is one finding of three operands.

The rule was filed as tier `types` and the types tier is not built, so what
ships is the literal-operand approximation `plan/plan.md` already sanctioned for
M5. On `++` that approximation is exact, since the only `IsString` instance for
a list of characters is the one for `String`. M7 widens it to the
concatenations with no literal in them, which is where `T.pack x <> y` and
`show x <> y` live.

## Three choices worth reviewing

**Read off the parse tree, not the token stream.** Every other syntactic fact in
this tool comes from the tokens, and here that would be wrong: a token before an
operator is not an operand of it, so `text "a" <> b` has a string literal beside
the `<>` while concatenating whatever `text` returns. That shape is all over the
corpus (`chunk ("Building " <> x)`, `toHtml ("Status: " <> s)`,
`textValue ("#" <> slug)`), and a token scan calls all of it a violation.

**Flatten the spine, do not trust its shape.** `GhcPs` has resolved no fixity:
it nests every infix application to the left whatever the real associativity is,
so `putStrLn $ "no such thing: " ++ what` arrives as
`(putStrLn $ "no such thing: ") ++ what` and the literal is an operand of the
`$`. Reading the operands straight off the tree missed that, and it is the most
common way this code is written: **139 of the 575 findings were invisible** until
the spine was flattened in source order instead. Fixity regroups operands but
never reorders them, so source order is the part that survives it. Regression
resource: `badBehindDollar.hs`.

**One finding per run, not per operator.** Two findings inside one statement
cannot both be answered: the second suppression a reader wrote would be one that
suppresses nothing. So `"at " <> path <> ": " <> msg` reports once. A spine
holding two unrelated concatenations still reports twice, with disjoint spans:
`"x" <> a == "y" <> b`.

## Corpus

575 findings over five repositories:

| | |
|---|---|
| smos | 232 |
| sydtest | 178 |
| centjes | 90 |
| autodocodec | 64 |
| feedback | 11 |

A sample of fifty `<>` sites was a `String` or a `Text` every time. The one class
where the rule asks for something it should not is the lazy representations: a
`<>` on a `Data.Text.Lazy.Builder` or a lazy `ByteString` appends a chunk rather
than copying, so it is the operator that type is for. Five sites of the 575, and
`<>` only. `ratchet` for anyone adopting it.

## What it cost here

Fourteen sites in this repository were the rule's own subject, almost all of
them a piece of punctuation appended inside a `unwords` list, and they are now
written as `concat`. hlint's `Use ++` hint rewrites exactly that back into the
operator, so the two cannot both be satisfied and the hint is turned off in
`.hlint.yaml` with the standard named as the reason.

29 lines for the rule, but it is the first one to want a fact extraction did not
produce, so it touched five Haskell files rather than the budgeted one:
`ConcatChain` and the `ModuleContext` field, the extraction that fills them, the
generator, the spec over the generator, and the registry. It also brings `syb`,
because finding every concatenation in a module means walking the whole
expression tree and hand-writing that over `HsExpr` would have been most of the
rule.

## Known gaps

- The section `("a" <>)` and prefix `(<>) "a" x` forms are missed.
- `` a `mappend` "b" `` is missed. It cannot survive here because hlint rewrites
  it to `<>`, but a repository without that hint has a hole.
- `ruleWhy` names only `<>`, though the rule also reports `++`.

`nix flake check` passes.

Total

4h36m59s

Created 1 month ago Started 1 month ago Finished 1 month ago

Timeline