sublimehq / Packages

Syntax highlighting files shipped with Sublime Text and Sublime Merge
https://sublimetext.com
Other
2.95k stars 586 forks source link

[Go] Built in functions no longer highlighted #1795

Closed ccampbell closed 4 years ago

ccampbell commented 5 years ago

This is a regression I noticed from #1662 (running dev build 3183).

All of the built-in functions that used to be highlighted as support.function.builtin.go have been removed and are now simply using variable.function.go. I miss the highlighting because it is useful when writing to be able to tell which functions are built in.

Removed here: https://github.com/sublimehq/Packages/pull/1662/files#diff-c06e69a4dd85794f1dd2e5688c83688eL118

- - match: \b(append|cap|close|complex|copy|delete|imag|len|make|new|panic|print|println|real|recover)\b

I understand if this was a conscious decision, but I disagree with it, and it is not consistent with other langauges. For example in Python:

wbond commented 5 years ago

This is definitely a regression from my point of view.

ccampbell commented 5 years ago

This is true of all built in types used for conversions too. For example:

Now using variable.function.go. Not sure what scope they had previously. I am guessing this bit:

https://github.com/sublimehq/Packages/pull/1662/files#diff-c06e69a4dd85794f1dd2e5688c83688eL365

rwols commented 5 years ago

To play the devil's advocate, why doesn't your color scheme just highlight them the same as support.function? Seems like an easy fix.

ccampbell commented 5 years ago

@rwols because then all function calls will be colored the same way. I like the distinction between built in/language functions and user defined functions. The main thing it helps is avoiding typos.

For example if I were to type stirng() instead of string() in a type conversion call by accident, the old syntax would alert me to that immediately (it would not be highlighted – when I am expecting it to be) whereas the new one treats it the same way, and changing my theme wouldn’t help with that either.

mitranim commented 5 years ago

I insisted on treating built-ins and user-defineds equally, so my apologies for the inconvenience. Would you mind explaining your ideal experience to bring us on the same page?

Also, please allow me some counter-questions, again to bring us on the same page.

Are you sure this serves your stated desire of catching typos? Is the percentage of built-ins in your function calls that high?

Are you sure this doesn't just distract from a general solution, such as a fancy linter, or a file watcher that runs go vet, catches far more errors, and scales up to user-defined functions and types?

Pretty sure that built-in ST3 color schemes support variable.function. In your color scheme, it seems to be disabled. Is that intended? This syntax is designed for the "current" color schemes. Have you tried one that supports more scopes, such as one of the recent built-ins, or something like this?

I'm trying my best to not come across as obstinate. This is a collaborative work directly serving its users, and it's in the nature of such projects to err on the side of "more optional features" rather than "one true way of doing things". We probably end up restoring this feature. I just don't feel comfortable immediately agreeing to provide a very underwhelming and underpowered solution without suggesting a much better alternative.

mitranim commented 5 years ago

See related issue #1803.

ccampbell commented 5 years ago

Thanks for your reply.

I will admit that I probably do not have typos in built in functions all that often, but what the highlighting provides is a certain peace of mind while coding. I understand that may be an unusual experience, and part of it may also be that I am so used to it because I have been using Sublime Text (and previously TextMate) for ages now, and this was always the behavior. As soon as I updated to the new syntax I felt like something was missing/wrong. I am certainly a creature of habit (as I imagine most developers are), and considering I spend more time in my code editor than I do pretty much anywhere else, I have very specific expectations for how things should work. I have written my own Sublime Packages to fill in certain gaps.

There is something satisfying to me about the color changing as soon as I type in the name of a function that matches a built in. There is also the fact that other languages do it too. For example, in JavaScript, when I type decodeURIComponen and it is still scoped as variable and then I add the final t to the end, and it becomes support it assures me that I typed the function call correctly by immediately turning blue (or whatever color support matches in your theme).

I am aware that in Go it is not as important since it is strongly typed and a linter (or building the program) will catch it, but I still like the distinction and the immediate feedback.

As for highlighting variable.function that won’t help because then all function calls will be highlighted the same color, and what I want is a way to distinguish between “yes I have typed it correctly” and “no I have not”. I would be okay if everything was the same color as long as in the interim state (while it is incorrect), it can display a different color – more on that below.


Ideal behavior

