x/tools/gopls/internal/analysis/modernize: stringsbuilder breaks code where string is reassigned

[jakebailey]

We have code that looks like this:

func getLibraryNameFromLibFileName(libFileName string) string {
	// Support resolving to lib.dom.d.ts -> @typescript/lib-dom, and
	//                      lib.dom.iterable.d.ts -> @typescript/lib-dom/iterable
	//                      lib.es2015.symbol.wellknown.d.ts -> @typescript/lib-es2015/symbol-wellknown
	components := strings.Split(libFileName, ".")
	var path string
	if len(components) > 1 {
		path = components[1]
	}
	i := 2
	for i < len(components) && components[i] != "" && components[i] != "d" {
		path += core.IfElse(i == 2, "/", "-") + components[i]
		i++
	}
	return "@typescript/lib-" + path
}

Note the path = components[1]

But, the new modernizer converts this to:

func getLibraryNameFromLibFileName(libFileName string) string {
	// Support resolving to lib.dom.d.ts -> @typescript/lib-dom, and
	//                      lib.dom.iterable.d.ts -> @typescript/lib-dom/iterable
	//                      lib.es2015.symbol.wellknown.d.ts -> @typescript/lib-es2015/symbol-wellknown
	components := strings.Split(libFileName, ".")
	var path strings.Builder
	if len(components) > 1 {
		path = components[1]
	}
	i := 2
	for i < len(components) && components[i] != "" && components[i] != "d" {
		path.WriteString(core.IfElse(i == 2, "/", "-") + components[i])
		i++
	}
	return "@typescript/lib-" + path.String()
}

Which is incorrect as you can't assign to a strings.Builder. I think the analysis needs to check to verify that the string is never assigned to, only starts as some constant.

cc @alandonovan

[jakebailey]

Alternatively, the fix could insert path.Reset() and perform another write? Probably ideally this would detect that nothing was ever written and skip the reset. Seems tough...

[adonovan]

Thanks for the bug report.

Sigh, someone should give a talk on how modernizers are hard, let's go shopping.

[gopherbot]

Change https://go.dev/cl/700900 mentions this issue: gopls/internal/analysis/modernize: stringsbuilder: fix s=x bug