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: introduce getter and setter kinds + computed properties #1989

Merged
merged 3 commits into from Jan 28, 2019

Conversation

masatake
Copy link
Member

Partially close #1949.
See https://www.ecma-international.org/ecma-262/5.1/#sec-11.1.5

Signed-off-by: Masatake YAMATO yamato@redhat.com

@coveralls
Copy link

coveralls commented Jan 26, 2019

Coverage Status

Coverage increased (+0.01%) to 84.965% when pulling 9d44448 on masatake:js-getter-and-setter into 75ba24d on universal-ctags:master.

@masatake
Copy link
Member Author

I wonder we can remove signature field for getter kind objects.

Signed-off-by: Masatake YAMATO <yamato@redhat.com>
…ted in string literals

Partially close universal-ctags#1949.

Note: in universal-ctags#1949, An idea capturing [Symbol.*] is show.
This change doesn't touch the idea to make the change small.

Signed-off-by: Masatake YAMATO <yamato@redhat.com>
@masatake masatake changed the title JavaScript: introduce getter and setter kinds JavaScript: introduce getter and setter kinds + computed properties Jan 28, 2019
Copy link
Member

@b4n b4n left a comment

Choose a reason for hiding this comment

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

Looks pretty good!

However, there's case where we'll have to do something with computed property names that aren't a simple literal: unfortunately, the following code is perfectly valid JS:

var o = {
	[(1+2)*3]: {
		subnum: function (){}
	},
};

Currently this will cause trouble ("warning: null tag" for the property, and subnum has an invalid scope).
I'm not sure if it's worth doing much here, but maybe we should record the expression and just use this literally? Not sure what makes sense, as we won't ever do this perfectly correctly though -- here the property name is actually 9, but could be an arbitrary computation using dynamic variables and such.

In any case, I think this can be dealt with later, as it's covering the cases where we really have something correct to give.

@masatake
Copy link
Member Author

I introduced is_dynamic_prop local variable to skip (1+2)*3. If you did not know this, look at
9d44448.
However, I didn' handle the nested case. For capturing subnum and fill the scope field for subnum, we have to put an anonymous tag as the proxy(?) for (1+2)*3.

Anyway I guess we have more things to do about #1949. So I would like to merge this PR.
Is your approve for my changes including 9d44448 ?

@b4n
Copy link
Member

b4n commented Jan 28, 2019

@masatake yes I've seen it, but as mentioned it's not checked in case of recursing into a subclass; and there we need something to provide scope for the members. Another way could be to ignore the subclass methods in this case.

And yes, my review applies at the time I made it, to the 3 commits.

@masatake
Copy link
Member Author

@b4n, thank you!

@masatake masatake merged commit b3d156f into universal-ctags:master Jan 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JavaScript: Missing methods inside modernish JavaScript class
3 participants