ctags icon indicating copy to clipboard operation
ctags copied to clipboard

[WIP] JavaScript: destructuring binding

Open masatake opened this issue 3 years ago • 24 comments

Close #1112.

Limitations:

  • [ ] If an object literal is specified as a default value in object restructuring, the parser may fail to extract the variable (or constant):

    const {var = {a: 1}};
    

    I will try to remove this limitation.

  • [ ] key in [key] is extracted unexpectedly.

    let key = 'z';
    let {[key]: foo} = {z: 'bar'};
    
  • [x] the parser fills the signature field for userDisplayName with a wrong value.

    function userDisplayName({displayName: dname}) {
       return dname;
    }
    
  • [ ] f is not tagged with function kind.

     let [f] = [function() {}];
    

    I will not try to fix this. I cannot find any easy way to remove this limitation.

ref. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Destructuring_assignment

masatake avatar Jul 09 '22 19:07 masatake

Codecov Report

Attention: Patch coverage is 93.28358% with 9 lines in your changes are missing coverage. Please review.

Project coverage is 85.38%. Comparing base (38fd8e3) to head (501e14b).

Files Patch % Lines
parsers/jscript.c 93.28% 9 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3435      +/-   ##
==========================================
+ Coverage   85.37%   85.38%   +0.01%     
==========================================
  Files         235      235              
  Lines       56458    56565     +107     
==========================================
+ Hits        48200    48299      +99     
- Misses       8258     8266       +8     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Jul 10 '22 01:07 codecov[bot]

Rebased on the latest master branch. @jafl, this pull request still has some TODO items. But I would like to get your review.

masatake avatar Aug 26 '22 17:08 masatake

@masatake Thanks for rebasing! I have had this one on my todo list for a while. I will try to get to it soon.

jafl avatar Aug 26 '22 19:08 jafl

It seems to be working to a limit.

Deconstruction in a multilines block of code fails:

files.push({
  relative, // <----
  basename: name,
  absolute: loc,
  mtime: +stat.mtime
});

I don't know if it is related but the following form fails as well:

computed: {
  ...mapGetters([
    'getAsylumAssistant',
    'getModel',
    'getType',
  ]),
},

I thought these forms would work but apparently not:

this.stagedlocalRecord = { localRecord, localRecordIndex }
// ---
return this.saveData({ partyId, id, data, rubyType: dbTypes[type], jsType: type })
// ---
createDocketEntryModel ({ commit, state }, requestData) {
  // ...
}

And there might be more but it takes too long for me to check all the warnings :sweat_smile:

AdrienGiboire avatar Jan 09 '23 15:01 AdrienGiboire

@AdrienGiboire thank you.

