sublimehq / Packages

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

[JavaScript] [RFC] Removing `readwrite` from variable scopes. #1336

Open Thom1729 opened 6 years ago

Thom1729 commented 6 years ago

Originally, JavaScript did not support constants; all variables could be reassigned. Now, with the adoption of const, variables are not necessarily writable. Unfortunately, it is impossible to make this distinction at the time of use. In the statement console.log(myVar);, it is not possible for the syntax to determine whether myVar is a read/write variable or a read-only variable. In other words, the JavaScript syntax does not distinguish read/write variables from read-only variables.

Currently, all variables are scoped as variable.other.readwrite (except in some special cases). This used to be trivially correct, because all JavaScript variables were read/write. Now, it is often incorrect. There is absolutely no way to correctly mark a variable as read/write or readonly, because fundamentally this is not a syntactic distinction.

In order to fix this, there is no option but to strip the readwrite subscope from the common case and mark variables simply as variable.other. We could follow the example of some languages and mark variables beginning with uppercase letters as constants because of common convention, although the language does not enforce this and there may often be exceptions. We can't really mark all-lowercase variables as readwrite because it is very common to have lowercase constants.

It would be possible to mark variables as readwrite/readonly when they are declared. However, because this is really a syntactic feature of the entire declaration than of the name itself, I think it would be more appropriate to mark the entire declaration with meta.declaration.variable.readwrite or something similar. (This would also mean substantially less code.)

Thoughts, objections, alternative suggestions?

FichteFoll commented 6 years ago

From my understanding, the readwrite name is mostly a convention for user-defined variables and used in several default syntax definitions since the initial JS rewrite in early 2016. I've never been too fond of the choice, however, since imo it would be better to name it something like variable.user to reflect that more accurately. (Basically, get rid of all the "other" stuff. The mere existence of that is odd.) Additionally, "constant" variables could be called variable.constant, like function variables are called variable.function.

A similar thing was discussed while looking at scope names for annotations: https://github.com/sublimehq/Packages/issues/709#issuecomment-266835130.

FichteFoll commented 6 years ago

Updating on that, the variable scope in general is intended for more or less user-controlled names for things. Basically what variables are, mostly. Technically speaking, the "readwrite" (or "default") variable type could get a scope of just variable and then the syntax suffix. However, I strongly believe that we need to differentiate these types of variables from the others, so it needs to get at least a second token to subclass these generic variables. Maybe variable.generic?

Thom1729 commented 6 years ago

Is there any need for a variable.generic scope at all? It seems to me that in JavaScript, and probably most languages, we can't often say that a variable is user-created; all we can say is that we don't know it to be something else. Should we simply represent a lack of such knowledge with the lack of a subscope?

The downside would be the added difficulty of coloring those variables. But is that useful in the first place? A variable scoped variable.generic could easily be rescoped (and thus recolored) when the syntax was improved; it seems a fragile thing to hang a color on.

I must admit that I'm skeptical of most of the built-in uses of variable subscopes as well. In the case of JavaScript, it's very rare that we can apply them reliably, and they add an annoying amount of complexity. But I can see that they work well and are quite useful in a lot of cases, so I wouldn't suggest removing them. I don't see a similar benefit in a variable.generic subscope, at least for a language like JavaScript where variable subscopes are guesswork in the first place. But I don't suppose that a generic scope would do any harm, either.

FichteFoll commented 6 years ago

Well, personally I prefer having variables in general not use a special color, since it would become too colorful, but I do like having colors for self, parameters and similar special offenders. I render function invocations in italics.

So, in order to target these "generic" or undecided variables in a scope selector, it'd have to be more specific than just variable.python, since otherwise I'd have to exclude all other variable candidates.

For reference, this is my current color scheme:

variable.language,
variable.other \- variable.other.readwrite \- variable.other.object.js \- variable.other.member,
variable.parameter {
    @name "Variable";
    foreground: #3399CC;
}

// variable \- variable.other.readwrite,
support.variable {
    @name "Variable: Support";
    foreground: #1786BD;
}

variable.function,
variable.annotation {
    @name "Functions (usage) and annotations";
    fontStyle: italic;
}
Thom1729 commented 6 years ago

On a tangential note, I've never really been satisfied with the way that the declaration and use of a name are (not) distinguished.

Consider JavaScript (surprise). There are several ways of declaring a name:

We scope these totally differently. In a function or class declaration or expression, we use entity. In formal parameters, we use variable.parameter. Pretty much everywhere else, we use variable.other.

