Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(csharp) add support for @identifier style identifiers #1843

Closed
ygra opened this issue Sep 25, 2018 · 11 comments · Fixed by #2414
Closed

(csharp) add support for @identifier style identifiers #1843

ygra opened this issue Sep 25, 2018 · 11 comments · Fixed by #2414
Labels
bug enhancement An enhancement or new feature good first issue Should be easier for first time contributors help welcome Could use help from community language

Comments

@ygra
Copy link

ygra commented Sep 25, 2018

E.g.

var group = null;

group is highlighted as if it were a keyword, even though it's an identifier. This probably extends to all other contextual keywords as well, like var, where, from, select, etc.

Sample:
https://jsfiddle.net/gzqdomnj/1/

@joshgoebel
Copy link
Member

Please provide a fuller example (a jsfiddle would be preferable). Are you even specifying cs as the language or is this a problem with the auto detection and SQL?

Quick template:
https://jsfiddle.net/ajoshguy/nagkqytv/

@ygra
Copy link
Author

ygra commented Oct 7, 2019

Taking your template, it'd look as follows: https://jsfiddle.net/gzqdomnj/1/

All three variables should have the same highlighting as identifiers, not keywords. This basically is a problem that there's no real parsing going on and contextual keywords are lumped in with normal keywords. Not sure whether there's a way around this, though, given how hljs operates.

@joshgoebel
Copy link
Member

joshgoebel commented Oct 7, 2019

Not sure whether there's a way around this, though, given how hljs operates.

Well, with hljs you have to attack problems one thing at a time, try to make the part of the grammar you're having issues with more specific. Like in this case we'd consider trying to teach it what a variable declaration looks like. Then when it saw one it wouldn't be thinking about keywords.

You could probably teach it keywords don't start with @...

But that wouldn't necessarily prevent your variables from getting highlighted later in the codebase as keywords again.

@joshgoebel joshgoebel added bug help welcome Could use help from community labels Oct 7, 2019
@joshgoebel
Copy link
Member

joshgoebel commented Oct 7, 2019

Can you so easily name variables after reserved words in Csharp? I think that's why this isn't a bigger problem because in most languages you can only use keywords as keywords. If doable, is that common?

@ygra
Copy link
Author

ygra commented Oct 7, 2019

C# has two kinds of keywords: "normal" keywords, which is basically the set of keywords C# 1 started out with. Everything added after that are contextual keywords, which behave like keywords in their respective contexts, but can still be used as identifiers (they're not reserved). This has the benefit that all pre-existing code continues to work. var for example, is only a keyword when used as a type name in a declaration. That's the only place it's a keyword and thus can safely be used as an identifier (yes, even for types, in which case var loses its special meaning; arguably this doesn't really happen in code).

The following code is legal, although I'd say no one would write that:

public class var {
  public void M() {
    /* type name */ var /* identifier */ var = new var();
  }
}

It can still be somewhat common for a limited set of contextual keywords that make for good variable names. Things like group, from, set.

Additionally, prepending @ can make any keyword into a valid identifier, e.g. @class can be used as a type or variable name. This is more common in code generation, as there are usually better names for things (and C# doesn't have Java's Class clazz problem, since type is not reserved).

I guess the last part should be doable easily, trying to figure out all contexts where an identifier can appear to solve the other problem might be tricky without actually doing a lot of parsing already (and there's still ambiguities, of course, e.g. when naming a type var, or a method nameof).

@ygra
Copy link
Author

ygra commented Oct 7, 2019

To add to the last point, variable declarations are not the only place that can have identifiers. We have

  • type names
  • member declarations
  • variable declarations
  • method parameters
  • lambda expression parameters
  • generic type parameters
  • variables in LINQ queries
  • named arguments
  • named attribute arguments
  • ...

That'd be a lot of syntax, of which most of it is different.

@joshgoebel
Copy link
Member

I think the best to hope for here is to fix common cases:

@group // @[identifier] rule could eat this, preventing it from being a keyword
var group ... // var [identifier] rule eat this, preventing it from being a keyword

This would probably allow to INVALID things such as var reserverd_word, but for our purposes highlighting I'm not sure that's so bad. So if someone wants to work on a fix for this I'd try to focus on the two above cases - unless you're just brilliant - then by all means try to tackle the larger problem. :-)

Does C# haver other common things in the same vein like JS? const, let, var... if we fix var it'd be easier to fix those also I imagine.

@joshgoebel joshgoebel changed the title C# contextual keywords are highlighted wrong (csharp) contextual keywords are highlighted wrong Oct 13, 2019
@joshgoebel joshgoebel added the good first issue Should be easier for first time contributors label Dec 5, 2019
@joshgoebel
Copy link
Member

joshgoebel commented Feb 1, 2020

@ygra Could you contribute a small snippet of real sample code with a few different uses of @[keyword] as variables/identifiers? I think focusing on fixing that since it's an easy pattern would be a good start and resolve this issue for now.

@ygra
Copy link
Author

ygra commented Feb 2, 2020

I'm not sure what sample code you need here. Any token consisting of valid identifier characters starting with @ is an identifier. I'm not sure there are any places in legal code where that wouldn't be one. So all of

@foo // not strictly needed, as it's not a keyword
@class // needed, as it's a keyword
@interface // same
@var // not strictly needed, as it's a contextual keyword
@let // same

are identifiers and can only occur in places where an identifier can occur, e.g.

var @class = new MyClass();
Console.WriteLine("{0}, {1}", @var, @foo)
...

@joshgoebel
Copy link
Member

Perfect. So if we just add an exception for @identifier so they are never marked as keywords, that solves that problem, yes?

@ygra
Copy link
Author

ygra commented Feb 2, 2020

I think it should, indeed.

@joshgoebel joshgoebel changed the title (csharp) contextual keywords are highlighted wrong (csharp) add support for @identifier style identifiers Feb 6, 2020
@joshgoebel joshgoebel added the enhancement An enhancement or new feature label Feb 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug enhancement An enhancement or new feature good first issue Should be easier for first time contributors help welcome Could use help from community language
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants