Skip to content

JavaScript: # indicates a private class field or method #4269

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jafl
Copy link
Contributor

@jafl jafl commented Jun 6, 2025

No description provided.

@jafl jafl requested a review from masatake June 6, 2025 20:33
@jafl
Copy link
Contributor Author

jafl commented Jun 6, 2025

@masatake How can I indicate that a private field is also static?

Copy link

codecov bot commented Jun 6, 2025

Codecov Report

Attention: Patch coverage is 96.96970% with 1 line in your changes missing coverage. Please review.

Project coverage is 85.96%. Comparing base (c06d333) to head (4512da5).

Files with missing lines Patch % Lines
parsers/jscript.c 96.96% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4269   +/-   ##
=======================================
  Coverage   85.95%   85.96%           
=======================================
  Files         246      246           
  Lines       63434    63454   +20     
=======================================
+ Hits        54525    54546   +21     
+ Misses       8909     8908    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@masatake
Copy link
Member

masatake commented Jun 7, 2025

@masatake How can I indicate that a private field is also static?

Interesting topic.

The main part doesn't provide a standard way for representing static methods (or class methods).

There are two approaches:

  1. Introducing a parser-specific field for recording "this method is static".
  2. Introducing a specialized kind for representing static methods.

Let's see some parsers.

Python uses a parser-specific field named decorators:.

$ cat foo.py
class C:
    @classmethod
    def m():
        pass
$ ~/bin/ctags -o - --fields-Python='*' /tmp/foo.py 
C	/tmp/foo.py	/^class C:$/;"	c
m	/tmp/foo.py	/^    def m():$/;"	m	class:C	decorators:classmethod

C++ also uses a parser-specific field named properties:.

