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

[JavaScript] Some minor highlighting issues #5396

Closed
advanceddeveloper opened this issue May 6, 2018 · 4 comments

Comments

@advanceddeveloper
Copy link

commented May 6, 2018

Here are a few edge cases I've found:

~{async a(){}};                  // Keyword `async` not highlighted
/a/s;                            // Flag `s` not highlighted
~class{}/a;                      // There should not be a regex
a+ ++/a/[a];                     // Regex is not highlighted
for(a of /a/);                   // Regex is not highlighted
@marijnh

This comment has been minimized.

Copy link
Member

commented May 6, 2018

The mode is a highlighter, not a serious parser, so corner cases like this which are unlikely to show up in real code don't interest me too much (feel free to submit patches, though).

Recognizing the s flag is easy enough, that's done in attached patch.

@marijnh marijnh closed this May 6, 2018
marijnh added a commit that referenced this issue May 6, 2018
Issue #5396
@advanceddeveloper

This comment has been minimized.

Copy link
Author

commented May 14, 2018

@marijnh

Corner cases like this which are unlikely to show up in real code don't interest me too much

I agree that some of these are very unlikely to appear in a real code (like anonymous class as the first operand of a division operator), but some of them are actually found in real scripts (modules from node package manager, which are considered as a real code (I think)).

So, I guess my question is why do you consider ~{async a(){}} as an "unreal" code? Forget about bitwise negation operator, what we have here is an object with asynchronous method a. It is pretty common to define a method in anonymous object and it is common to declare some methods asynchronous. Like you would declare asycnhronous methods in a class, but here we don't need to instantiate it, we just have such method of the object literal (which is useful when we use some object only once (no need to declare whole class in order to instantiate it)).

Here is my opinion about these cases:

  • ~{async a(){}} - As I explained above, it is found in a real script
  • /a/s - Thanks for the fix
  • ~class{}/a - I agree this one is probably not worth fixing, since it is useless unless someone modify Function.prototype[Symbol.toPrimitive], which is considered as a bad practice in general, so I think this edge case can be considered as a code which doesn't appear in real scripts
  • a+ ++/a/[a] - I don't see why you don't consider this as a real code. Funstions and regex objects (unlike numbers, strings and booleans) may contain arbitrary number of properties. Therefore, storing (and uptading) a property in a regex object is something common. Here is an example how it can be used:
Example (click to expand)
'use strict';

var prop = 'count';
var regex = /a{2}/;
var string = 'a'.repeat(5);

regex[prop] = 0;

do{
  var match = string.match(regex);
  if(match === null) break;
  console.log('Count: ' + ++regex[prop]);
  string = string.substring(match.index + match[0].length);
}while(1);

  • for(a of /a/) - Similar to the third example, I think this one may be considered as a bad code. It is useless unless someone defines RegExp.prototype[Symbol.iterator], which is again bad practice, so I think this may be considered as wontfix.

To summarize, here are my thoughts:

  • Example 1: worth fixing
  • Example 2: fixed
  • Example 3: not worth fixing
  • Example 4: debatable
  • Example 5: not worth fixing

Example 4 is actually debatable, since the syntax breaks only with regex literal, not when identifier is there (which has a reference to a regex), but modifying (incrementing) properties of regex literal doesn't make sense, so I think this one may be considered not worth fixing, but I'm not sure.

I would like to submit pull requests for these examples, but I would like to hear from you which of these are worth fixing (according to all the explanations and example use cases I posted above), because I don't think it is worth spending time on fixing something which will never be used in real code.

Also, don't let single-char variable names confuse you, if I posted it as a larger code with larger variable names and proper indent levels, maybe it would affect the way someone judges the code. These example are reduced as much as possible, but it doesn't mean they would appear in that exact form in a real code.

My idea is me to submit fixes for example 2 and 4 (if it is worth fixing), but I would like firstly to hear what you think about it.


What is the definition of a "real" code

I found a lot more JavaScript syntax highlighting issues, so I would like to know what is the criterion for distinguishing between "real" and "unreal" code. Therefore, I would suggest the following definition:

A code is considered as a "real" code (and thus if the highlighting is broken, it should be fixed) if all the following conditions are satisfied:

  1. It is a valid JavaScript syntax according to the latest official EcmaScript standard specification
  2. The code itself is deterministic and produces the exact same output independedtly of the environment it runs in
  3. The code doesn't throw SynaxErrors or any other type of throwable values
  4. The code uses the core part of the syntax provided to exploit JavaScript capabilities in order to store, modify or obtain non-trivial properties of some native JavaScript objects
  5. It doesn't modify (redefine, override, delete, or add) properties or functionalities to JavaScript native prototypes
  6. The code can be found in at least one widely used JavaScript source file (debatable, see below)
  7. The code works properly in strict mode

According to these requirements, the examples I reported satisfy:

  • ~{async a(){}} - Satisfies all the conditions
  • /a/s - Same, but it is already fixed
  • ~class{}/a - Doesn't satisfy conditions 5 and 6
  • a+ ++/a/[a] - Doesn't satisfy 2 and 6 (violates 2 because it needs to be run in exatly the same form (regex should not be stored to a variable) in order to produce useful results, otherwise violates 4)
  • for(a of /a/) - Doesn't satisfy 5 and 6

Condition 6 is indeed debatable, since the fact that some syntax structure doesn't appear in a widely used script (the definition of "widely used" is debatable itself) doesn't mean it would not be used in a near future.

My question is what do you think about these reuirements. Do you follow them when you determine which script is worth and which isn't worth fixing, or do you have some other rules to apply here? I would like to work on improving CodeMirror's JavaScript mode script in order to fix a lot of new highlighting issues (which are also brought by new ES standard), but I'm not sure which ones are worth and which aren't worth fixing.

Thanks.

@advanceddeveloper

This comment has been minimized.

Copy link
Author

commented May 16, 2018

@marijnh

Any thoughts about that?

@marijnh

This comment has been minimized.

Copy link
Member

commented May 22, 2018

Any thoughts about that?

Feel free to submit pull requests fixing the failures that bother you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.