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

Add support for Vala (finish work by Alberto Fanjul) #2661

Closed
wants to merge 7 commits into from

Conversation

rrthomas
Copy link
Contributor

@rrthomas rrthomas commented Oct 8, 2020

@coveralls
Copy link

coveralls commented Oct 8, 2020

Coverage Status

Coverage decreased (-0.001%) to 87.006% when pulling 5eb210d on rrthomas:vala into 952bd1e on universal-ctags:master.

@codecov
Copy link

codecov bot commented Oct 8, 2020

Codecov Report

Merging #2661 into master will decrease coverage by 0.00%.
The diff coverage is 86.16%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2661      +/-   ##
==========================================
- Coverage   86.91%   86.91%   -0.01%     
==========================================
  Files         183      184       +1     
  Lines       39018    39238     +220     
==========================================
+ Hits        33912    34102     +190     
- Misses       5106     5136      +30     
Impacted Files Coverage Δ
parsers/vala.c 86.16% <86.16%> (ø)
parsers/verilog.c 98.96% <0.00%> (+0.16%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 952bd1e...5eb210d. Read the comment docs.

@masatake masatake self-requested a review October 8, 2020 15:10
@masatake
Copy link
Member

masatake commented Oct 8, 2020

Please update win32/ctags_vs2013.vcxproj.filters .
Please add Vala to docs/NEWS.rst .

@masatake
Copy link
Member

masatake commented Oct 8, 2020

Though I don't know Vala wll but I think your evalation of the parser may not enough.

/* Speaker: extends from GObject */
interface Speaker : Object {
  /* speak: abstract without a body */
  public abstract void speak ();
}

/* Fox: implements Speaker, implements speak () */
class Fox : Object, Speaker {
  public void speak () {
    stdout.printf ("  Fox says Ow-wow-wow-wow\n");
  }
}

For the input, ctags repots only speak in the interface.

$ ./ctags --options=NONE -o - input.vala 
ctags: Notice: No options will be read from files or environment
speak	input.vala	/^  public abstract void speak ();$/;"	m

@rrthomas
Copy link
Contributor Author

rrthomas commented Oct 8, 2020

Thanks for the new test case, I'll have a look. In any case, I'm not worried about trying to make it perfect.

I had a look at anjuta-tags, the ctags fork with Vala support for the Anjuta IDE. Here's the Vala parser: https://gitlab.gnome.org/GNOME/anjuta/-/blob/master/plugins/symbol-db/anjuta-tags/vala.c

I see why you weren't able to include it, as it is mostly written in Vala. By using the Vala parser directly I guess it will provide accurate results. So, I may be able to use it as a comparison. In any case, that makes me think that it is better simply to have reasonable Vala support in universal-ctags, without worrying too much about making it perfect the first time.

@masatake
Copy link
Member

masatake commented Oct 9, 2020

The class Fox is not extracted. I guess "class" is an important concept in Vala parser. I wonder who wants to use the parser.
I do not and cannot say a parser must be perfect (when we merge it). However, it doesn't mean we should merge a parser even if it is broken.

I would like you who knows Vala to take a bit more time for evalation.

This comment is applicable to myself, too. The Ansibleplaybook parser that I wrote is broken. I should not merge it.

@masatake masatake mentioned this pull request Oct 9, 2020
masatake and others added 5 commits October 10, 2020 12:45
Fix a misleading comment.

Fix incorrect spacing of close brace.

Remove unnecessary conversion to bool.

Add an “else” before a mutually-exclusive “if”.
Attributes, in matched square brackets, should be ignored, as otherwise they
upset parsing.

Add a test.
signature fields are tested.
@rrthomas
Copy link
Contributor Author

I tried your test case with anjuta-tags, and indeed it gets several other results:

Fox	input.vala	8;"	c
Speaker	input.vala	2;"	i
speak	input.vala	4;"	m	interface:Speaker
speak	input.vala	9;"	m	class:Fox

I'll see what I can do about that in universal-ctags.

@rrthomas
Copy link
Contributor Author

I made some improvements! I'm not satisfied yet, but I would be interested in your opinion on my changes so far.

My method has been to compare with the output of anjuta-tags, which as you know uses the actual Vala compiler's parser.

One of the reasons that I stopped is that I realized that logically, parseClassBody in vala.c should call recurseValaTags (because a class can contain classes), and it doesn't. Is there some reason for this?

@masatake
Copy link
Member

One of the reasons that I stopped is that I realized that logically, parseClassBody in vala.c should call recurseValaTags (because a class can contain classes), and it doesn't. Is there some reason for this?

The original version which your work is based is just incompleted. So the reason may be just "no resource and no time."

@@ -12,12 +12,12 @@ void main(string[] args) {
print("Hello %s, you're %d years old\n", p.name, p.age);
}

class Address {
public class Address {
Copy link
Member

Choose a reason for hiding this comment

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

FYI, you can store "public" to "access:" field.

[yamato@control ctags-github]$ cat /tmp/foo.cc
cat /tmp/foo.cc
class A {
public:
  int x;
};
[yamato@control ctags-github]$ ./ctags -o - --fields=+'{access}' /tmp/foo.cc
./ctags -o - --fields=+'{access}' /tmp/foo.cc
A	/tmp/foo.cc	/^class A {$/;"	c	language:C++	file:
x	/tmp/foo.cc	/^  int x;$/;"	m	language:C++	class:A	typeref:typename:int	file:	access:public

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the hint, I did wonder about this.

@rrthomas
Copy link
Contributor Author

rrthomas commented Oct 13, 2020

One of the reasons that I stopped is that I realized that logically, parseClassBody in vala.c should call recurseValaTags (because a class can contain classes), and it doesn't. Is there some reason for this?

The original version which your work is based is just incompleted. So the reason may be just "no resource and no time."

Ah, OK. I saw from the original discussion that you helped a lot, and provided the basis, so I thought there might be a reason! Since there's not, I will go ahead and try to make the recursion.

It seems that even in etags mode, the output of anjuta-tags does not work well with Emacs, which keeps complaining that it cannot find definitions, whereas the output of universal-ctags, even with the buggy parser, works fine. So I would very much like to get a Vala parser merged!

@masatake
Copy link
Member

I made some improvements! I'm not satisfied yet, but I would be interested in your opinion on my changes so far.

Good. I think we can record the information about generics as C++ parser does:

$ cat /tmp/foo.cc
template <typename G> class A {
public:
  G x;
};
$ ./ctags -o - --kinds-C++='*' --fields=+'{access}' --fields-C++='*' /tmp/foo.cc
A	/tmp/foo.cc	/^template <typename G> class A {$/;"	c	language:C++	file:	template:<typename G>
G	/tmp/foo.cc	/^template <typename G> class A {$/;"	Z	language:C++	class:A	typeref:meta:typename	access:private
x	/tmp/foo.cc	/^  G x;$/;"	m	language:C++	class:A	typeref:typename:G	file:	access:public

My method has been to compare with the output of anjuta-tags, which as you know uses the actual Vala compiler's parser.

I don't say the approach is bad. However, Making the Vala parser of u-ctags the same as that of anjuta-tags is not our goal.

As a Vala programmer, do you think ctags extracts what you expect with proper kinds?

This is the most important question.

/* Speaker: extends from GObject */
interface Speaker : Object {
  /* speak: abstract without a body */
  public abstract void speak ();
}

/* Fox: implements Speaker, implements speak () */
class Fox : Object, Speaker {
  public void speak () {
    stdout.printf ("  Fox says Ow-wow-wow-wow\n");
  }
}

I don't know Vala, but I thought "Speaker" should be tagged with "interface" with my knowledge about some other OOP languages.

Writing down ideal tags files (expected tags file) helps you design a parser.
For the input, my expectations are:

Speaker	input.vala	/^interface Speaker/;"	interface	inherits:Object
speak	input.vala	/^  public abstract void speak/;"	methoddecl	scope:interface:Speaker	access:public	typeref:typename:void	signature:()	properties:abstract
Fox	input.vala	/^class Fox/;"	class	inherits:Object,Speaker
speak	input.vala	/^  public void speak/;"	method	scope:class:Fox	access:public	typeref:typename:void	signature:()

(I wonder how "abstract" should be recorded.)
I don't say we must implement the parser as it can emit the ideal tags file.
However, I would like you to think about what parts of the ideal tags file you want to implement at least.

@masatake
Copy link
Member

It seems that even in etags mode, the output of anjuta-tags does not work well with Emacs, which keeps complaining that it cannot find definitions, whereas the output of universal-ctags, even with the buggy parser, works fine. So I would very much like to get a Vala parser merged!

What kind of .el for utilizing TAGS output? just xref?

I think emacs should have xref-ctags.el. I believe you understand what I mean. If you are good at emacs-lisp, and have an interest in native tags file support in GNU Emacs, could you open a new issue for the topic? I'm talking about contributing to GNU Emacs.
I would like to use xref-ctags.el just after doing "yum install emacs".

I have worked on u-ctags for years. However, I myself cannot enjoy our fruits of the efforts on my favorite editor, GNU Emacs.
If you lead the development of xref-ctags.el, I will fishih the vala parser instead.

@rrthomas
Copy link
Contributor Author

Yes, I am just using xref. I had the same idea, I think it would be an excellent idea to make Emacs support ctags, rather than require its own format. I would be very happy to look into that and try to develop xref-ctags.el.

@masatake
Copy link
Member

The code-exchange contract is concluded!

@masatake
Copy link
Member

I take over this Vala parser development in #2677.

@masatake masatake closed this Oct 25, 2020
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.

4 participants