Rename go_highlight_methods to go_highlight_function_calls

[arp242]

Separating method calls from calls on a package is not easy with the
current regexp based syntax highlighting. Is fmt.Println a method call
or function call?

This fixes #1500 by clarifying what the setting is supposed to do. I
think this makes sense, because people who want to highlight function
declarations probably want to do this for both functions and methods,
and people who want to highlight function calls want to do this for
functions and methods, too.

[arp242]

I also wonder if Type is the best highlight group to apply here, as it's the same as, well, types :-) Wouldn't using Function here make more sense?

[bhcleek]

I see your reasoning here, but I've always understood g:go_highlight_methods and g:go_highlight_functions to be about doing exactly what they say. I am also not convinced that users that want to highlight the names of functions also want to highlight the names of methods using the same configuration value.

Assuming that's the case, the problem identified in #1500 may simply be that:

• the syn match for goMethodCall should come before goFunctionCall.
• there should be no guards for the syn matches
• there should be guards for the hi defs of the match groups.

[arp242]

I am also not convinced that users that want to highlight the names of functions also want to highlight the names of methods using the same configuration value.

I don't know either, but this is how it has actually works right now and thus far no one complained. It just wasn't documented as such :-)

[arp242]

Any thoughts @bhcleek @fatih ? I think that with this small PR it corrects and documents how it currently works already; if we want to change/improve that we can always do that later on.

[bhcleek]

As I mentioned in #1557 (comment), I think this change is a departure from how it currently behaves, and only @fatih can resolve the conflicts in our understanding. How would you feel about trying what I suggested to simply resolve the highlighting instead of changing the meaning of the variables?

[bhcleek]

I should point out that back when g:go_highlight_methods was added, go:go_highlight_functions already existed, and it seemed at the time that the two values were intended to make the highlighting of methods and functions orthogonal, as prior to the addition of g:go:highlight_methods, g:go_highlight_functions only highlighted functions.

[fatih]

Could it be that it might be regressed while we refactored it? I always remember it was working fine. 3 years ago when I started this project, the method setting was one of the first added. It made it really easy to see methods and distinguish them from ordinary functions (as methods are grouped together). I'm all good to fix it, but we shouldn't remove or change the meaning. Functions and methods are two different things in the spec.

[bhcleek]

Thanks @fatih! I layed out a possible root cause in #1557 (comment)

[arp242]

go_highlight_methods has always highlighted method calls, and never definitions, as far as I can find. And go_highlight_functions has always highlighted both function and method declarations.

I guess the confusion started when someone added goFunctionCall inside the go_highlight_functions block several years later.

Highlighting method calls separate from function calls is impossible, since we can't disambiguate between calling a function on another package, and calling a method; not with Vim's regexp engine anyway. This feature has never worked quite right in that sense.

So; regardless of what it was in the past, which settings do we want to have? At the very minimum the documentation should be clarified. To me, it still seems to make sense to group goFunctionCall and goMethodCall with one setting; although we could also make two settings for that (highlight_method_calls and highlight_function_calls). I don't particularly care, as such.

there should be no guards for the syn matches
there should be guards for the hi defs of the match groups.

I'm not sure if I understand how this will fix the issue in PR, as such? Either way, it would not be a good idea as it will mean all those regexps will be run for everyone, even people who haven't enabled the setting. That will be a severe performance hit, especially on slightly older systems (e.g. my 2005 OpenBSD laptop still works kind of okay :-))

[fatih]

Oh I see, yeah function calls and declarations are two different things. My comment was about function and method declarations, not calls. I'm ok to have a single setting for function calls. I'm just not sure if someone would want to have two different highlights for the foo() and t.foo(). Maybe it could be useful?

[codecov-io]

Codecov Report

❗ No coverage uploaded for pull request base (master@b50d8f2). Click here to learn what that means.
The diff coverage is 0%.

@@            Coverage Diff            @@
##             master    #1557   +/-   ##
=========================================
  Coverage          ?   14.79%           
=========================================
  Files             ?       53           
  Lines             ?     4150           
  Branches          ?        0           
=========================================
  Hits              ?      614           
  Misses            ?     3536           
  Partials          ?        0

Flag	Coverage Δ
#nvim	5.06% <0%> (?)
#vim74	14.72% <0%> (?)
#vim80	14.48% <0%> (?)

Impacted Files	Coverage Δ
syntax/go.vim	0% <0%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b50d8f2...264fe46. Read the comment docs.

