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

Moose subparser based on perl parser #2070

Merged
merged 4 commits into from May 14, 2019

Conversation

masatake
Copy link
Member

@masatake masatake commented Apr 1, 2019

Close #2067.

If a subparser uses addLanguageCallbackRegex, ctags didn't call
a inputStart() subparser method. This is a bug.

ctags should not call inputStart() of a subparser if
METHOD_NOT_CRAFTED is set to the method field of the subparser.

There is a critical typo to extract the it for METHOD_NOT_CRAFTED from
a flag.

Signed-off-by: Masatake YAMATO <yamato@redhat.com>
Signed-off-by: Masatake YAMATO <yamato@redhat.com>
Signed-off-by: Masatake YAMATO <yamato@redhat.com>
@masatake
Copy link
Member Author

masatake commented Apr 2, 2019

A critical internal bug is fixed.

parsers/moose.c Outdated
"{exclusive}",
extendClass, &mooseSubparser.notInMoose,
&mooseSubparser);
addLanguageCallbackRegex (language, "^ *(has|after) *'([^']+)'",

Choose a reason for hiding this comment

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

If you're going to do after you may as well do around and before just to finish them off. They all fit into a class of Aspect Oriented Programming modifiers that wrap methods provided by the parent class.

Choose a reason for hiding this comment

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

As another special note, it seems from this regex that the ' is required but in perl, they're not required (and they can and ofter are omitted entirely). As an added complexity they can be " (double quotes) too.

Choose a reason for hiding this comment

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

This seems to only cover the has 'literal1' =>, but there is another form the has [qw/literal1 literal2/] form which is a shorthand to define many attributes with the same property.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see. I will add more commits for the comments.

parsers/moose.c Outdated

static void initializeMooseParser (langType language)
{
addLanguageCallbackRegex (language, "^ *use +Moose(:| *;)",
Copy link

Choose a reason for hiding this comment

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

I'm not sure how this syntax works, but this sublanguage which we've been referring to in the ticket as "Moose et al" should also trigger on Moo which also provides all these same features. https://metacpan.org/pod/Moo#IMPORTED-SUBROUTINES

Mouse would be another module with the same interface. https://metacpan.org/pod/Mouse

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe just "use Moose;" is enough. I should remove "Moose:".

I don't know Moo but I guess we should implement is as a different subparser running on Perl.
even if Moo and Moose subparesrs can share C functions and data types definitions.

Copy link
Member Author

Choose a reason for hiding this comment

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

"Moose:" may have a sense when we support Rules.

Choose a reason for hiding this comment

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

Moo and Mouse essentially follow the same spec as Moose. So much so that I think they both naturally use Moose if it's there. (it becomes a noop) The primary reason for their existence is to shift work into compile time and eliminate work from run-time, but they coat-tail (follow very closely) the Moose syntax.

Choose a reason for hiding this comment

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

@masatake we're in a situation where I either have to duplicate the patch for a new subparser "Moo" that works that the same on the initial push. Or, we can add Moo to the trigger so it works on both. The difference between the two is pretty minimal. I don't see the purpose of maintaining two subparsers, but we are losing a lot of users of this because it's not yet catching Moo and Mouse. Please advise. If you want me to duplicate the parser (and thus have one subparser for each Moose we'll quickly stack up a few highly duplicate code bases).

Copy link
Member Author

Choose a reason for hiding this comment

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

Having multiple object systems in a language. It looks a bit crazy situation. But I just remember "There's More Than One Way To Do It."
This is the perl way.

Copy link

Choose a reason for hiding this comment

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

@masatake they are not the same because of their implementation. The API into them is the same. The sublanguage should be based on the API. For example, there would be no difference in the patch you've provided to support Moose and Moo other than the enter language line. In fact, in my use case here I don't even care about Moose. I've verified your code works by changing that Moose to (Moose|Moo).

Copy link
Member Author

Choose a reason for hiding this comment

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

I've verified your code works by changing that Moose to (Moose|Moo).

How about Mouse?

Choose a reason for hiding this comment

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

I think we can and should add Mouse to it. None of these libraries have any significant change in API. The differences are extremely minor, and all of these people benefit from your code.

Really 99% of the functional advantage of this patch is simply in the attribute tag with has. That's cool. It'll work on any Moose-like module build system. The remaining useful functionality is mostly the around/before/after method modifiers.

Mouse hasn't been patched since August of last year. Mouse exists so people can write modern perl objects into actual 1992 CGI scripts. It's not a very high visibility project and Perl as a whole is on the decline. I would enable the subparser for Mouse/Moo/Moose and it'll be good enough.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see. Just extending the regex pattern may be enough. However, as a field value, language:Moose is reported for all 3 object systems. "Different things should be tagged differently", is the motto of u-ctags.
Reporting the language field differently for 3 object systems like "language:Moose", "language"Moo", and "language:Moo", will be better in this case
See also #2079 and 2076.

How about the patch at the end of this page?

@EvanCarroll suggested me various Moose notation.

Signed-off-by: Masatake YAMATO <yamato@redhat.com>
@masatake
Copy link
Member Author

masatake commented Apr 7, 2019

Updated.
This one only supporst Moose.

@masatake masatake changed the title [WIP, FYI] Moose subparser based on perl parser Moose subparser based on perl parser Apr 7, 2019
@masatake masatake merged commit c8405be into universal-ctags:master May 14, 2019
@masatake
Copy link
Member Author

How about this one?

diff --git a/parsers/moose.c b/parsers/moose.c
index 1d7aaf10..69ac4cb2 100644
--- a/parsers/moose.c
+++ b/parsers/moose.c
@@ -34,6 +34,16 @@
 #include "routines.h"
 #include "trace.h"
 
+#include <string.h>
+
+/*
+*   MACROS
+*/
+
+#define MOOSE_NAME "Moose"
+#define MOO_NAME   "Moo"
+#define MOUSE_NAME "Mouse"
+
 /*
  *   DATA DECLARATIONS
  */
@@ -447,7 +457,22 @@ static void findMooseTags (void)
 
 static void initializeMooseParser (langType language)
 {
-	addLanguageCallbackRegex (language, "^[ \t]*use +Moose *;",
+	const char *langName = getLanguageName (language);
+	const char *use_pattern;
+
+	Assert (langName);
+
+	if (strcmp (langName, MOOSE_NAME) == 0)
+		use_pattern = "^[ \t]*use +Moose *;";
+	else if (strcmp (langName, MOO_NAME) == 0)
+		use_pattern = "^[ \t]*use +Moo *;";
+	else if (strcmp (langName, MOUSE_NAME) == 0)
+		use_pattern = "^[ \t]*use +Mouse *;";
+	else
+		AssertNotReached ();
+
+
+	addLanguageCallbackRegex (language, use_pattern,
 							  "{exclusive}",
 							  enterMoose, NULL,
 							  &mooseSubparser);
@@ -485,16 +510,10 @@ static void initializeMooseParser (langType language)
 							  &mooseSubparser);
 }
 
-extern parserDefinition* MooseParser (void)
+static parserDefinition* perlMxParserNew (const char *name)
 {
-	parserDefinition* const def = parserNew("Moose");
-
-	static parserDependency dependencies [] = {
-		[0] = { DEPTYPE_SUBPARSER, "Perl", &mooseSubparser },
-	};
+	parserDefinition* const def = parserNew(name);
 
-	def->dependencies = dependencies;
-	def->dependencyCount = ARRAY_SIZE (dependencies);
 
 	def->kindTable = MooseKinds;
 	def->kindCount = ARRAY_SIZE(MooseKinds);
@@ -508,3 +527,36 @@ extern parserDefinition* MooseParser (void)
 
 	return def;
 }
+
+extern parserDefinition* MooseParser (void)
+{
+	parserDefinition* const def = perlMxParserNew (MOOSE_NAME);
+	static parserDependency dependencies [] = {
+		[0] = { DEPTYPE_SUBPARSER, "Perl", &mooseSubparser },
+	};
+	def->dependencies = dependencies;
+	def->dependencyCount = ARRAY_SIZE (dependencies);
+	return def;
+}
+
+extern parserDefinition* MooParser (void)
+{
+	parserDefinition* const def = perlMxParserNew (MOO_NAME);
+	static parserDependency dependencies [] = {
+		[0] = { DEPTYPE_SUBPARSER, "Perl", &mooseSubparser },
+	};
+	def->dependencies = dependencies;
+	def->dependencyCount = ARRAY_SIZE (dependencies);
+	return def;
+}
+
+extern parserDefinition* MouseParser (void)
+{
+	parserDefinition* const def = perlMxParserNew (MOUSE_NAME);
+	static parserDependency dependencies [] = {
+		[0] = { DEPTYPE_SUBPARSER, "Perl", &mooseSubparser },
+	};
+	def->dependencies = dependencies;
+	def->dependencyCount = ARRAY_SIZE (dependencies);
+	return def;
+}

@EvanCarroll
Copy link

@masatake that looks good. I'm unconvinced of the merits of it but if you like that -- go for it. Do we make the same distinction about API-vs-implementation for things like Iron Python and cPython, or SQL (where the differences are more drastic) like PostgreSQL, SQL Server, and MySQL? It seems like this level of purity (though not without merit) is going to be a LOT of work.

@masatake
Copy link
Member Author

masatake commented Jun 1, 2019

I found very different approach.
The reason I don't like just extending the regex pattern is that it hides whether input file uses Moose or Moo though ctags itself knows which module is used.
At the first time, I've tried to use "language:" field to let client tools know which module is used.
I'm not sure whether the original approach is over engineering or not.
However, I found I should do before using the field.
If perl parser captures modules specified in use built-in function, a client tool can know which module is used in the source file. Introducing module kind solves my concern indirectly.

masatake added a commit to masatake/ctags that referenced this pull request Jun 1, 2019
Close  universal-ctags#2103.
The design discussion was done at universal-ctags#2070.

Signed-off-by: Masatake YAMATO <yamato@redhat.com>
masatake added a commit to masatake/ctags that referenced this pull request Jun 22, 2019
Close  universal-ctags#2103.
The design discussion was done at universal-ctags#2070.

Signed-off-by: Masatake YAMATO <yamato@redhat.com>
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.

Feature Request: Perl Attributes with (has)
2 participants