A few weeks ago I used the golang tool deadcode to find and remove unreachable code from the DNSControl project. I also fixed some unrelated unused parameter warnings. This was my first experience with the utility and it was a good experience.

What is deadcode?

deadcode is a utility that detects unreachable Go code. Read this excellent blog post for more info.

What is DNSControl

DNSControl is “infrastructure as code” for DNS. It supports 40+ DNS providers. More info on https://dnscontrol.org/ or Github.

Installation

Installing deadcode is easy. Just do:

1

go install golang.org/x/tools/cmd/deadcode@latest

Starting state

This is the “before” picture. Deadcode found many, many, functions that are not used.

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32

$ deadcode .
models/dnsrr.go:20:6: unreachable func: RRstoRCs
models/unmanaged.go:32:6: unreachable func: DebugUnmanagedConfig
pkg/credsfile/providerConfig.go:22:6: unreachable func: quotedList
pkg/credsfile/providerConfig.go:29:6: unreachable func: keysWithColons
pkg/diff2/analyze.go:318:6: unreachable func: justMsgString
pkg/diff2/compareconfig.go:177:26: unreachable func: CompareConfig.String
pkg/diff2/diff2.go:261:17: unreachable func: Change.String
pkg/diff2/diff2.go:281:22: unreachable func: ChangeList.String
pkg/diff2/groupsort.go:13:6: unreachable func: groupbyRSet
pkg/powershell/backend/ssh.go:28:15: unreachable func: SSH.StartProcess
pkg/powershell/backend/ssh.go:52:15: unreachable func: SSH.createCmd
pkg/powershell/backend/ssh.go:67:15: unreachable func: SSH.quote
pkg/powershell/middleware/utf8.go:21:6: unreachable func: NewUTF8
pkg/powershell/middleware/utf8.go:29:16: unreachable func: utf8.Execute
pkg/powershell/middleware/utf8.go:47:16: unreachable func: utf8.Exit
pkg/prettyzone/prettyzone.go:48:6: unreachable func: WriteZoneFileRR
pkg/printer/printer.go:57:6: unreachable func: Errorf
pkg/rejectif/caa.go:31:6: unreachable func: CaaTargetHasSemicolon
pkg/spflib/parse.go:16:21: unreachable func: SPFRecord.Lookups
pkg/spflib/parse.go:105:6: unreachable func: dump
pkg/spflib/parse.go:125:21: unreachable func: SPFRecord.Print
providers/cloudflare/cloudflareProvider.go:830:6: unreachable func: PrepareCloudflareTestWorkers
providers/cloudflare/rest.go:361:30: unreachable func: cloudflareProvider.createTestWorker
providers/doh/auditrecords.go:8:6: unreachable func: AuditRecords
providers/internetbs/auditrecords.go:8:6: unreachable func: AuditRecords
providers/loopia/types.go:19:22: unreachable func: paramString.param
providers/loopia/types.go:26:19: unreachable func: paramInt.param
providers/loopia/types.go:34:22: unreachable func: paramStruct.param
providers/loopia/types.go:45:29: unreachable func: structMemberString.structMember
providers/loopia/types.go:52:26: unreachable func: structMemberInt.structMember
providers/opensrs/auditrecords.go:8:6: unreachable func: AuditRecords

Which editor?

I switch between vim and VSCode. I use vim when I need to serious work done, and VSCode when I want the editor to do work for me.

I like vim because it is snappy and responsive. Because I have decades of experience with it, my fingers just know what to do.

I like VSCode because it reports warnings from a variety of tools, it has a great “rename symbol” command, an amazing “refactor” feature, and it has a VIM-emulator plug-in that isn’t perfect, but makes my fingers happy.

Even when I’m using vim I still keep VSCode running on a separate monitor because the warnings and other messages it displays in the “problems” tab are extremely useful.

Another thing I like about VSCode is the -g command line option.

1

code -g providers/cloudflare/rest.go:361:30:

opens the file at that line and column. The filename:row:col format is used by deadcode and many other tools.

I used the -g option to quickly move to each of deadcode’s findings.

FYI: You can do something similar with vim: vim +361 providers/cloudflare/rest.go opens the file and scrolls down to that line, but this is more work because it requires 2 copy-and-paste operations instead of one.

