Highlight builtin functions as keywords

[arp242]

This was changed from keywords to the regexp in #605, but I think that
was a mistake.

If you do new := "foo" then this is perfectly valid Go, but I don't
think it's a bad think to add a reminder that you're actually overriding
a global built-in by highlighting the new, just as it's not a bad idea
to highlight the new() in func new() to serve as a similar reminder.

This is a common-ish mistake that most of us have probably made at least
once or twice.

The only case where this fails is in method declaration and calls:

func () x new() {
    other.new()
}

But this is the case with the current highlight as well, so it doesn't
make that worse.

CC @mmlb

[bhcleek]

I'm 👎 on this. Builtins are not keywords and #605 laid out good rationale for the current behavior.

[arp242]

Builtins are not keywords

I never said they were, just that highlighting them always is useful as a reminder that you're overriding a keyword (just as e.g. list is highlighted in list = [] in Python, as list is a built-in global variable).

[bhcleek]

I don't view the builtins any differently than stdlb functions.

Highlighting them as functions actually serves as a useful reminder that they are just functions and can be reassigned. Highlighting them as keywords would give a user the false sense that they are immutable.

[arp242]

I don't view the builtins any differently than stdlb functions.

The difference is that you can (accidentally) override them. I can't accidentally do fmt.Println = "foo or func foo(fmt.Println string), but I can do that with new, append, etc.

they are just functions and can be reassigned. Highlighting them as keywords would give a user the false sense that they are immutable.

Sure, and arguably there is nothing wrong with overriding them, as such, but as mentioned I think reminding people "hey, remember that this is a built-in function" is a good idea.

Anyone who knows Go know they can be overwritten, so I don't think it's that much of an issue. New Go programmers may not know this, but they probably also won't know that they're overriding a global new, which will sooner or later cause much more headaches than renaming a variable.

[bhcleek]

I think you're looking at this too narrowly.

Given

type Foo struct {
    Bar() func() error
}
func (f Foo) Quux() error { return nil }

var Baz = func() error { return nil }

Baz and any Foo's Bar and Quux will be highlighted as a functions, despite that fact that Baz and any Foo's Bar could be mutated.

The builtins have the property of being mutable. keywords are not mutable. Highlighting exists partly to help users understand what they can do with an identifier. If you want to change the highlighting of the builtins, then changing their highlighting to make it clearer what can be done with them (e.g. change them to be highlighted as similarly to the other predefined identifiers), then I could get behind that. But IMO, vim-go should not signal a stronger guarantee than the language actually provides, especially if your concern is new users. @mmlb provided good rationale in #605 along with an example.

Having said all that, I do like the idea of using the syntax keyword for these, because I believe it will help performance just a bit. I think the mistake that was made with #605 is requiring a ( after the builtin name. What do you think about simply adding this patch to this PR:

diff --git a/syntax/go.vim b/syntax/go.vim
index a10f9ec..aa1b61e 100644
--- a/syntax/go.vim
+++ b/syntax/go.vim
@@ -143,12 +143,11 @@ hi def link     goFloats            Type
 hi def link     goComplexes         Type
 
 " Predefined functions and values
-syn keyword     goBuiltins                 append cap close complex copy delete imag len
-syn keyword     goBuiltins                 make new panic print println real recover
+syn keyword     goBuiltins                 append cap close complex copy delete imag len make new panic print println real recover
 syn keyword     goBoolean                  true false
 syn keyword     goPredefinedIdentifiers    nil iota
 
-hi def link     goBuiltins                 Keyword
+hi def link     goBuiltins                 Identifier
 hi def link     goBoolean                  Boolean
 hi def link     goPredefinedIdentifiers    goBoolean

That would give a different highlighting to the builtin functions, which draws the user's attention to it, but doesn't signal to the user that it's immutable. 🤔

[arp242]

For me, it seems to me that the syntax highlighter should simply highlight things based on whether it's good UX or not, and not overly concern itself with "details" in the Go language specification (of course those details are important when programming, just not when highlighting). I suppose this is our fundamental disagreement.

I really want to like your proposed solution as a middle ground, but I'm hesitant. I'm rather fond of the default minimalism of syntax/go.vim; there are only four colours: red for literals, brown for keywords, green for types, and blue for comments.
The more colours you add, the less meaning colours have (see e.g. this, this). Your solution would add a new (cyan) colour to that, which is a 25% increase.
I'm not sure if that is a good idea, especially since the cyan in the default theme stands out quite distinctly, and just makes things look more "messy" to me (I genuinely don't understand why so many people want to highlight every small detail; it just makes everything seem like a jumbled mess to me).

Either way, the issue is a minor one, and people wanting to highlight the built-in functions as brown/Keyword can still do so with a simple vimrc change, so as far as I'm concerned this PR can be closed.

[bhcleek]

Thank you for thoughtfully considering my suggested change.

One thing to keep in mind regarding your hesitation; with my suggestion, you're free to update the colors for the Identifier group in your vimrc to keep your colors to the current set :-) or to just link goBuiltins to Keyword as they are now. 🤷‍♂️ :-)

Since @fatih seemed to think #605 was a good idea, perhaps he can weigh on this PR as it stands, the slight addition to it that I suggested, and whether we should close this without merging it, merge it as it, or add my suggestion and merge it?

[arp242]

you're free to update the colors for the Identifier group in your vimrc to keep your colors to the current set

Yeah, sure; but the question is more what works well for the majority of users? I'm not sure if adding a new colour for this does (although it's hard to be sure). Last time someone did something similar it was actually reverted: #1030

[fatih]

Highlighting is one of the things that you never will make everyone happy. Hence the settings to disable all the stuff. I don't like either to much highlighting and all of the settings are disabled for me, but people, especially the younger generation love highlighting (don't ask what younger is, just an observation). I know we have already ton of settings, why not introduce another one, which is disabled by default.

Having said that:

I don't view the builtins any differently than stdlib functions.

I'm with @Carpetsmoker here. I don't see any valid reason to override it. Imho, you should never override it and I think it's one of the fault design parts of the Go. But this doesn't mean we should lead people to the dark side. That's why highlighting them is a good indicator and warning on our side, on behalf of others. Was there ever a reason people want to override it? Back then I've merged the PR, but when I think now, I think this is the correct way.

[bhcleek]

Whether we think it's good practice to reassign (even in a small scope) builtin identifiers or not, vim-go should still consider the user's experience when they deal with code that may do it.

master

Notice that new is highlighted differently depending on whether or not it's a function call.

This PR as is:

new is highlighted as a Keyword regardless of how it's used, implying to the user that it's immutable.

This PR + changing the linked group:

new is highlighted as an Identifier regardless of how it's used, drawing the user's attention to the fact that it's special somehow, but not implying that it's immutable.

[fatih]

Let's close this. After reading @bhcleek's comment it makes to keep it this way. We can later re-iterate again in the future.

[bhcleek]

From #1939

Those are my two cents . We should remove the Identifier part, keep the rest and just be done.

This is that "rest", so I'm re-opening in anticipation of merging.