I think ideally the highlighting would be scoped based on if the function matches a method within the package regardless of if it is builtin or not. What I mean by that is something like this:

strin(someVar)
^ variable.function.invalid.go

string(someVar)
^ variable.function.go

And also in an ideal world this would work for user defined functions within the current package:

errorRespons
^ variable.other.invalid.go

errorResponse(http.StatusInternalServerError, "Error")
^ variable.function.go

It would also work within method calls on other packages:

base64.StdEncodi
       ^ variable.other.member.invalid.go

base64.StdEncoding
       ^ variable.other.member.go

base64.StdEncoding.DecodeStri
                   ^ variable.other.member.invalid.go

base64.StdEncoding.DecodeStrin(base64String)
                   ^ variable.function.invalid.go

base64.StdEncoding.DecodeString(base64String)
                   ^ variable.function.member.go

This way you would be able to tell if you have any typos on any function call (built in or user defined) in real time. The highlighting would be “flipped” – when your code is all done and correct the scopes will match what you have now in your syntax, but as you are typing (while they are incorrect), they would be scoped differently to allow themes to pick up on this in some subtle way.

I don’t believe this kind of behavior is possible in sublime-syntax files natively though since it would require knowing the context and structure of your application. I also would have to try it out in practice to know for sure if it feels right. Since it is not possible, I think providing the behavior for built-in functions is better than applying it nowhere.

Anyway, I appreciate the work you have done here. I hope in the end we can settle on something that everyone is happy with.


EDIT: One other thing to point out is that using a custom scope for builtins still will allow you to treat them the same by changing your theme to have the scope match the color used by variable.function.go, but the way it is currently, I am not able to edit my theme to achieve the behavior that I want (the previous behavior).

mitranim commented 5 years ago

Thanks for the detailed thoughts. This explains your ideal experience and makes a lot of sense.

(Commence off-topic rant.)

You're right that context-aware scoping is beyond the capabilities of Sublime's current syntax engine. It's available in some editors/IDEs, but last time I tried them, they felt slow.

Every language has its own ideas about scopes and modules. This may be muddled by macros / code generation / preprocessor directives, with C being one of the worst offenders. This may be muddled at runtime, particularly in JS and various Lisps. I doubt the possibility of a generic framework for robust context-aware scoping.

IDEs with rich language support tend to be specialized to a few languages. Said support also tends to be ridiculously expensive to implement. Whereas one of Sublime's biggest strengths, in my opinion, is how it accommodates virtually all languages by lowering the denominator. I very much value this approach. Partially since I believe that polyglotism dramatically improves your quality as a developer, and a polyglot editor lowers the barrier to using more languages. Partially because it makes language development more democratic by lowering the barrier to implementing language support.

Some languages are designed for an IDE, and some are designed to be friendly towards editors like Sublime. This may be projection, but I wouldn't be surprised if outside of the ubiquitous JS/HTML/CSS, the Sublime demographic tends to herd towards editor-friendly languages like Go.

What I'm trying to say is that context-aware scoping might have such a tragic cost that we shouldn't even venture in that territory.

(End rant.)

mitranim commented 5 years ago

Back to the main topic.

using a custom scope for builtins still will allow you to treat them the same by changing your theme to have the scope match the color used by variable.function.go, but the way it is currently, I am not able to edit my theme to achieve the behavior that I want (the previous behavior)

Yeah, makes sense. This phenomenon happened to be referenced earlier:

it's in the nature of such projects to err on the side of "more optional features" rather than "one true way of doing things"

What really bothers me about context-free scoping of built-ins is how easily it breaks on redefinition. Consider:

func (self PubErr) Error() string {
    var out string

    append := func(str string) {
        if str != "" {
            out += ": " + str
        }
    }

    append(self.Code)
    append(self.Msg)
    append(self.Details)

    return out
}

Choosing to scope append() as a built-in function, context-free, automatically means that this code will be scoped incorrectly. This code is not contrived; I happened to write something similar a few times. Many gophers would frown at such redefinition, in part because they expect editors to mess it up. However, there's a viable way to not mess it up. Two such ways being: (1) context-awareness; (2) no special treatment. Since we have no access to (1), the next best thing is (2). Not sure if there's a way to make this feature purely optional. Simply including it, for those who want it, will sometimes produce incorrect scoping for those who don't.