$ cat /tmp/foo.cpp 
class C {
  static void m(void) {}
};
$ ~/bin/ctags -o - --fields-C++='*' /tmp/foo.cpp
C	/tmp/foo.cpp	/^class C {$/;"	c	file:
m	/tmp/foo.cpp	/^  static void m(void) {}$/;"	f	class:C	typeref:typename:void	file:	properties:static

Ruby uses a specialized kind named singletonMethod for the purpose:

$ cat /tmp/foo.rb
class C
  def self.m
    
  end
end
$ ~/bin/ctags -o - --fields=+Kz  --fields-Ruby='*' /tmp/foo.rb
C	/tmp/foo.rb	/^class C$/;"	kind:class
m	/tmp/foo.rb	/^  def self.m$/;"	kind:singletonMethod	class:C

@jafl
Copy link
Contributor Author

jafl commented Jun 7, 2025

@masatake I am hoping to do it the C++ way. From studying the code, however, I couldn't figure out how it was done. I found cxxTagSetProperties in cxx_tag.c, which ultimately calls attachParserField in entry.c. What fieldType should I specify (from field.h)?

@masatake
Copy link
Member

masatake commented Jun 8, 2025

I recommend you read the code parser/python.c and the comment about attachParserField() in main/entry.h.

diff --git a/parsers/jscript.c b/parsers/jscript.c
index 168dfb533..869daac0a 100644
--- a/parsers/jscript.c
+++ b/parsers/jscript.c
@@ -41,6 +41,7 @@
 #include "debug.h"
 #include "dependency.h"
 #include "entry.h"
+#include "field.h"
 #include "keyword.h"
 #include "numarray.h"
 #include "parse.h"
@@ -251,6 +252,10 @@ typedef enum {
 	JS_CLASS_CHAINELT,
 } jsClassRole;
 
+typedef enum {
+	F_THEFIELD,
+} jsField;
+
 static roleDefinition JsFunctionRoles [] = {
 	/* Currently V parser wants this items. */
 	{ true, "foreigndecl", "declared in foreign languages" },
@@ -280,6 +285,14 @@ static kindDefinition JsKinds [] = {
 	{ true,  'M', "field",		  "fields"           },
 };
 
+static fieldDefinition JsFields[] = {
+	{
+		.name = "theNameOfField",
+		.description = "...description...",
+		.enabled = true,
+	},
+};
+
 static const keywordTable JsKeywordTable [] = {
 	/* keyword		keyword ID */
 	{ "function",	KEYWORD_function			},
@@ -527,6 +540,14 @@ static int makeJsTagCommon (const tokenInfo *const token, const jsKind kind,
 	updateTagLine (&e, token->lineNumber, token->filePosition);
 	e.extensionFields.scopeIndex = scope;
 
+	/* There are two ways (A and B) to attach a value to field.
+	 *
+	 * This is the way A. A. is suitable for a static value.
+	 * Attaching after initTagEntry() before makeTagEntry():  */
+	if (is_static)
+		attachParserField (&e, JsFields[F_THEFIELD].ftype,
+						   "value");
+
 	if (is_private)
 		e.extensionFields.access = "private";
 
@@ -565,6 +586,21 @@ static int makeJsTagCommon (const tokenInfo *const token, const jsKind kind,
 		e.allowNullTag = 1;
 
 	index = makeTagEntry (&e);
+
+	/*
+	 * This is the way B. B. is suitable for dynamic value.
+	 * Attaching after makeTagEntry():
+	 *
+	 * Choose A or B.
+	 */
+	{
+		vString *tmp = vStringNew();
+		/* build the value for the field */
+		attachParserFieldToCorkEntry (index, JsFields[F_THEFIELD].ftype,
+									  vStringValue (tmp));
+		vStrngDelete (tmp);
+	}
+
 	/* We shold remove This condition. We should fix the callers passing
 	 * an empty name instead. makeTagEntry() returns CORK_NIL if the tag
 	 * name is empty. */
@@ -3797,6 +3833,8 @@ extern parserDefinition* JavaScriptParser (void)
 	 */
 	def->kindTable	= JsKinds;
 	def->kindCount	= ARRAY_SIZE (JsKinds);
+	def->fieldTable = JsFields;
+	def->fieldCount = ARRAY_SIZE (JsFields);
 	def->parser		= findJsTags;
 	def->initialize = initialize;
 	def->finalize   = finalize;

if (is_private)
{
vString * s = vStringNew ();
vStringPut (s, '#');
Copy link
Member

Choose a reason for hiding this comment

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

You can unify the two function calls into one with:

vString * s = vStringNewInit ("#");

I don't say this is efficient:-).

if (is_private)
{
vString * s = vStringNew ();
vStringPut (s, '#');
Copy link
Member

Choose a reason for hiding this comment

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

vStringNewInit() can be used here.

vStringPut (s, '#');
vStringCat (s, name->string);
vStringCopy (name->string, s);
vStringDelete (s);
Copy link
Member

Choose a reason for hiding this comment

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

Instead of updating the string member of the name token, we can make the # prefixed private name in makeJsTagCommon.

diff --git a/parsers/jscript.c b/parsers/jscript.c
index 168dfb533..e88799c2e 100644
--- a/parsers/jscript.c
+++ b/parsers/jscript.c
@@ -486,12 +499,21 @@ static int makeJsRefTagsForNameChain (char *name_chain, const tokenInfo *token,
 		: index;
 }
 
+static vString *makPrivateName (vString *orignal)
+{
+	vString * s = vStringNewInit ("#");
+	vStringCopy (s, orignal);
+	return s;
+}
+
 static int makeJsTagCommon (const tokenInfo *const token, const jsKind kind,
 							vString *const signature, vString *const inheritance,
 							bool anonymous, bool is_private, bool nulltag)
 {
 	int index = CORK_NIL;
-	const char *name = vStringValue (token->string);
+	vString *vname = is_private? makePrivateName (vname): token->string;
+	const char *name = vStringValue (vname);
+
 
 	const char *p;
 	char *name_chain = NULL;
@@ -501,7 +523,11 @@ static int makeJsTagCommon (const tokenInfo *const token, const jsKind kind,
 			name_chain = eStrndup (name, (size_t) (p - name));
 		name = p + 1;
 		if (name[0] == '\0')
+		{
+			if (is_private)
+				vStringDelete (vname);
 			return CORK_NIL;
+		}
 	}
 
 	int scope = token->scope;
@@ -565,8 +599,11 @@ static int makeJsTagCommon (const tokenInfo *const token, const jsKind kind,
 	 * an empty name instead. makeTagEntry() returns CORK_NIL if the tag
 	 * name is empty. */
 	if (index != CORK_NIL)
 		registerEntry (index);
 
+	if (is_private)
+		vStringDelete (vname);
+
 	return index;
 }
 

Just an idea.

@masatake
Copy link
Member

masatake commented Jun 8, 2025

Whether # is part of the method name is a question you/we must ponder.
These days, I discuss these topics with ChatGPT:-P

Thinking about shell scripts, do you think $ is a part of the name of a variable?

Even if a name doesn't include #, a user can know the method is private from acccess:private field.

These are just inputs to you. I have no answer for you. Please, make JavaScript people and developers of client tools comfortable!

@jafl
Copy link
Contributor Author

jafl commented Jun 9, 2025

Thinking about shell scripts, do you think $ is a part of the name of a variable?

Absolutely. In JavaScript, a class can have both a public function foo and a private function #foo. And in Perl, you can have $foo and @foo and %foo.

@jafl jafl force-pushed the javascript-private-class-members branch from e5dff62 to 538cd02 Compare June 10, 2025 03:44
@jafl
Copy link
Contributor Author

jafl commented Jun 10, 2025

@masatake Thanks for the help. I have updated the PR.

@masatake
Copy link
Member

Unlike C++, JavaScript itself has the concept named properties. I'm afraid that using the name properties as the new parser-specific field name makes ctags' users programming with JavaScript confused. Is it O.K.?


The test is failed because you added a new field.
After running make tmain locally, run misc/review.
You can reflect the actual output to expectext output interactively.

... make tmain ...
Failed tests
============================================================
ptag-in-optlib-parser/stdout-compare
list-roles/stdout-compare
list-fields/stdout-compare

make: *** [Makefile:8166: tmain] Error 1
[yamato@dev64]~/var/ctags-github% misc/review 
1) <n>ext
2) <d>iff
3) <a>ccept
4) <S>hell
5) <R>un
6) <q>uit
[1/3 [ Tmain/list-fields.d ]]? =
###  stdout
--- ./Tmain/list-fields.d/stdout-expected.txt	2025-06-10 15:45:33.851066553 +0900
+++ /home/yamato/var/ctags-github/Tmain/list-fields.d/stdout-actual.txt	2025-06-10 15:45:43.336692476 +0900
@@ -34,0 +35,2 @@
+-	macrodef	no	C++	s--	no	--	macro definition
+-	name	yes	C++	s--	no	--	aliased names
[1/3 [ Tmain/list-fields.d ]]? a
1) <n>ext
2) <d>iff
3) <a>ccept
4) <S>hell
5) <R>un
6) <q>uit
[2/3 [ Tmain/list-roles.d ]]? 
...