Once a name is declared, there's no way of knowing where it came from at the site of use. The name foo could have been declared in a let statement, or imported, or it could be a hoisted name from a class declaration. It could also be of any type. We can't know. Yet it seems that we often scope the use of a name by trying to guess what's in it or how it was declared.

The actual use cases we can practically distinguish are:

In particular, we can't say that a given name usage refers to a function parameter or anything like that. And so it seems to me that we have a set of distinctions pertaining to the declaration of a name, and another set of distinctions pertaining to the use of a name, but the former are rudely intruding upon the namespace of the latter.

For example, take variable.parameter. This is a formal declaration of a new name, like entity.name.function. But it's not scoped like a declaration, but like a usage, Or take the statements let x = 0; console.log(x);. The first x is a name declaration, but the second x is a name reference. Yet they have identical scopes.

This might seem like an irrelevant aesthetic quibble, but suppose that the first x were scoped entity.name.variable (or somesuch). It would be almost trivial to use that kind of scoping information to later find where x was declared, just like we rely on entity.name.function to find where a function was declared. When I wrote my own JavaScript syntax a while back, I used exactly that approach to implement completions that knew which variables were in scope and how/where they were declared.

I wonder if anyone would complain if I went ahead and implemented a set of declaration scopes in JavaScript. Of course, I'd have to maintain backward compatibility — a function parameter might be entity.name.variable.parameter variable.parameter for the sake of existing syntax highlighting.

This really does feel like something that should be standardized. I vaguely remember seeing a suggestion for a standard meta.declaration scope at one point. If several languages scoped name declarations consistently, it should be reasonably simple to write a lexically-aware completion plugin that worked on all of them.

FichteFoll commented 6 years ago

I'm with you on variable.parameter being used incorrectly because they are being defined there, not used. In the same fashion, we should probably change variable declarations to using some entity scope as well.

What about dynamically typed languages that don't require a variable to be declared with certain syntax? i.e. x = 1 vs let x = 1.

Thom1729 commented 6 years ago

I don't think there's anything we can do about those languages. The “declaration” is just the first usage, and the first usage is syntactically indistinguishable from any other usage. Syntactically, there's just no declaration to mark.

As far as scopes go, something like this for the names:

For entire declarations, possible including multiple names, keywords, and other stuff:

* In JavaScript, function declarations and function expressions look very similar but have subtly different syntax and semantics. Function declarations would be scoped meta.declaration.function meta.function, while function expressions would only get meta.function.

Overall, the intended meaning of meta.declaration would be ‘a construct that declares an entity for use outside the construct’. There may be cases where an entity.name is not contained within a meta.declaration. For instance, in the JavaScript function expression (function myFunc() { console.log(myFunc); }) the function name myFunc is exposed inside the function body even though the function expression is not a declaration that would expose the function to other code. The same applies more generally to function formal parameters in most languages.

borela commented 6 years ago

@Thom1729

We could follow the example of some languages and mark variables beginning with uppercase letters as constants because of common convention, although the language does not enforce this and there may often be exceptions.

+1 for uppercase convention.

When I wrote my own JavaScript syntax a while back, I used exactly that approach to implement completions that knew which variables were in scope and how/where they were declared.

Did you need to write a python plugin for that? Is the source available?

borela commented 6 years ago

@Thom1729 entity.name.variable breaks many color schemes, a solution would be having a meta.declared-variable or something simpler.

FichteFoll commented 6 years ago

Why does it break which color schemes in what ways?

borela commented 6 years ago

Lots of color schemes define the color for variable but not entity.name.variable, https://github.com/borela/naomi/issues/74 what happens in those cases is that the variable will usually assume the context color which might be the function, class, etc... and will make the code look broken.

FichteFoll commented 6 years ago

They should define a color for entity.name.

borela commented 6 years ago

I wish this was an isolated case, but lots of color schemes don't and if you change it, it'll have the same fate as changing the object keys to unquoted strings.

I am in favor of having precise scopes(like in the object keys example) but at the same time I don't want to make good color schemes look bad.

FichteFoll commented 6 years ago

I was specifically saying entity.name, which is the more general scope name and would apply to entity.name.variable too. It's also listed in the minimal coverage docs: https://www.sublimetext.com/docs/3/scope_naming.html#color_schemes

Imho any scheme that does not define a color for entity.name (or at least entity) is inherently a bad scheme, which are plenty unfortunately.

borela commented 6 years ago

