LLMs and almost good code
tl;dr: My new prior is that top-of-the-line llms working on easy tasks<br>generate code that is maybe 10 % more complicated than necessary. I also think<br>we accept this complexity too easily, because it comes from code that is right<br>here, right now, solving an immediate problem. This may have consequences<br>for maintenance in the long term.
The background to this discovery was that I needed to do some crud plumbing in<br>a work project. It was a simple change that mostly mirrored existing<br>functionality. This is a perfect fit for llms, in my experience, so I used a<br>frontier model to generate the code for it. The change ended up being a total of<br>just over 200 lines, mostly additions.
The part of the generated code we’ll talk about is a 24-line function that<br>converts an arbitrary (user-supplied) string to a safe http header<br>value.11 Encoding it into a safe value is necessary to avoid confusing<br>mistakes, but also to prevent http header injection attacks.
In[1]:<br>toHeaderValue :: Text -> Text<br>toHeaderValue raw =<br>let<br>attrChars = "!#$&+-.^_`|~"<br>padHex t = if Text.length t 2 then "0" <> t else t<br>percentEncode c =<br>if (isAscii c && isAlphaNum c) || elem c attrChars then<br>Text.singleton c<br>else<br>Text.concat<br>[ "%" <> padHex (Text.toUpper (Text.pack (showHex b "")))<br>| b ByteString.unpack (encodeUtf8 (Text.singleton c))<br>rfc5987Encode = Text.concatMap percentEncode<br>isPrintable c = c >= ' ' && c /= '\DEL'<br>replacePathSeparator c =<br>if c == '/' || c == '\\' then<br>'_'<br>else<br>cleaned =<br>Text.map replacePathSeparator (Text.filter isPrintable raw)<br>in<br>rfc5987Encode cleaned
When looking at this function like this, it obviously seems a bit too<br>complicated, but recall that this was just 24 lines in a 200-line change. I<br>confirmed that the underlying idea was correct, and that the generated tests<br>covered all the edge cases I would want to see covered. It’s not pretty code,<br>but it is proven correct by tests.
More importantly, it is highly local. If anything about this code needs<br>replacing, it can be replaced without touching anything else. Apprentice-level<br>programmers worry equally about code quality everywhere; I’ve long wanted to<br>write an article called “Don’t worry, it’s local” where I tell these<br>programmers that bad code quality is fine, as long as it’s self-contained in a<br>small location.22 The reason I haven’t written that article is that I’m<br>waiting on a bunch of examples the article would need. But I keep forgetting to<br>collect them, so the total number of examples I have collected so far are zero.
I accepted this code. I needed the implementation to work, and this code<br>obviously worked. It was right there, right now. It would have been silly to<br>not accept it! Accepting it was the easy choice, and certainly not a bad<br>decision.
However, in a pleasant twist of fate, the ci pipeline for this project has a<br>mandatory statement test coverage check33 I’m not a huge fan of statement<br>coverage, but that’s another separate article I’ve put off writing for years.,<br>and that check failed for this code. See if you can spot where and why.
I’ll give you a hint: it has to do with the padHex function, which takes a<br>hexadecimal value in the range 0x0–0xff and zero-pads it if it is less than<br>0x10.
The data passed into padHex has already gone through the isPrintable filter,<br>which removes all bytes lower than 0x20. Thus no value passed to padHex is<br>ever below 0x10, and it never ends up padding anything! It is always a no-op.<br>The statement coverage check warns on the padding branch of padHex, because it<br>is exercised by no test. It is in fact impossible to exercise it in a test.
This was annoying:
On the one hand, we shouldn’t assume percentEncode is always called with<br>characters greater than 0x1f, even if that happens to be true at the moment.<br>Such an assumption relies on spooky action at a distance, which – even if it<br>is local to this function – we want to avoid.
On the other hand, the coverage report is right too: there is something<br>awkward about this whole construction.
So I stepped in and wrote my own implementation. The implementation that ended<br>up shipping was closer to this:
In[2]:<br>toHeaderValue :: Text -> Text<br>toHeaderValue =<br>let<br>retainPrintable = Text.filter (\c -> c >= ' ' && c /= '\DEL')<br>replacePathSeparators = Text.replace "/" "_" . Text.replace "\\" "_"<br>-- URL encoding is also legal RFC5987 encoding.<br>rfc5987Encode = decodeUtf8 . urlEncode True . encodeUtf8<br>in<br>rfc5987Encode . replacePathSeparators . retainPrintable
This is 15 lines of complexity shorter. That’s around 8 % of the change.
The llm did not generate bad code.44 In some sense, its code is better. The<br>rfc 5987 encoding is more lax than url encoding, so my implementation<br>technically over-encodes. It just generated code that was 8 % more complex than<br>it needed to be. That’s not a disaster today, and when there’s pressure to ship,<br>it is easy to accept...