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

Identify JavaScript function parameters #1446

Merged
merged 7 commits into from
Dec 3, 2018
Merged

Conversation

RexSkz
Copy link
Contributor

@RexSkz RexSkz commented Jun 19, 2018

Support parameters for these types of functions:

// es6 class method
foo(x, y) {}
// es6 arrow function
(x, y) => x
x => x
// es5 function
function foo(x, y) {}
// es5 anonymous function
function (x, y) {}

@@ -14,9 +14,25 @@ Prism.languages.insertBefore('javascript', 'keyword', {
},
// This must be declared before keyword because we use "function" inside the look-forward
'function-variable': {
pattern: /[_$a-z\xA0-\uFFFF][$\w\xA0-\uFFFF]*(?=\s*=\s*(?:function\b|(?:\([^()]*\)|[_$a-z\xA0-\uFFFF][$\w\xA0-\uFFFF]*)\s*=>))/i,
pattern: /[_$a-z\xA0-\uFFFF][$\w\xA0-\uFFFF]*(?=\s*=\s*(?:async\s*)?(?:function\b|(?:\([^()]*\)|[_$a-z\xA0-\uFFFF][$\w\xA0-\uFFFF]*)\s*=>))/i,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be async\s+ ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we use async\s+, we couldn't match the variable foo in foo = async(a, b) =>, where the async here is a reserve word instead of the function name.

I found a better solution async(?:(?:\s*(?=\())|\s+).

alias: 'function'
},
'es5-function-parameter': {
pattern: /(function\s*)(?:[_$a-z\xA0-\uFFFF][$\w\xA0-\uFFFF]*)?\s*(\([^()]*\))/,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will also match functionAbc(). Maybe something like this is more suitable:

/(function)(?:\s+[_$a-z\xA0-\uFFFF][$\w\xA0-\uFFFF]*)?\s*(\([^()]*\))/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, I'll fix my code. Thanks~

@RunDevelopment
Copy link
Member

First of all, I like this RP.
We could use this to fix the false positives of keywords in parameter names (like in the API docs). You don't currently do that, but we could.

Now for the execution:

  1. I don't get why you don't just put everything you don't want (function name) in lookarounds?
  2. We don't you just make one 'parameter': [ /* patterns */ ]? This will save you from using this many aliases.

Apart from that, I like it.
(Even though this might not equate to much because I'm not a maintainer or anything...)

@RexSkz
Copy link
Contributor Author

RexSkz commented Jul 12, 2018

@RunDevelopment

  1. Do you mean those reserve words in the regex for es6-class-method-parameter? I thought in a normal JS code, beside function names, only those words will appear before a (. If someone thinks it hard to find all possible words, I will put all JS reserve words in it.
  2. I didn't realize I can use an array here, will fix my code soon.

RexSkz added a commit to RexSkz/prism that referenced this pull request Jul 12, 2018
@RunDevelopment
Copy link
Member

@RexSkz

Because we need to identify function name too, function name is also wrapped in parameters. This is not the best idea, but I can't find another way.

I meant that despite the name parameter, you not only match the parameters, but also the function name itself. This is somewhat confusing, so I wanted to suggest, that the function name (and trailing spaces) should be included into a lookbehind.
I don't know if this approach given us trouble identifying the function name itself, so maybe I'm overlooking something?

@RexSkz
Copy link
Contributor Author

RexSkz commented Jul 12, 2018

@RunDevelopment

I just found another way to exclude "function name" from "parameters": exclude () from "parameters" at the same time so Prism can keep using ( to recognize "function".

A new commit was just created.

@@ -19,16 +19,22 @@ Prism.languages.insertBefore('javascript', 'keyword', {
},
'parameter': [
{
pattern: /(function)(?:\s+[_$a-z\xA0-\uFFFF][$\w\xA0-\uFFFF]*)?\s*(\([^()]*\))/,
pattern: /(function(?:\s+[_$a-z\xA0-\uFFFF][$\w\xA0-\uFFFF]*)?\s*\(\s*)\S[^()]*(?=\s*\))/,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 Problems here:

  1. You probably don't want trailing spaces, so please make it [^()]*?. Otherwise (?=\s* won't match anything.
  2. This also matches function a()). Not valid JS, I know, but still. Replace \S with [^\s()] and your good to go.

inside: Prism.languages.javascript
},
{
pattern: /\b(?!await|delete|export|for|if|new|return|switch|throw|typeof|while|yield)(?:[_$a-z\xA0-\uFFFF][$\w\xA0-\uFFFF]*\s*)(\([^()]*\))(?=\s*\{)/,
pattern: /(\()[^()]+(?=\)\s*=>)/,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leading and trailing spaces might not be wanted.

inside: Prism.languages.javascript
},
{
pattern: /(\b(?!await|delete|export|for|if|new|return|switch|throw|typeof|while|yield)(?:[_$a-z\xA0-\uFFFF][$\w\xA0-\uFFFF]*\s*)\(\s*)\S[^()]*(?=\s*\)\s*\{)/,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. There might be problems with \S again.
  2. Spaces?
  3. awaitSomething(a){} is not matched. Neither is $await(a){} (\b might not have been the best choice).

@RexSkz
Copy link
Contributor Author

RexSkz commented Jul 12, 2018

@RunDevelopment

All fixed. Is there any other problems?

@RunDevelopment
Copy link
Member

@RexSkz

Not problems, just the suggestion to replace (?:async(?:(?:\s*(?=\())|\s+))?(function with (?:async\s*)?(?:\bfunction. It's less verbose and easier to understand imo.

At last, a question:
One of your expressions uses a bunch of keywords to avoid matching stuff like if(abc){}. Why did you use this specific subset of JS keywords?

/((?:\b|\s|^)(?!(?:await|delete|export|for|if|new|return|switch|throw|typeof|while|yield)(?![$\w\xA0-\uFFFF]))(?:[_$a-z\xA0-\uFFFF][$\w\xA0-\uFFFF]*\s*)\(\s*)[^\s()][^()]*?(?=\s*\)\s*\{)/

@RexSkz
Copy link
Contributor Author

RexSkz commented Jul 12, 2018

@RunDevelopment

  1. Your suggestion seems good, I'll replace my regex.
  2. In normal JS code, beside function names, only those words will appear before a (. Maybe putting all JS keywords here is a better idea? But it will make this regex much longer.

@mAAdhaTTah
Copy link
Member

@RunDevelopment Besides the conflict, is this PR ready to go?

@RunDevelopment
Copy link
Member

@mAAdhaTTah Yep, I think this is ready to go.


Maybe it's time for JS-Extras? (Especially when considering #1468.)

@mAAdhaTTah mAAdhaTTah merged commit 0cc8c56 into PrismJS:master Dec 3, 2018
@mAAdhaTTah
Copy link
Member

@RunDevelopment I'm OK with this PR in the main JS language; maybe that PR is extensive enough to break it out into a separate language?

@RunDevelopment
Copy link
Member

maybe that PR is extensive enough to break it out into a separate language?

@mAAdhaTTah Do you mean the Regex one? If so, then I agree but I think we need to discuss how that language should look like because the JS regex syntax is a very stripped down version of the syntax other languages (e.g. Java, .Net) use. (And they are not necessarly compatible with each other...)

@atomiks
Copy link

atomiks commented Feb 11, 2019

This doesn't appear to work with destructuring:

function Example({ prop }) {}

@RexSkz
Copy link
Contributor Author

RexSkz commented Feb 11, 2019

@atomantic Yes, it's my fault. :-/

Prism now does work with destructuring, but I forgot to add i flag to regex, so it fails when your function name (which I use [_$a-z\xA0-\uFFFF][$\w\xA0-\uFFFF]* to recognize) starts with an uppercase letter. Will make a new PR to fix it.

Update: will be fixed in #1722

schalkneethling pushed a commit to mdn/kuma that referenced this pull request Mar 27, 2019
Resolves [bug 1538693](https://bugzilla.mozilla.org/show_bug.cgi?id=1538693).

<!--
# ------------------------ >8 ------------------------
Remove everything below this line from the commit message
-->

<details><summary>
<h2>Kuma affecting changes:</h2>
<table><td>
📝 <strong>Note:</strong> See <a href="https://github.com/PrismJS/prism/releases/tag/v1.16.0">PrismJS/prism@v1.16.0</a> for a full list of changes.
</td></table></summary>

### Updated components

* __C__
	* Improve C language (PrismJS/prism#1697) PrismJS/prism@7eccea5c
* __C-like__
	* Simplify function pattern of C-like language (PrismJS/prism#1552) PrismJS/prism@b520e1b6
* __C/C++/Java__
	* Operator fixes (PrismJS/prism#1528) PrismJS/prism@7af8f8be
* __CSS__
	* Fix tokenizing !important (PrismJS/prism#1585) PrismJS/prism@c1d6cb85
	* Added the comma to the list of CSS punctuation PrismJS/prism@7ea2ff28
	* CSS: Comma punctuation (PrismJS/prism#1632) PrismJS/prism@1b812386
	* Reuse CSS selector pattern in CSS Extras (PrismJS/prism#1637) PrismJS/prism@e2f2fd19
	* Fixed CSS extra variable (PrismJS/prism#1649) PrismJS/prism@9de47d3a
	* Identify CSS units and variables (PrismJS/prism#1450) PrismJS/prism@5fcee966
	* Allow multiline CSS at-rules (PrismJS/prism#1676) PrismJS/prism@4f6f3c7d
	* CSS: Highlight attribute selector (PrismJS/prism#1671) PrismJS/prism@245b59d4
	* CSS: Selectors can contain any string (PrismJS/prism#1638) PrismJS/prism@a2d445d0
	* CSS extras: Highlighting for pseudo class arguments (PrismJS/prism#1650) PrismJS/prism@70a40414
* __Java__
	* Add Java 10 support (PrismJS/prism#1549) PrismJS/prism@8c981a22
	* Added module keywords to Java. (PrismJS/prism#1655) PrismJS/prism@6e250a5f
	* Improve Java (PrismJS/prism#1474) PrismJS/prism@81bd8f0b
* __JavaScript__
	* Fix regex for `catch` and `finally` (PrismJS/prism#1527) PrismJS/prism@ebd1b9a6
	* Highlighting of supposed classes and functions (PrismJS/prism#1482) PrismJS/prism@c40f6047
	* Added support for JS BigInt literals (PrismJS/prism#1542) PrismJS/prism@2b62e57b
	* Fixed lowercase supposed class names (PrismJS/prism#1544) PrismJS/prism@a47c05ad
	* Fixes regex for JS examples (PrismJS/prism#1591) PrismJS/prism@b41fb8f1
	* Improve regex detection in JS (PrismJS/prism#1473) PrismJS/prism@2a4758ab
	* Identify JavaScript function parameters (PrismJS/prism#1446) PrismJS/prism@0cc8c56a
	* Improved JavaScript parameter recognization (PrismJS/prism#1722) PrismJS/prism@57a92035
	* Make `undefined` a keyword in JS (PrismJS/prism#1740) PrismJS/prism@d9fa29a8
	* Fix `function-variable` in JS (PrismJS/prism#1739) PrismJS/prism@bfbea4d6
	* Improved JS constant pattern (PrismJS/prism#1737) PrismJS/prism@7bcec584
	* Improved JS function pattern (PrismJS/prism#1736) PrismJS/prism@8378ac83
	* JS: Fixed variables named "async" (PrismJS/prism#1738) PrismJS/prism@3560c643
	* JS: Keyword fix (PrismJS/prism#1808) PrismJS/prism@f2d8e1c7
* __JSON__ / __JSONP__
	* Fix bugs in JSON language (PrismJS/prism#1479) PrismJS/prism@74fe81c6
	* Adds support for comments in JSON (PrismJS/prism#1595) PrismJS/prism@8720b3e6
	* Cleaned up JSON (PrismJS/prism#1596) PrismJS/prism@da474c77
	* Added `keyword` alias to JSON's `null` (PrismJS/prism#1733) PrismJS/prism@eee06649
	* Fix JSONP support (PrismJS/prism#1745) PrismJS/prism@b5041cf9
	* Fixed JSON/JSONP examples (PrismJS/prism#1765) PrismJS/prism@ae4842db
* __Markup__
	* Decouple XML from Markup (PrismJS/prism#1603) PrismJS/prism@0030a4ef
	* Fix for markup attributes (PrismJS/prism#1752) PrismJS/prism@c3862a24
	* Markup: Added support for CSS and JS inside of CDATAs (PrismJS/prism#1660) PrismJS/prism@57127701
	* Markup `addInline` improvements (PrismJS/prism#1798) PrismJS/prism@af67c32e
* __Rust__
	* Add missing keywords (PrismJS/prism#1634) PrismJS/prism@3590edde

### Updated plugins

* Better class name detection for plugins (PrismJS/prism#1772) PrismJS/prism@c9762c6f
* __Line Numbers__
	* Added inheritance for the `line-numbers` class (PrismJS/prism#1799) PrismJS/prism@14be7489

### Other changes

* __Core__
	* `insertBefore` now correctly updates references (PrismJS/prism#1531) PrismJS/prism@9dfec340
	* Invoke `callback` after `after-highlight` hook (PrismJS/prism#1588) PrismJS/prism@bfbe4464
	* Improve `Prism.util.type` performance (PrismJS/prism#1545) PrismJS/prism@2864fe24
	* Remove unused `insertBefore` overload (PrismJS/prism#1631) PrismJS/prism@39686e12
	* Ignore duplicates in insertBefore (PrismJS/prism#1628) PrismJS/prism@d33d259c
	* Remove the Prism.tokenize language parameter (PrismJS/prism#1654) PrismJS/prism@fbf0b094
	* Call `insert-before` hook properly (PrismJS/prism#1709) PrismJS/prism@393ab164
	* Improved languages.DFS and util.clone (PrismJS/prism#1506) PrismJS/prism@152a68ef
	* Core: Avoid redeclaring variables in util.clone (PrismJS/prism#1778) PrismJS/prism@b06f532f
	* Made prism-core a little more editor friendly (PrismJS/prism#1776) PrismJS/prism@bac09f0a
	* Applied Array.isArray (PrismJS/prism#1804) PrismJS/prism@11d0f75e

</details>

---

review?(@jwhitlock)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants