@Alhadis is there something I'm missing with my review comments?

Ok, I made the suggested change except for changing the end rule to remove the \\s*. This causes the space after the type to also be marked as entity.name.type.instance.jsdoc in two of the test cases but otherwise does not seem to effect things. Removing the space matching breaks the tests completely.

I would prefer the solution listed here if possible, though this PR would still contain some nice scope fixes :).

@Alhadis Is there something I'm missing with my review comments?

Nope, you've got it right.

@mjbvz We really, really, really shouldn't remove the existing string and comment scopes for fairly obvious highlighting reasons. Imagine how jarring it'd be for an author to see JSDocs coloured like ordinary source code.

Example:

/** "Doc-ish" comment line */
function(){

}

@Alhadis Not sure I understand the part about removing scopes. The intent of this change is to add an additional meta.embedded scope to the type part of doc comments. VS Code looks at theses scope when deciding if editor features such as automatic suggestions should be enabled at the current position. Inside normal comments, they should not be. Inside comment content like types that is marked meta.embedded, we do want auto suggestions

Yes, I understand. The problem is that you're assigning these scopes with equal depth: E.g., meta.embedded.line.js.jsdoc.type.

TextMate's docs are quite elaborate on how the meta-type scopes are to be used:

The meta scope is generally used to markup larger parts of the document. For example the entire line which declares a function would be meta.function and the subsets would be storage.type, entity.name.function, variable.parameter etc, and only the latter would be styled.

So the author's example can be visualised as:

meta.embedded
    +
    |
    +--- entity.name.parameter

Whereas what you're doing is essentially this:

meta.embedded.entity.name.parameter

The key difference is in scope priority. Remember, the same definition of "scope" is used for two entirely seperate affairs: conveying semantic information, and syntax highlighting. You're concerned with the former, but the proposed changes are messing with the latter.

What was that Edsger Dijkstra said about separation of concerns, again...? ;)

@50Wliu I better take care of this. Both parties can be satisfied, but it takes a "text mate" to do it. 😜 I need to finish saving the trees first; tree-view in particular, if you catch my gist.

I should also stress this grammar has violated TextMate scope-semantics dozens of times already, and the scopes chosen for JSDoc highlighting were sadly some of the worst. Unfortunately, what we have is what themes are using for highlighting (as well as GitHub), so we're kinda stuck with this for a while.

This also includes themes which mistakenly highlight the meta scope. 😠

Any updates on this? #541 does not address the same thing as this PR

On my list to re-review.

Sorry for the delay! Looking into this now.

I am with @Alhadis that adding the meta.embedded scope doesn't feel entirely right since we are not embedding the entire JavaScript language. Is it not possible for VSCode to whitelist the entity.name.type.instance scope instead?

@50Wliu We can use any meta.embedded.*, such as meta.embedded.jsdoctype. The type is an embedded language in my understanding. How does that sound?

@mjbvz It's a mistake to treat this as any sort or language, and it's only for the sake of maintainability that JSDoc's patterns have even been separated. Keep in mind this grammar has been stuck with incorrect scope choices for a very long time tgab

I'm sorry, I pocketed my mobile while typing this, and my phone must have gotten impatient waiting for me to finish... 😓

*Keep in mind that this grammar has been stuck with incorrect scope choices for a very long time, and it's only recently that we've been reaching some sort of consistent and logical structure with relation to our scope choices. Glueing scopes to overcome a technical limitation (not to mention a rather arbitrary one) feels like a step backwards.

I'll admit this grammar doesn't have the cleanest history when it comes to structure or semantics, but it's come a long way.

@Alhadis I don't understand why this would be backwards step for the grammar though. In JSDocs, the type is a distinct part of the comment and it uses its own domain specific language. This proposed change seems like a great use case for the meta.embedded scope and I don't think the fact that is driven by our tooling needs makes the proposal less valid

We added a way to explicitly override the token kind (string vs comment vs language) per scope in VS Code: microsoft/vscode@d74145e

This accomplishes the same thing as using meta.embedded so I'll close this PR