Unused Parameters reveals a bug

I started up VSCode which reported a number of unused parameter issues:

This warning does not come from deadcode but I’m glad I went on this detour from my original plan because I found a bug!

The warning about targetSpec indicated that the integration tests for IGNORE() ignored the 2nd parameter! It always tested “all types” instead of the one type you’ve specified. Oops!

The fix looked like this:

1
2
3
4

 func ignoreTarget(targetSpec string, typeSpec string) *models.RecordConfig {
-       return ignore("*", "*", targetSpec)
+       return ignore("*", typeSpec, targetSpec)
 }

After commiting this change I ran the full integration test suite via GitHub Actions (GHA).

Those tests verify some algorithms related to deleting data. I feared that once the tests were fixed I’d learn that the code was not deleting what it was supposed or (my real fear) it was deleting things that shouldn’t be deleted!

I breathed a sigh of relief when the tests completed without error.

This has nothing to do with deadcode but I thought I’d mention it.

Unused parameters trick

“And the second trick will amaze you!”

In most cases an “unused parameter” could just be removed from the definition and all uses of the function. I found it was best to remove it from the implementation or interface, then let VSCode tell you all the (now) invalid uses of that function. It was then easy to click on each VSCode “problem” and delete the (now unneeded) parameter.

However some situations I could not change the function signature. The parameter was needed to meet an interface, even though the paramter would go unused.

One way to fix this is to contrive a use for the variable:

1

   _ = otherwiseUnusedVariable

However a better way is to use _ as the name in the function signature.

1