The issue is that most color schemes assume entity is used for classes or functions, so you would still have issues similar to the object key and then having to revert the patch. I used entity.name.variable in the beginning, but after going through the color schemes in color sublime I realized that it made them look odd.

Thom1729 commented 6 years ago

Yeah, the scope naming guidelines specifically call this out:

Historically, many color schemes have provided one color for entity.name.function and entity.name.type, and often a different color for entity.name.tag. This leaves new entity.name.* scopes un-highlighted.

Color schemes should instead specify a color for entity.name that will be applied to classes, types, structs, interfaces and many other data structures. This color can be overridden for the two scopes entity.name.tag and entity.name.section, that are used for different types of constructs.

I'm all for supporting existing color schemes, but we can't refrain from using the standard scopes where they are appropriate just because some color schemes might ignore them. Not when they're on the list of minimal scope coverage for color schemes and when there's an entire section reminding color scheme authors to color that scope in particular.

borela commented 6 years ago

entity.name that will be applied to classes, types, structs, interfaces and many other data structures.

This made almost all color schemes assume that entity.name is used for structures similar to classes, functions, etc... anything but variables.

FichteFoll commented 6 years ago

Not all changes will be backwards compatible. I agree that this situation is unfortunate, but all the color schemes that were only highlighting functions and types were based on https://manual.macromates.com/en/language_grammars#naming_conventions. If we want to make a change and improve scope naming guidelines, we will inevitably have to make changes that cause color schemes to break in minor or major ways. We always try to stay on the minor side, but I really do not see a way to avoid introducing more entity.name scopes.

We have steadily been introducing new entity.name scopes over the past years and nobody has complained ever. Most people are using the default color schemes anyway, which are controlled by Sublime Hq and thus easily changed, so that will cover most users already. The remainder seems to either not care or use a color scheme that matches entity.name (or entity).

Regardless, we can add a compatibility layer and assign the old and the new scope to the same token, i.e. variable entity.name.variable. Things wouldn't change for old color scheme users since the old scope is still there and they didn't define anything for entity.name.variable, but new color schemes do profit fromt his granularity.

borela commented 6 years ago

We have steadily been introducing new entity.name scopes over the past years and nobody has complained

Because the change probably didn't affect the colors at all; I modified a local syntax to have a variable.other.readwrite and entity.name.variable side by side and went through Color Sublime color schemes, lots of them show a completely different color.

This happens because they usually have a rule for variable and another for entity.name, so the entity rule will always win even if the variable have both tokens.

Most people are using the default color schemes anyway

Highly disagree, what I see is the opposite, people install sublime then the first thing they do is to customize the appearance with a theme which in most cases comes with a color scheme that suits the theme.

This is in fact why people love sublime text, it's fast and you can make it look the way you want;


Just look at the object literal key thread https://github.com/sublimehq/Packages/issues/141#issuecomment-182826789 this was done before and it had to be reverted.

Users will wake up someday to find that their code is looking "broken" and then come here asking why.

mitranim commented 6 years ago

I think we should leave variables as plain text, and scope only their declarations. Or at the very least, scope declarations differently from other occurrences. Honestly, it doesn't seem useful to highlight regular occurrences of variables, as it's just about the only part of source code that is not already something fancy, like a keyword, a type name, etc.

In fact, I had to disable variable.other.readwrite highlighting in my color scheme because JS (Babel) scoped every occurrence of every variable, and the code looked like a christmas tree. For variables, regular black or white is fine. Declaration highlighting would be useful though. Let's give them distinct scopes.

keith-hall commented 6 years ago

I would agree with scoping variable declarations as variable.declaration, but it is very important to still scope variable usages, as some people will rely on it for hashed syntax highlighting.

FichteFoll commented 6 years ago

@keith-hall any specific reason for not considering entity.name.variable?

keith-hall commented 6 years ago

mainly for color scheme reasons, I imagine people would expect variables to all be colored consistently. For example, function definitions that take parameters are "declaring variables", and they get variable.parameter, and variables are declared a lot and interspersed with usages, so even if we get some kinda goto definition on them, I suspect keeping the variable top level scope would be desirable. I have a feeling I've seen variable.other.declaration scope used somewhere before...

Thom1729 commented 6 years ago

I think that using variable.parameter for function formal parameters is arguably wrong, and that entity.name.parameter would be more appropriate. Of course, it's been there long enough that I don't think there's any chance of changing it.

One compromise might be scoping declared variable names as entity.name.variable variable.whatever.