See the sister issue #1803 about the built-in types. It overlaps with the current thread when it comes to casts. In Go, it's impossible to distinguish calls from casts in a context-free syntax engine. If we decide to support built-in types, how should we treat casts? Giving them a "type" scope is wildly inconsistent with non-built-in types, and one of the principles behind this syntax is the equivalent treatment of built-ins and user-defineds. Giving them some special "function" scope is weird when the syntax actually thinks they're types. I don't see an "obviously good" solution here. Suggestions are welcome.

ccampbell commented 5 years ago

I don’t have a good answer for you, but just from my own experience I find (at least the option of) having built-ins highlighted more useful than the edge case of “what happens if I redefine a built in function?”.

I don’t have data to back it up, but I would guess that in the extreme majority of cases when people are using append() in go they are using the built-in function vs. a custom one.

I don’t see the big deal of highlighting these as support.function.builtin.go or whatever they were before, and then for people who are really bothered by it, they can change their themes to make that scope highlight the same way as variable.function.go. I understand that syntactically they may not have the same meaning, but it is more about giving people the ability to edit themes and change the syntax highlighting to match what they want.

Usually I am all for taking a hard stance and saying “too bad”, but in the case of theming specifically I think it is better to err on the side of having the scopes be too specific than not specific enough.

wbond commented 5 years ago

Here is why I consider this a regression.

Sublime Text syntaxes started as the open source .tmLanguage files from TextMate. With that, we inherited a set of implied standards and expectations on what is highlighted and using what scopes. In general over the past three years we've been trying to refine that. This includes things like more specific scopes, better accuracy, and overall a richer set of highlighting. Users in general like their code to be highlighted, especially using colors that impart semantics. Very few users prefer monochromatic or minimalistic highlighting, but those that do can customize their color scheme to remove information they find distracting.

While we have fixed huge swaths of bugs in the improving the syntaxes, part of what we have been doing is making them richer. We use consistent scopes for punctuation, braces, function calls, class names, inheritance, etc. All of these changes are additive. Users can get more information from the syntax highlighting. They are more likely to spot issues, or better understand how the code will be interpreted by the language.

This change was subtractive. It does not provide more information to the user, but instead removes it. Not only that, but there isn't a good reason to remove the information. Now, in certain edge cases we have to make a trade off between correct syntax highlighting and adding support for a feature. We usually try to walk the line of trying to find what will be beneficial to the majority of users. The fact that in very few cases a user has overridden a built-in is not a good reason to ignore builtins. If that was the case, pretty much every dynamic language would have no information about builtins since they can be redefined. In fact, I use this on a regular basis to avoid using builtin names so that definition/usage lookup isn't noisy.

I appreciate all of the work you did on the Go syntax @mitranim, however this change wasn't, in my opinion, a good change for users at large. I want to make sure we get appropriate scoping of builtins back in for the next stable build at the latest, but ideally sooner for all of the dev users.

I'm working on the new Git functionality at the moment, but I will be sure to review and merge a PR for this, or take care of it myself if no one else ends up taking a swing at it.

mitranim commented 5 years ago

No problem. I'll have a go at a PR this week.

Feedback paradox: as a product developer, you only really get feedback when users are annoyed. (Otherwise they don't come chat with you.) Wringing the most information out of them, to try and understand the users more, involves keeping them in the "annoyed" state by temporarily withholding the "carrot" (the fix).

FichteFoll commented 5 years ago

from my own experience I find (at least the option of) having built-ins highlighted more useful than the edge case of “what happens if I redefine a built in function?”.

Just chiming in to +1 this sentence. Highlighting builtins in python specifically is just better considering how the language functions and that it has a global scope where these are active all the time (and that it has methods and stuff to namespace things).

In rust, for example, this wouldn't make all to much sense because there are no builtin functions in the global scope but only methods and those are impossible to distinguish from user-defined ones. Although many of the methods defined in the stdlib will not be redefined to mean something else (thanks to trait implementations), it still doesn't make a lot of sense to highlight/scope these methods any different from others, or at least I can't imagine it being advantageous.

Note that types (in statically typed languages) are affected by the same considerations. (Though that's just whaut #1803 is about.)

mitranim commented 5 years ago

Should be addressed by #1825.

wbond commented 4 years ago

This issue was previously resolved, so I am closing it