func newRoute53(m map[string]string, _ json.RawMessage) (*route53Provider, error) {

Da da!

Removing dead code

Next I went through the deadcode output and looked into each complaint.

Rather than deleting the dead code, I would rename the variable by adding an “x” to the start of the name and see if the code no longer compiled.

If it did compile, I would run unit and integration tests. If all that succeeded I would delete the function. Or, in some cases, I would comment it out because the function was for debugging and I might want to bring it back some day.

I committed all the changes individually (or in groups) so that I could revert any one individually in the future if a problem was found. (Then I had to remind myself not to use the “squash” function in Github when I merged the final PR.

Test functions

One interesting case was RRstoRCs. This function was unused by the main code, but the unit tests need it. Therefore, I moved it to the appropriate _test.go file.

deadcode normally doesn’t examine the _test.go files. Add the -test flag to test those. It was interesting to see what warnings appeared/disappeared with that flag.

justMsgString and possibly others fell into this category.

The difficult cases

Most of the deadcode warnings were easy to fix. Some were more difficult. Two were impossible.

This was was interesting:

1
2

providers/cloudflare/cloudflareProvider.go:852:6: unreachable func: PrepareCloudflareTestWorkers
providers/cloudflare/cloudflareProvider.go:868:30: unreachable func: cloudflareProvider.createTestWorker

After studying the code and exprimenting with various potential fixes I had to give up. It turns out these two functions are layering violations that exists to facilitate integration testing. The layering violations are needed because of a bad design decision early on in DNSControl history.

I tried refactoring the functions themselves with no luck. It just doesn’t seem possible.

Is it a bug that deadcode lists those with and without -test? I’m not sure. I would expect the warning to go away with the -test flag.

The remaining problems are all related:

1
2
3
4
5

providers/loopia/types.go:19:22: unreachable func: paramString.param
providers/loopia/types.go:26:19: unreachable func: paramInt.param
providers/loopia/types.go:34:22: unreachable func: paramStruct.param
providers/loopia/types.go:45:29: unreachable func: structMemberString.structMember
providers/loopia/types.go:52:26: unreachable func: structMemberInt.structMember

The code related to those items is super complex. I poked around but decided it was too risky to remove them. I don’t have a test account on Loopia to verify my changes didn’t break anything.

I do have an awesome volunteer that maintains the Loopia plug-in, and they have a test account. I’ll be filing a bug asking them to look into it.

The .String() function

I got a number of warnings about .String() functions that were unused. I’m very nervous about removing them, because while nobody is using them outright, there might be fmt.Printf statements that does.

I’m not sure if deadcode is smart about printf’s so I left some of them in. Others I commented out so I can bring them back easily.

Results:

Here are all the changes I made. This list includes both deadcode and “unused parameter” issues that a different utility raised.

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20
21
22
23

* e6c03c09 2024-03-03 cleanup: Remove dead code and unused params [Tom Limoncelli]
* 798c0d6b 2024-03-03 deadcode: Change/ChangeList.String [Tom Limoncelli]
* 0576cec7 2024-03-03 deadcode: Errorf [Tom Limoncelli]
* 4f32ddf1 2024-03-03 deadcode: DebugUnmanagedConfig [Tom Limoncelli]
* 2ff585d4 2024-03-03 deadcode: dump Print [Tom Limoncelli]
* c3580b1d 2024-03-03 unused parameter knownFailures in cfg[], getProvider, runTests [Tom Limoncelli]
* 3b01dc8b 2024-03-03 unused param t in testPermitted() [Tom Limoncelli]
* 822a40c4 2024-03-03 unused params in makeUknown() and formatDsl() [Tom Limoncelli]
* a3dad4d3 2024-03-03 deadcode: RRstoRCs (moved) [Tom Limoncelli]
* 15f77370 2024-03-03 deadcode: providers/opensrs/auditrecords.go (not a DSP) [Tom Limoncelli]
* bdad44f2 2024-03-03 deadcode: providers/internetbs/auditrecords.go (not a DSP) [Tom Limoncelli]
* 1b16cf45 2024-03-03 deadcode: providers/doh/auditrecords.go (not a DSP) [Tom Limoncelli]
* c0d8ca7a 2024-03-03 deadcode: move createTestWorker [Tom Limoncelli]
* ea71a9c3 2024-03-03 deadcode: CaaTargetHasSemicolon [Tom Limoncelli]
* 28ca119b 2024-03-03 deadcode: WriteZoneFileRR [Tom Limoncelli]
* 4d13c025 2024-03-03 deadcode: NewUTF8, utf8.Execute, utf8.Exit [Tom Limoncelli]
* 951b47d5 2024-03-03 deadcode: SSH.StartProcess, SSH.createCmd, SSH.quote [Tom Limoncelli]
* 80ff8149 2024-03-03 deadcode: groupbyRSet [Tom Limoncelli]
* 34d6e079 2024-03-03 deadcode: groupbyRSet [Tom Limoncelli]
* ed999b99 2024-03-03 deadcode: justMsgString (move to test) [Tom Limoncelli]
* e4dc7aaf 2024-03-03 deadcode: quotedList() keysWithColons() [Tom Limoncelli]
* 18f72080 2024-03-03 Remove unused t parameter in setupTestShellCompletionCommand [Tom Limoncelli]
* dfda97b5 2024-03-03 Remove dead code in WriteTypes [Tom Limoncelli]

What was the final reduction?

1
2
3
4
5

-rwxr-x---  1 tlimoncelli  staff  53425744 Mar 11 16:26 dnscontrol.NEW_SMALLER
-rwxr-x---  1 tlimoncelli  staff  53429680 Mar 11 16:25 dnscontrol.OLD_BIG
$ echo $(( 53429680 - 53425744 ))
3936
$

Congrats! The new version is 4k smaller!

Ok, that wasn’t the huge savings I was hoping for. At least the code is cleaner now. Plus, I found and fixed some bugs.

Final thoughts

I’m impressed by deadcode. It is very fast and impressively accurate.

In this case, the size reduction was tiny. However the bugs that I happened to find while removing the dead code was a good thing.

Some functions were impossible to remove. As the help page says, “In any case, just because a function is reported as dead does not mean it is unconditionally safe to delete it.” You have to use your best judgement.

I’m unsure if .String() functions are safe to remove. It would be nice if printf format strings were analyzed. I’m also not sure if it is a bug that PrepareCloudflareTestWorkers is listed as dead when the -test flag is included.

There’s no obvious place to file bug reports about deadcode. The -h page doesn’t tell you, nor does the the documentation.

It would be nice if there was a //nolint: equivalent for deadcode so that I don’t have to see warnings about the “impossible” functions that I don’t want to remove. Of course, I’d like a flag that ignores such comments.

If I ever do get to the point where deadcode finds no issues, I’d add this to my CI/CD pipeline to warn me if new dead code appears. However I can’t do that until some kind of //nolint option appears.

Bottom line: Dead code is bad. deadcode is good.