When adding/removing kinds, roles, parser-specific fields, and/or parser-specific extras (KRFX or KRFE), I would like you to update versionCurrent and versionAge of the parser like:

diff --git a/parsers/jscript.c b/parsers/jscript.c
index d9cb6d07d..d7ff2d44e 100644
--- a/parsers/jscript.c
+++ b/parsers/jscript.c
@@ -3773,8 +3773,8 @@ extern parserDefinition* JavaScriptParser (void)
 	def->dependencies = dependencies;
 	def->dependencyCount = ARRAY_SIZE (dependencies);
 
-	def->versionCurrent = 1;
-	def->versionAge = 1;
+	def->versionCurrent = 2;
+	def->versionAge = 2;
 
 	return def;
 }

See main/parse.h when we should increment the members.
See also a planed change about parser versioning, #4271 .

@jafl
Copy link
Contributor Author

jafl commented Jun 10, 2025

Unlike C++, JavaScript itself has the concept named properties. I'm afraid that using the name properties as the new parser-specific field name makes ctags' users programming with JavaScript confused. Is it O.K.?

I think it's good to be consistent with the rest of the languages that use "static". I suppose we could use "attributes", though that is also a keyword.

@masatake
Copy link
Member

My alternative ideas are static:1, static:yes, static:true, or static:.
These are just ideas. If you, a person who uses JavaScript, think it is comfortable, it's good.

@jafl jafl force-pushed the javascript-private-class-members branch from 538cd02 to 4512da5 Compare June 11, 2025 01:34
@masatake
Copy link
Member

masatake commented Jun 11, 2025

Sorry to be late in saying the following:

  • Write about the new field to "Parser related changes" in docs/news/HEAD.rst
  • Enabling the field by default is not a bad idea.

@jafl jafl force-pushed the javascript-private-class-members branch from 4512da5 to 69108e1 Compare June 11, 2025 03:50
@jafl
Copy link
Contributor Author

jafl commented Jun 11, 2025

Sorry to be late in saying the following

No problem! Updated.

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.

2 participants