files.push({
  relative, // <----

I don't understand this. Is relative a variable? Is there any document that describes this syntax?

I cannot find the notation in https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Destructuring_assignment.

masatake avatar Jan 11 '23 23:01 masatake

files.push({
  relative, // <----

This assumes that relative is a variable, and it is shorthand for

files.push({
  relative: relative,

jafl avatar Jan 12 '23 01:01 jafl

@jafl Thank you. So the question is, in which context can we use the shorthand syntax?

masatake avatar Jan 12 '23 03:01 masatake

Here it describes the case you're wondering about, especially this part:

Function parameter definitions

As developers, we can often expose more ergonomic APIs by accepting a single object with multiple properties as a parameter instead of forcing our API consumers to remember the order of many individual parameters. We can use destructuring to avoid repeating this single parameter object whenever we want to reference one of its properties:

function removeBreakpoint({ url, line, column }) {
  // ...
}

Knowing that it should support default values.

jQuery.ajax = function (url, {
  async = true,
  beforeSend = noop,
  cache = true,
  complete = noop,
  crossDomain = false,
  global = true,
  // ... more config
}) {
  // ... do stuff
};

Basically, if a variable is between {} and there is no explicit key, it's meant to be desconstructed. If an assignment is between {}, it's meant to be desconstructed and have a default value.

AdrienGiboire avatar Jan 12 '23 05:01 AdrienGiboire

@AdrienGiboire Thank you.

function removeBreakpoint({ url, line, column }) {
  // ...
}

I found even this input causes the warning message. I found we can skip such parameter lists because the JavaScript parser doesn't extract parameters as tags. I added a commit for this change.

files.push({
  relative, // <----
  basename: name,
  absolute: loc,
  mtime: +stat.mtime
});

It seems that we must improve skipArgumentList(). I will inspect this one more.

masatake avatar Jan 12 '23 21:01 masatake

$ cat /tmp/foo.js                                                                                                                                     
cat /tmp/foo.js                                                                                                                                       
computed: {                                                                                                                                           
  ...mapGetters([                                                                                                                                     
    'getAsylumAssistant',                                                                                                                             
    'getModel',                                                                                                                                       
    'getType',                                                                                                                                        
  ]),                                                                                                                                                 
},                                                                                                                                                    
$ node foo.js                                                                                                                                         
node foo.js                                                                                                                                           
node:internal/modules/cjs/loader:998                                                                                                                  
  throw err;                                                                                                                                          
  ^                                                                                                                                                   
                                                                                                                                                      
Error: Cannot find module '/home/jet/var/ctags-github/foo.js'                                                                                         
    at Module._resolveFilename (node:internal/modules/cjs/loader:995:15)                                                                              
    at Module._load (node:internal/modules/cjs/loader:841:27)                                                                                         
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:81:12)                                                             
    at node:internal/main/run_main_module:23:47 {                                                                                                     
  code: 'MODULE_NOT_FOUND',                                                                                                                           
  requireStack: []                                                                                                                                    
}                                                                                                                                                     
                                                                                                                                                      
Node.js v18.12.1                          

Could you provide an example input that node accepts? Thank you.

masatake avatar Jan 12 '23 23:01 masatake

$ cat /tmp/foo.js                                                                                                                                     
cat /tmp/foo.js                                                                                                                                       
computed: {                                                                                                                                           
  ...mapGetters([                                                                                                                                     
    'getAsylumAssistant',                                                                                                                             
    'getModel',                                                                                                                                       
    'getType',                                                                                                                                        
  ]),                                                                                                                                                 
},                                                                                                                                                    
$ node foo.js                                                                                                                                         
node foo.js                                                                                                                                           
node:internal/modules/cjs/loader:998                                                                                                                  
  throw err;                                                                                                                                          
  ^                                                                                                                                                   
                                                                                                                                                      
Error: Cannot find module '/home/jet/var/ctags-github/foo.js'                                                                                         
    at Module._resolveFilename (node:internal/modules/cjs/loader:995:15)                                                                              
    at Module._load (node:internal/modules/cjs/loader:841:27)                                                                                         
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:81:12)                                                             
    at node:internal/main/run_main_module:23:47 {                                                                                                     
  code: 'MODULE_NOT_FOUND',                                                                                                                           
  requireStack: []                                                                                                                                    
}                                                                                                                                                     
                                                                                                                                                      
Node.js v18.12.1                          

Could you provide an example input that node accepts? Thank you.

Did you mean to run node /tmp/foo.js instead?

AdrienGiboire avatar Nov 16 '23 10:11 AdrienGiboire

Did you mean to run node /tmp/foo.js instead?

You are correct. However, even though I specified /tmp/foo.js, I got an error.

$ cat /tmp/foo.js 
computed: {                                                                                                                                           
  ...mapGetters([                                                                                                                                     
    'getAsylumAssistant',                                                                                                                             
    'getModel',                                                                                                                                       
    'getType',                                                                                                                                        
  ]),                                                                                                                                                 
},
$ node /tmp/foo.js 
/tmp/foo.js:2
  ...mapGetters([                                                                                                                                     
  ^^^

SyntaxError: Unexpected token '...'
    at internalCompileFunction (node:internal/vm:73:18)
    at wrapSafe (node:internal/modules/cjs/loader:1178:20)
    at Module._compile (node:internal/modules/cjs/loader:1220:27)
    at Module._extensions..js (node:internal/modules/cjs/loader:1310:10)
    at Module.load (node:internal/modules/cjs/loader:1119:32)
    at Module._load (node:internal/modules/cjs/loader:960:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:86:12)
    at node:internal/main/run_main_module:23:47

Node.js v18.18.2

masatake avatar Nov 16 '23 19:11 masatake

You can try something like this:

const embedded = { foo: 'bar' }
const something = {
  ...embedded,
  baz: 'fox',
}

AdrienGiboire avatar Nov 16 '23 19:11 AdrienGiboire

Thank you. Based on your hit, I modified /tmp/foo.js:

$ cat /tmp/foo.js
const mapGetters = function() {}
const computed = {                                                                                                                                           
  ...mapGetters([                                                                                                                                     
    'getAsylumAssistant',                                                                                                                             
    'getModel',                                                                                                                                       
    'getType',                                                                                                                                        
  ]),                                                                                                                                                 
}
$ node /tmp/foo.js
$

Works fine. Thank you.

[yamato@dev64]~/var/ctags-github% git rebase master
Successfully rebased and updated refs/heads/js--destructural-binding.

[yamato@dev64]~/var/ctags-github% make
...
[yamato@dev64]~/var/ctags-github% ./ctags -o - /tmp/foo.js 
ctags: Notice: ignoring null tag in /tmp/foo.js(line: 3, language: JavaScript)
computed	/tmp/foo.js	/^const computed = {                                                                              /;"	C
mapGetters	/tmp/foo.js	/^const mapGetters = function() {}$/;"	f

I cannot remember what we were talking about quickly. But I guess the issue is about ignoring null tag warning.

masatake avatar Nov 16 '23 19:11 masatake

[yamato@dev64]~/var/ctags-github% cat /tmp/bar.js         
const embedded = { foo: 'bar' }
const something = {
  ...embedded,
  baz: 'fox',
}
[yamato@dev64]~/var/ctags-github% ./ctags -o - /tmp/bar.js
ctags: Notice: ignoring null tag in /tmp/bar.js(line: 3, language: JavaScript)
embedded	/tmp/bar.js	/^const embedded = { foo: 'bar' }$/;"	v
foo	/tmp/bar.js	/^const embedded = { foo: 'bar' }$/;"	p	variable:embedded
something	/tmp/bar.js	/^const something = {$/;"	C

I found two issues here:

  • [x] the null tag warning
  • [x] Though foo in embedded is extracted, baz in something is not.

embedded in something is not a definition, so we don't have to extract it as a definition tag. Instead, THe parser should skip the ...embedded.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Spread_syntax

masatake avatar Nov 16 '23 19:11 masatake

It makes sense but at the same time would it not be legit to have the definition?

For example, you can have something like:

const something = {
  fn: () => { /* doing something */ },
}

It would make sense to have the definition of the function referenced. What do you think?

AdrienGiboire avatar Nov 16 '23 20:11 AdrienGiboire

This is a difficult question. However, regarding the example, fn should be tagged as a definition tag. fn is introduced as a new name.

const embedded = { foo: 'bar' }
const something = {
  ...embedded,
  baz: 'fox',
}

In my understanding, this is the same as:

const something = {
  foo: 'bar',
  baz: 'fox',
}

Am I correct?

The embedded in something is not a key. So embedded should not be tagged as a definition tag. It is a reference tag having shredded role of variable (or unknown) kind.


Added after commenting

[yamato@dev64]~/var/ctags-github% cat /tmp/bar.js 
const embedded = { foo: 'bar' }
const something = {
  ...embedded,
  baz: 'fox',
}

something.foo;

[yamato@dev64]~/var/ctags-github% node /tmp/bar.js

My understanding may be correct.

masatake avatar Nov 16 '23 20:11 masatake

[yamato@dev64]~/var/ctags-github% git diff                
diff --git a/parsers/jscript.c b/parsers/jscript.c
index 78f563dde..f316d7f7d 100644
--- a/parsers/jscript.c
+++ b/parsers/jscript.c
@@ -2019,6 +2019,12 @@ static bool parseMethods (tokenInfo *const token, int class_index,
 
            deleteToken (saved_token);
        }
+       else if (isType (token, TOKEN_DOTS))
+       {
+           /* maybe spread operator. Just skip the next expression. */
+           findCmdTerm(token, true, true);
+           continue;
+       }
 
        if (! isType (token, TOKEN_KEYWORD) &&
            ! isType (token, TOKEN_SEMICOLON))
[yamato@dev64]~/var/ctags-github% cat /tmp/bar.js 
const embedded = { foo: 'bar' }
const something = {
  ...embedded,
  baz: 'fox',
}
[yamato@dev64]~/var/ctags-github% ./ctags --sort=no -o - /tmp/bar.js
foo	/tmp/bar.js	/^const embedded = { foo: 'bar' }$/;"	p	variable:embedded
embedded	/tmp/bar.js	/^const embedded = { foo: 'bar' }$/;"	v
baz	/tmp/bar.js	/^  baz: 'fox',$/;"	p	variable:something
something	/tmp/bar.js	/^const something = {$/;"	v

masatake avatar Nov 16 '23 20:11 masatake

Surprisingly, the change for skipping spread operators works well with the current master branch. So I can submit it as a standalone pull request.

masatake avatar Nov 16 '23 21:11 masatake

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Spread_syntax

This tells spread syntax. I'm looking for the spread operator, ",@" in lisp..

masatake avatar Nov 16 '23 21:11 masatake

const embedded = { foo: 'bar' }
const something = {
  ...embedded,
  baz: 'fox',
}

In my understanding, this is the same as:

const something = {
  foo: 'bar',
  baz: 'fox',
}

Am I correct?

Pretty much, yes.

Surprisingly, the change for skipping spread operators works well with the current master branch. So I can submit it as a standalone pull request.

It does look good :+1:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Spread_syntax

This tells spread syntax. I'm looking for the spread operator, ",@" in lisp..

I'm not sure I understand what you mean. And I know nothing about Lisp so I can't relate ^^' I tried googling but I have not seen ,@ :man_shrugging:

AdrienGiboire avatar Nov 17 '23 09:11 AdrienGiboire

@AdrienGiboire Sorry, after rereading the page at developer.mozilla.org, "spread syntax" is the correct word.

masatake avatar Nov 17 '23 19:11 masatake

@jafl, can I merge this?

masatake avatar Dec 09 '23 18:12 masatake

.b test cases must be added.

masatake avatar Dec 22 '23 20:12 masatake