format.go nilToken not "thread" safe

[TomerHeber]

While running some tests I noticed the following issue in format.go

		for i, token := range line.lead {
			var before, after *Token
			if i > 0 {
				before = line.lead[i-1]
			} else {
				before = nilToken
			}
			if i < (len(line.lead) - 1) {
				after = line.lead[i+1]
			} else {
				after = nilToken
			}
			if spaceAfterToken(token, before, after) {
				after.SpacesBefore = 1
			} else {
				after.SpacesBefore = 0
			}
		}

If after gets nilToken it will be modified by after.SpacesBefore = 0 or after.SpacesBefore = 1.
When two or more goroutines run this will cause a race condition.

nilToken should probably not be a global variable.
Happy to fix it if that makes sense.

[apparentlymart]

Hi @TomerHeber! Thanks for reporting this.

It seems like even if not running this codepath concurrently it's still incorrect for this to modify nilToken, since indeed it's supposed to be a fixed placeholder value.

I believe the intent of a single shared nilToken here was to avoid repeatedly allocating lots of placeholder objects whenever working at the beginning or end of a line, and that would be defeated if allocating it locally at the time of use, although it could be reasonable for the formatSpaces function to allocate just a single nilToken which lives for the duration of that call since formatSpaces is called only once per formatting call and so a single allocation per call could be reasonable.

Looking at the control flow here though, it seems like another plausible option would be to replace after = nilToken with continue in both of the loops, since if there isn't any after token then there is no useful work to do here anyway: this codepath's only purpose is to modify after. I think that would address the problem you identified without any changes to the allocation behavior.

Based on that reasoning, the last change I described (replacing with continue) would be my favored path forward here. Does that seem reasonable to you too, or can you see something I'm missing that makes that not a suitable answer?

If you'd be willing to write a PR for that, I'd be happy to review and merge it. Thanks!

[TomerHeber]

@apparentlymart I've created PR #534

[apparentlymart]

Oh, whoops! I forgot to close this after I merged #534, but this is now fixed in the main branch in preparation for the next release. Thanks @TomerHeber!