[arp242]

My comment was about function and method declarations, not calls.

No, nothing changes for declarations. Sorry for the confusion.

I'm just not sure if someone would want to have two different highlights for the foo() and t.foo(). Maybe it could be useful?

Ehm, probably not. goFunctionCall also highlights methods, so it already works like that. I removed goMethodCall for now.

We could also maybe just remove g:go_highlight_methods; it doesn't really do all that much right now.

[bhcleek]

Why is g:go_highlight_methods being removed? From Fatih's comment,

I'm all good to fix it, but we shouldn't remove or change the meaning. Functions and methods are two different things in the spec.

Which he confirmed was about declarations in #1557 (comment), I thought the highlighting of methods should be fixed... 🤷‍♂️

[fatih]

So to recap, currently we have the following settings:

# highlights function declarations and calls, but also
# highlights method declarations and calls 
go_highlight_functions

# highlights method calls
go_highlight_methods

This is imho broken. Because go_highlight_functions overlap with
go_highlight_methods. Also in addition, go_highlight_methods doesn't
highlight method declarations, just method calls.

Martin's PR changes the name to make it clearer but also changes the meaning of
them :

# highlights function and method declarations
go_highlight_functions

# highlights function and method calls
go_highlight_functions_call

The meaning of go_highlight_functions is changed to only show declarations,
and go_highlight_methods is removed in favor of
go_highlight_function_calls.

This is of course, at least better compared to the current situation. So the question,
I think boils down if we want to change this, as it changes the meaning of the setting
that we have for years and also removes the go_highlight_methods setting.

---

Having said that, I suggest something else. I vote to change the current intepration to this, just for now:

# highlights function declarations and calls
go_highlight_functions

# highlights method declarations and calls 
go_highlight_methods

So we're going to fix go_highlight_functions that it no more highlights
methods and fix go_highlight_methods so it also highlight method calls.

This is the first step, the second step would be then to introduce these new
four settings and remove the both go_highlight_methods and
go_highlight_functions above:

go_highlight_function_declarations
go_highlight_function_calls
go_highlight_method_declarations
go_highlight_method_calls

We could do this in two separate steps or just in one single step and be done
with with.

[bhcleek]

TL;DR:

• let's rename g:go_highlight_functions to g:go_highlight_function_declarations and then it's LGTM.
• Given that it's not possible to distinguish between method and function calls, the two suggestions in your recent comment are both non-starters, IMO.
• I'm all for doing it in a single step if we want to distinguish between function and method declarations.

I think the go_highlight_method_calls you suggest has to be combined into go_highlight_function_calls, because it's not possible to distinguish between method and function calls in the general case; a package identifier on a function call looks the same as a receiver on a method call. e.g. given log.Println("hello world"), one cannot determine whether Println is a method or a function.

That also means, of course, that your first suggestion in your recent comment to

# highlights function declarations and calls
go_highlight_functions

# highlights method declarations and calls 
go_highlight_methods

is impossible for similar reasons.

In any case, I'm mostly fine with this PR as is, but if we want to allow highlighting of function and method declarations separately, then instead of the two settings in this PR, we'd need these three settings:

# highlights function declarations
go_highlight_function_declarations
# highlights method declarations
go_highlight_method_declarations
# highlights function and method calls
go_highlight_function_calls

To prepare for that eventuality, let's rename g:go_highlight_functions to g:go_highlight_function_declarations for clarity and to set us up for adding g:go_highlight_method_declarations later if we want.

[fatih]

because it's not possible to distinguish between method and function calls in the general case; a package identifier on a function call looks the same as a receiver on a method call. e.g. given log.Println("hello world"), one cannot determine whether Println is a method or a function.

Maybe not now, but if we switch to a Go parser with type informations attached to the AST, then it would be easy to identify (because we would know if Println() is a function of the log package or whether it's a method of a type). We're not there yet and I'm not sure if it's possible but it's a possibility in the future. I know at least there is a ruby syntax highlighter that is AST based which was demonstrated in VimConf 2017 in Tokyo.

In this case I think this PR in its current state is good to go, so:

# highlights function and method declarations
go_highlight_functions

# highlights function and method calls
go_highlight_functions_call

I think we should open the door for future improvements and introduce the _declarations and _calls suffixes once we have the ability to distinguish between method and function calls. Until then I believe two settings are good enough for our needs.

[bhcleek]

SGTM