Some scope errors after mathworks/MATLAB-Language-grammar transition

[elpollodiablo84]

I've found some issues after the recent migration to the different base. I'll list them all here for practical reasons, but let me know if it's better to create separate issues.

1. Problems with the transpose operator:

   1. A = B': the variable B is also under the keyword.operator.transpose scope.
   2. A = {B, C}': the operator has no scope at all.

2. Inconsistency with double percentage comments:

   1. a = 1; % Very interesting comment %%: in this line %% is considered comment.line.percentage.matlab > entity.name.section.breakpoint.
   2. %% Simple double percentage comment: in this line is comment.line.double-percentage.matlab > punctuation.definition.comment.matlab.

3. Problem with string.interpolated and inline code: in the line clear someVars; x = 1; the instruction x = 1 is still considered a part of the string.

4. I think fprintf should be a keyword.other like sprintf, instead it's considered a storage.type.control.

5. In general there are duplicate entries in various support.XXXX scopes, take a look at fprintf or fopen for example. This is not a real problem, but since the scope is unique and chosen by some kind of priority I think it's useless.

6. There is another nuisance in support.XXXX scopes: they "reserve" a lot of common/simple names, like test or ss. I know that assigning such names to variables is not a good practice, but since a lot of them refer to optional Matlab toolboxes maybe they can be ignored.

(Just to state the obvious, I'm referring to the Matlab part of the syntax)

[rlivings39]

I submitted https://github.com/mathworks/MATLAB-Language-grammar/issues/7 for (3). Feel free to open other issues against that repo. That repo provides highlighting for the MATLAB language but doesn't provide any function highlighting.

The function highlighting discussed in (4)-(6) comes from functions-reference.cson this repo.

1. This definitely seems inconsistent. I'd suggest opening an issue on the MathWorks grammar and including a description of what you'd want/expect with this highlighting.

2. The MATLAB grammar seems to only treat %% specially when the line begins with it. This is likely due to the MATLAB section comment syntax where sections begin with %%. So the %% in the middle of the first line is just part of the larger single % comment. What problem does this cause for you?

   I also don't follow what the > entity.name.section.breakpoint means in (2)i. Looking at the scopes in Atom I just see source.matlab; meta.function.matlab; comment.line.percentage.matlab. Can you elaborate? My Atom knowledge is not super deep so any hints would be appreciated.

(4)-(5) seem simple to fix. Do you want to open a PR here to change them? Otherwise I can try to take a look at doing this in a bit.

(6) Is definitely understandable. We're currently using all of the functions from the original TextMate bundle. Any thoughts on how we'd decide what would stay and what should go?

[thedavidprice]

Hi gents, First off, thanks for working on this! But know I have very little bandwidth for the next several weeks. Will keep watching this for PRs or anything I can quickly move along but won’t be able to chip in otherwise for awhile.

@rlivings39 Saw the issues you opened in mathworks repo. Thanks! Just keep me posted as needed.

@elpollodiablo84 +1 to any PRs if you have timer regarding 4 and 5 above. Would be a great help.

(6) definitely open to pruning and ways forward here. But also not sure how to evaluate (one idea, are there any stats about toolbox install/use? If so, maybe could check to see which ones are no longer relevant.) Are there any specific names that seem to be annoyingly in the way?

[elpollodiablo84]

OK, here's my counter-feedback:

1. I'll open the issue as soon as possible.
2. That's on me: the scope entity.name.section.breakpoint was defined by another package (Hydrogen), sorry! 😅
3. Nice!

Regarding points 4. and 5.: it seems it's an easy fix, but maybe it isn't. The problem is which entry of a duplicate function/keyword to keep and which to delete: for example, someone could argue to keep the support.xxx scope of fprintf and change sprintf from a keyword.xxx to a support.xxx. I think it's worth deciding on a common strategy to deal with this issue. But in this regard we have point 6.: pruning is an answer for sure, but at this point.... why keep all this functions in the syntax? I was already thinking of deleting them from my package: they give no particular "feedback" to the user since, I think, the majority (totality?) of Atom syntax themes don't decorate support.xxx scopes. Moreover, even if the theme decorates the word, it could cause some misunderstanding, since the user may have defined a function with the same name as a supported one (again, bad practice, but think of some obscure toolbox reserving names).

What are your thoughts about this?

[rlivings39]

Questions like those you raised are among the reasons that function highlighting is omitted from MATLAB-Language-grammar. I can say that when vscode-matlab switched to using the MathWorks grammar we didn't initially support function highlighting. This was objected to: https://github.com/Gimly/vscode-matlab/issues/76 and we wound up adding them back in.

Here's a proposal we could debate:

1. Only keep functions that are in base MATLAB to limit the number of highlighted symbols.
2. Move them to something other than keyword.xxx since they're not really keywords in MATLAB. Shadowing many of them can cause issues but isn't disallowed.

[thedavidprice]

@rlivings39 Were you suggesting 1 and 2 as different approaches or a two-step approach? I interpreted as the former but might not understand.

Adding to the above with a brainstorm suggestion:

1. +1 to keeping only base MATLAB and pruning rest
2. For those we prune, could move to a reference file with instruction about adding back if desired (and/or adding additional for that matter)

Pros: continuity (if people want it) Cons: complexity (maybe)

[rlivings39]

I was suggesting a two-step approach. First only keep base MATLAB functions. Then change those from keyword.xxx to something else since they're not really language keywords.

Moving the others to a reference file sounds very useful.

How common is it for Atom users to add styling to non-standard scopes? If it is we could also move the non-base MATLAB functions to an esoteric scope that motivated users could style as they wish.

[thedavidprice]

Ah, now understood about the 2-steps. And rationale makes sense to me for changing keyword.xxx.

My guess is anecdotal, but feels like 1% or less of users customize. And those that really want to customize probably create a fork. Currently I think it’s non-trivial for new package users to add styling/scopes. So if there’s a simple way we could handle all this and add to readme, I think that’s a win. (But this is also coming from the guy who’s not able to put in the work right now :) ). Most likely falls into a category of cool feature that very small minority would use...

