db3d6bfa

By: Tom Sydney Kerckhove <syd@cs-syd.eu>

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

The standards say to put the pieces of a concatenation in a list and call
concat, unwords or unlines. <> does not format well and a literal list
does: it shows its own shape, a piece added later is an element rather
than an operator in the right place, and whether there is a space
between two pieces is something a reader reads rather than counts.

Both operators, because ++ is otherwise the way around the rule and
concatenates the same things. They also run together, since the fix is
the same list either way, so `"at " ++ s <> "."` is one finding of three
operands rather than two findings sharing one.

The types tier this rule was filed under does not exist yet, so what
ships is the literal-operand approximation the plan already sanctioned: a
concatenation with a string literal as one of its operands. On ++ that is
exact, since the only IsString instance for a list of characters is the
one for String. On <> it is as far as a parser goes, and what it costs is
the lazy representations: <> on a Data.Text.Lazy.Builder or a lazy
ByteString appends a chunk rather than copying, and is the operator that
type is for. Five sites of 575 over the corpus, and what the suppression
is for.

Read off the parse tree rather than the token stream, which is the
opposite of how the other syntactic facts are read and is load-bearing: 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 everywhere in the corpus, in chunk, in toHtml, in
textValue, and a token scan would have called all of it a violation.

The tree is no help either, though, and reading the operands off it
directly was wrong. 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 $. That is the most common way this code is written, and it hid 139
of the 575 findings. So the whole infix spine is flattened in source
order and the maximal runs of concatenation are read off that: fixity
regroups operands but never reorders them, so source order is the part
that survives it. A spine holding two unrelated concatenations reports
twice, and the span is the concatenation rather than the enclosing
expression.

A run is one finding rather than one per operator, because two findings
inside one statement cannot both be answered: the second suppression a
reader wrote would be one that suppresses nothing. Parentheses are peeled
before the spine is read, so ("a" <> t) <> "b" is one run too.

This is the first rule to want a fact extraction did not produce, so it
brings ConcatChain, the traversal that fills it, and syb for that
traversal: 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. The operand shapes stay in the fact rather than
collapsing to the answer, so the types tier widens what an operand can
be without changing what the rule decides.

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 with the standard named as the reason.