x/tools/gopls: inconsistent documentation for builtin `error.Error`

[arkriny]

Currently, error.Error appears to be the only symbol without a doc comment in builtin.go. This causes inconsistency in how gopls behave. Specifically, the signatureHelp method returns a hardcoded string, while other methods like hover and completion show no documentation at all.

[arkriny]

The solution I see is to add a doc comment in builtin.go and adapt gopls to use it. The starting point for the doc text could be the string already used in signatureHelp:

Error returns the error message.

Are there any pitfalls or concerns I might be missing? I’d appreciate feedback on this and would be glad to help with the implementation.

[xieyuschen]

Method Error of interface error doesn't have any doc comment, so it's reasonable for gopls to print nothing and it's consistent with non-builtin symbols.

go/src/builtin/builtin.go

Lines 306 to 310 in 6c3b5a2

	// The error built-in interface type is the conventional interface for
	// representing an error condition, with the nil value representing no error.
	type error interface {
	Error() string
	}

You may send a CL to add your proposed comment, and discuss with the maintainer. But in my view the method name is already self-explained. It's not a gopls issue.

[arkriny]

Method Error of interface error doesn't have any doc comment, so it's reasonable for gopls to print nothing and it's consistent with non-builtin symbols.

Maybe I wasn’t clear enough in my description. Currently gopls behavior is not consistent across methods specifically with error.Error symbol. For signatureHelp it shows a hardcoded string that does not appear anywhere else.

https://github.com/golang/tools/blob/b5e741b9516cb9b03a3a4e43dcfe01146dfe82cf/gopls/internal/golang/signature_help.go#L132

It's not a gopls issue.

gopls issue is in inconsistency. It should be resolved by making gopls respect the builtin.go file and stop handling error.Error as a special case. However, this would cause signatureHelp to lose the existing doc, which, as @adonovan pointed out, would be a step back. Therefore, I believe the best option is to add a doc comment to builtin.go and use it consistently across all LSP methods.

[adonovan]

The builtins.go file does follow normal documentation convention for interfaces (see e.g. io.Reader), so I don't think we should add a doc comment just for gopls's sake. We should do whatever we would do for io.Reader.Read. Currently, I suspect that is nothing, but perhaps it should be something else.