[thedavidprice]

Quick update: Just released v1.0.2 thanks to @rlivings39 and the work upstream: #20

Neither bug fix seems to address issues in this thread. Any thoughts about moving these topics forward?

[rlivings39]

Doing your (1) and (2) above, namely pruning to only base MATLAB functions and preserving the others should be straightforward if we want to go down that route.

I've been doing some thinking about the semicolon handling for the original (3) https://github.com/mathworks/MATLAB-Language-grammar/issues/7. That will require some more thought as the highlighting rule for it is currently a beast.

The original (1) with transpose needs some investigation as to the cause as well. Will take a look when I/we get time. Any help investigating the root cause of that one would be appreciated.

[rlivings39]

I did some poking around other languages in atom and they seem to highlight all calls as entity.name.function. What if we prune out the non-base functions, and then remap all of the other functions to entity.name.function? That would be somewhat parallel to other languages.

One difference between my proposal and other languages is that the other languages, c++, js, ts, categorize all calls as entity.name.function. Given the ambiguity of parens in MATLAB syntax, f(in) could either be a call or indexing, I'm hesitant to go that far.

[thedavidprice]

+1 to "prune out the non-base functions, and then remap all of the other functions to entity.name.function"

Agreed about not extending to all calls per your reasoning above.

Barely keeping my head above water with other projects right now. So unfortunately not available to work on this. No pressure or expectations to advance things. But I would welcome it if you'd like.

[ghost]

Please don't kill support.function! It's useful to look at the function call's color and know if it's a toolbox function with a help page. I've used all the toolboxes mentioned as crucial/useful here

[ghost]

@elpollodiablo84 how do you feel about the v1.1.0 release? Does that address most or all of your concerns?

[elpollodiablo84]

Sadly all my concerns are still in place... There are a lot of inconsistencies, many more than those listed at the beginning (I'm speaking about syntax highlighting errors). I have an example file to debug/show these problems, if anybody wants to fix them. There is something that bothers me though: if you check the Github syntax highlighting of my test file, you'll see that all these inconsistencies are fixed. Was't this package supposed to be a direct "copy" of the Github language? Am I missing something? This is what I get in Matlab vs this package (version 1.1.0):

[ghost]

If you want identical highlighting to MATLAB, you'll need to customise the highlighting to reverse the IDE specific enhancements. The current highlighting is actually consistent to conventions in other languages AFAIK

This boils down to MATLAB's syntax highlighter being much more simple than the Atom one

[elpollodiablo84]

If you want identical highlighting to MATLAB, you'll need to customise the highlighting to reverse the IDE specific enhancements.

That's true, in fact in my package I tried to replicate all these enhancements, partially succeeding. It's just that I thought that, using the Github highlighting, these inconsistencies would have been fixed, but surely I've misunderstood something.

The current highlighting is actually consistent to conventions in other languages AFAIK

That's probably true, but some "errors" should be avoidable, like the transpose one. I'm still not convinced that something doesn't work on my part, since the Github Language parses nicely all this corner cases, but again 🤷‍♂️

This boils down to MATLAB's syntax highlighter being much more simple than the Atom one

Personally I don't think is more simple, since MATLAB has a lot of strange constructs and legacy code styles to be managed. The catch is that it's (almost) the only language that the editor supports.

[ghost]

That's weird, your package is a similar grammar to my own:

[elpollodiablo84]

I honestly don't know what's happening, I've just tried to reinstall this package disabling all other syntax managers, and I get my previous (messy) result... I'm on version 1.1.0, the latest uploaded to apm. I will try with a fresh Atom install whenever I can. Your result it's a lot better by the way. However there are still some inconsistencies that should be addressed, as always in my opinion. Some (all?) off them:

• Line 9-10: escaping correctly "" and ''
• Line 11: highlight system commands
• Double percentage comments are not comments

I hope that the new incoming version will work as it should be on my machine, so I can help adjusting this issues if needed.