highlight predefined identifiers

[bhcleek]

Match builtins as keywords and highlight with the Identifier group.

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 thing 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.

If users want the previous (or different) behavior, they are free to
link the groups to different highlighting groups in their configs.

edit: removed the description of a commit that was removed from the PR.

[bhcleek]

This applies the changes from #1782 with the modifications that I suggested there and extends it just a bit for the predeclared identifiers that vim-go already had groups for, but which weren't being highlighted.

edit: strike through the portion that's no longer relevant.

[arp242]

The goBoolean and goPredefinedIdentifer groups were the only groups that contained any of the predeclared identifiers that were not highlighted out-of-the-box with vim-go.

They are? Both link back to Boolean, which is red in the default colour scheme.

This has been discussed extensively in #1782, as well as in Slack, and a similar change was reverted in #1030. I think your proposal is poor UX. Highlighting e.g. append the same as true makes no sense. Adding more colours is more subjective, but I see no reason to do that, either; especially since it's a bright colour in the default colour scheme which stands out a lot.

When it comes to syntax highlighting no one cares what the spec says, for all practical purposes true, false, nil, and iota are special. That you can do true := "x" is correct, but also stupid. Literally no one does that. We should do what is helpful UX, not attempt to parse Go code according to the spec.

[bhcleek]

They are? Both link back to Boolean, which is red in the default colour scheme.

Perhaps it's bright; iTerm2 has has some issues with its solarized color scheme, so that's probably just an issue on my machine. I'll revert that piece.

[bhcleek]

I force pushed to remove that second commit.

[fatih]

I checked Identifier and it means:

*Identifier	any variable name

If we take a step back and think about this, is new, append, etc.. really different than some of the keywords we have, such as defer? Those are not any identifier at all,those are special functions, built into the language itself.

All these builtin functions are indeed special and I think we should treat them that way. It's stupid overriding the true name or using a function called new(), so I agree with Martin here.

I really don't want this topic to be derailed like this, however, I always aimed to make things better for the user. Some of the examples top of my mind which are different:

• :GoBuild doesn't run plain go build, it was running go build -i to make it faster (we removed after caching was introduced in 1.11). It wasn't also producing any binary (still doesn't), it's discarding it because I never wanted the user to deploy a binary created from their editor. The binary should always be created externally, such as CI systems
• We set a scope for guru, so it works at least decent. Again not something we have to do, bu we do it for better UX.
• We try to find the optimal GOPATH based on the current directory, just to make sure the editor works fine. Even if the user didn't have set GOPATH at all. I know this is going to be obsolote in the future no that 1.11 is released, but it's still there.

There are many such things in the code base. So yes, sometimes we need to aim for correctness, but not in expense of a worse UX. Having said that, I think it's ok to remove the ( ) to make it clear these names are special, but I think it's not ok to change the syntax type to Identifier as it would change the syntax highlighting of everyone dramatically in next release.

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

[bhcleek]

Closed since I'm travelling today. Martin's previous PR be reopened and merged to get the desired result.

[fatih]

Would like to see a FAQ entry describing how to change the syntax highlighting so the builtins are highlighted as keywords.

[fatih]

lgtm