Re: [Emacs-diffs] emacs-25 504696d: Etags: yet another improvement in Ruby tags

Re: [Emacs-diffs] emacs-25 504696d: Etags: yet another improvement in Ruby tags

[Dmitry Gutov]

On 02/03/2016 07:25 PM, Eli Zaretskii wrote:

> --- a/test/etags/ruby-src/test1.ru
> +++ b/test/etags/ruby-src/test1.ru
> @@ -24,10 +24,14 @@ module A
>       end
>       def X

Just noticed this. attr_X calls will not, as a rule, be inside a method 
definition (which is what 'def X' is).

If an attr_X call is inside a method definition, we're unlikely to be 
able to make much sense of it. Most likely, the arguments will be local 
variables, not Symbol literals. It's also likely that the target of this 
call in that kind of situation won't be the current class.

Anyway, the example shouldn't put attr_X calls inside a method 
definition, or it gives an impression that we handle this situation 
intentionally, or somehow differently from the usual case. Whereas we 
could as well skip those tags altogether (but we don't really have to, 
as long as we only generate non-qualified tags, and check that every 
argument is a Symbol literal, i.e. it starts with a colon).

>         attr_reader :foo
> -      attr_reader :read1, :read2; attr_writer :write1, :write2
> -      attr_writer :bar
> +      attr_reader :read1 , :read2; attr_writer :write1, :write2
> +      attr_writer :bar,
> +                  :baz,
> +                  :more
>         attr_accessor :tee
> -      alias_method :qux, :tee
> +      alias_method :qux, :tee, attr_accessor :bogus

This one is a bit weird as well:

- An alias_method call with three arguments will raise an ArgumentError.

- If it didn't, the 'attr_accessor :bogus' calls would raise a 
SyntaxError, due to evaluation rules. However, an attr_X call can be 
inside an expression, such as:

class C
   puts(attr_accessor :bogus)
end

This is not a typical case, we don't need to handle it, but it's odd to 
see a test case that implies that this example is invalid, and we 
somehow prohibit it. Hopefully, this observation will allow you to 
simplify some code.

Re: [Emacs-diffs] emacs-25 504696d: Etags: yet another improvement in Ruby tags

[Eli Zaretskii]

> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Thu, 4 Feb 2016 02:46:45 +0300
> 
> Just noticed this. attr_X calls will not, as a rule, be inside a method 
> definition (which is what 'def X' is).
> 
> If an attr_X call is inside a method definition, we're unlikely to be 
> able to make much sense of it. Most likely, the arguments will be local 
> variables, not Symbol literals. It's also likely that the target of this 
> call in that kind of situation won't be the current class.
> 
> Anyway, the example shouldn't put attr_X calls inside a method 
> definition, or it gives an impression that we handle this situation 
> intentionally, or somehow differently from the usual case. Whereas we 
> could as well skip those tags altogether (but we don't really have to, 
> as long as we only generate non-qualified tags, and check that every 
> argument is a Symbol literal, i.e. it starts with a colon).

Please modify the test files as you see fit, and tell me what the tags
should be.

> > +      alias_method :qux, :tee, attr_accessor :bogus
> 
> This one is a bit weird as well:
> 
> - An alias_method call with three arguments will raise an ArgumentError.

It's there to test the algorithm, which should not tag the bogus
accessor.

> - If it didn't, the 'attr_accessor :bogus' calls would raise a 
> SyntaxError, due to evaluation rules. However, an attr_X call can be 
> inside an expression, such as:
> 
> class C
>    puts(attr_accessor :bogus)
> end
> 
> This is not a typical case, we don't need to handle it, but it's odd to 
> see a test case that implies that this example is invalid, and we 
> somehow prohibit it. Hopefully, this observation will allow you to 
> simplify some code.

I'm not sure how this simplifies things.  The point was that a comma
doesn't reset the mini-state machine to the state where it is once
again ready to see attr_accessor.  If you are saying there are other
situations like that, please describe them.

IOW, does etags handle the above intentionally invalid code correctly?
It should.

Thanks.

Re: [Emacs-diffs] emacs-25 504696d: Etags: yet another improvement in Ruby tags

[Dmitry Gutov]

On 02/04/2016 06:48 AM, Eli Zaretskii wrote:

>> Anyway, the example shouldn't put attr_X calls inside a method
>> definition, or it gives an impression that we handle this situation
>> intentionally, or somehow differently from the usual case. Whereas we
>> could as well skip those tags altogether (but we don't really have to,
>> as long as we only generate non-qualified tags, and check that every
>> argument is a Symbol literal, i.e. it starts with a colon).
>
> Please modify the test files as you see fit, and tell me what the tags
> should be.

I've updated the example and the tags. No further action needed, thanks.

>>> +      alias_method :qux, :tee, attr_accessor :bogus
>>
>> This one is a bit weird as well:
>>
>> - An alias_method call with three arguments will raise an ArgumentError.
>
> It's there to test the algorithm, which should not tag the bogus
> accessor.

Why is it bogus, though? If it were not a syntax error (because of 
alias_method using a paren-less call as well, not because of preceding 
comma), it would be a valid attr_accessor call.

For instance, these examples are syntactically valid and would result in 
the generation of the method 'foo':

class C
   puts(attr_accessor :foo)
end

class C
   1 + 2; attr_reader :bar
end

We don't really need to support them, but actively fighting against them 
seems odd.

> I'm not sure how this simplifies things.  The point was that a comma
> doesn't reset the mini-state machine to the state where it is once
> again ready to see attr_accessor.  If you are saying there are other
> situations like that, please describe them.
>
> IOW, does etags handle the above intentionally invalid code correctly?
> It should.

Point is, the example code is syntactically invalid. etags doesn't need 
to handle that kind of code at all, right? It would be better to get a 
syntactically-valid example, if we an find one. Not sure if we can; 
these are also invalid:

1, 2, attr_accessor :bogus
puts(1, attr_accessor :bogus)

Ok, so it it doesn't simplify things, don't worry about it.

Re: [Emacs-diffs] emacs-25 504696d: Etags: yet another improvement in Ruby tags

[Eli Zaretskii]

> Cc: emacs-devel@gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Thu, 4 Feb 2016 12:36:25 +0300
> 
> >>> +      alias_method :qux, :tee, attr_accessor :bogus
> >>
> >> This one is a bit weird as well:
> >>
> >> - An alias_method call with three arguments will raise an ArgumentError.
> >
> > It's there to test the algorithm, which should not tag the bogus
> > accessor.
> 
> Why is it bogus, though?

Because its syntax is invalid.

> For instance, these examples are syntactically valid and would result in 
> the generation of the method 'foo':
> 
> class C
>    puts(attr_accessor :foo)
> end
> 
> class C
>    1 + 2; attr_reader :bar
> end
> 
> We don't really need to support them, but actively fighting against them 
> seems odd.

We don't fight them.  The above 2 examples work as expected, please
try them (if you didn't already).

The example which is in the test file specifically tests the handling
of stuff that follows a comma after alias_method, since that requires
some logic I wanted to be sure I got right.

> Point is, the example code is syntactically invalid. etags doesn't need 
> to handle that kind of code at all, right?

No, but I wanted to be sure the invalid code doesn't adversely affect
valid code further in the file.

> It would be better to get a syntactically-valid example, if we an
> find one.

If you can, by all means.  As I don't speak Ruby, I just went with the
simplest one I could throw together.

Re: [Emacs-diffs] emacs-25 504696d: Etags: yet another improvement in Ruby tags

[Dmitry Gutov]

On 02/04/2016 08:28 PM, Eli Zaretskii wrote:

>> class C
>>     puts(attr_accessor :foo)
>> end
>>
>> class C
>>     1 + 2; attr_reader :bar
>> end
>>
>> We don't really need to support them, but actively fighting against them
>> seems odd.
>
> We don't fight them.  The above 2 examples work as expected, please
> try them (if you didn't already).

Tried them now. They do work! Thanks.

>> Point is, the example code is syntactically invalid. etags doesn't need
>> to handle that kind of code at all, right?
>
> No, but I wanted to be sure the invalid code doesn't adversely affect
> valid code further in the file.

Fair enough.

> If you can, by all means.  As I don't speak Ruby, I just went with the
> simplest one I could throw together.

Would

     alias_method :qux, :tee, :bogus

trigger the same problem, and use the same fix?

If not, please disregard this email. The code works well, and supports 
all the main use cases.

Re: [Emacs-diffs] emacs-25 504696d: Etags: yet another improvement in Ruby tags

[Dmitry Gutov]

On 02/05/2016 08:26 AM, Dmitry Gutov wrote:

> Would
>
>      alias_method :qux, :tee, :bogus
>
> trigger the same problem, and use the same fix?

or

alias_method :qux, :tee, foo(1, 2, 3)

?

Re: [Emacs-diffs] emacs-25 504696d: Etags: yet another improvement in Ruby tags

[Eli Zaretskii]

> Cc: emacs-devel@gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Fri, 5 Feb 2016 08:26:53 +0300
> 
> Would
> 
>      alias_method :qux, :tee, :bogus
> 
> trigger the same problem, and use the same fix?

No.  What comes after the comma must begin with attr_SOMETHING or
alias_method.  The issue being tested here is that we are not in a
state where matches for these are being tried.

But if you ever figure out how to do that with a less abnormal syntax,
feel free to update the test files.

It could also be a good idea to add a Rakefile or a Thorfile to the
ruby-src directory (when I tested the change, I just renamed one of
the other files).  It could be that those present special challenges,
and in any case we should test the file-name rules.

> If not, please disregard this email. The code works well, and supports 
> all the main use cases.

OK, thanks.  I guess we now have the "best etags in the West" (and in
the East as well), as far as Ruby support is concerned.

Re: [Emacs-diffs] emacs-25 504696d: Etags: yet another improvement in Ruby tags

[Dmitry Gutov]

On 02/05/2016 12:14 PM, Eli Zaretskii wrote:

> No.  What comes after the comma must begin with attr_SOMETHING or
> alias_method.  The issue being tested here is that we are not in a
> state where matches for these are being tried.

     alias_method :qux, :tee, attr_accessor(:bogus)

or

     alias_method :qux, :tee, alias_method(:bogus, :bogus2)

are the main options, I suppose. But they might also look misleading, 
and indicate that we don't support the paren-using syntax intentionally.

(It's another omission, but AFAICS nobody uses attr_XXX without parens 
in the context we're interested in.)

> But if you ever figure out how to do that with a less abnormal syntax,
> feel free to update the test files.

The problem with me updating the tests is I can't revert the 
corresponding fix and make sure that the test fails without it.

> It could also be a good idea to add a Rakefile or a Thorfile to the
> ruby-src directory (when I tested the change, I just renamed one of
> the other files).  It could be that those present special challenges,
> and in any case we should test the file-name rules.

I believe the file-name rules should be tested in a language-agnostic 
way, or just with one language. There's not much point in having all 
possible file names in test/etags. Or at least that's how I usually try 
to write tests: in as orthogonal way as possible.

> OK, thanks.  I guess we now have the "best etags in the West" (and in
> the East as well), as far as Ruby support is concerned.

You could say so.

It's definitely better than "Exuberant Ctags 5.9~svn20110310" I have 
installed through the default Ubuntu repositories.

It's better [0] than "Ctag Revival" [1], at least until they add support 
for qualified tags for Ruby [2], which could take a while.

We're probably better in some things, and worse in others, than "Ripper 
Tags" [3]. I haven't tried them yet myself.

In any case, it'll give me something to try using day-to-day, when we 
have at least some solution for tags being out of date (an experimental 
xref backend that simply rescans the whole project every time comes to 
mind).

[0] https://github.com/universal-ctags/ctags/issues/408
[1] https://github.com/universal-ctags/ctags
[2] https://github.com/universal-ctags/ctags/issues/524
[3] https://github.com/tmm1/ripper-tags/

Re: [Emacs-diffs] emacs-25 504696d: Etags: yet another improvement in Ruby tags

[Eli Zaretskii]

> Cc: emacs-devel@gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Fri, 5 Feb 2016 13:11:50 +0300
> 
> On 02/05/2016 12:14 PM, Eli Zaretskii wrote:
> 
> > No.  What comes after the comma must begin with attr_SOMETHING or
> > alias_method.  The issue being tested here is that we are not in a
> > state where matches for these are being tried.
> 
>      alias_method :qux, :tee, attr_accessor(:bogus)
> 
> or
> 
>      alias_method :qux, :tee, alias_method(:bogus, :bogus2)
> 
> are the main options, I suppose.

These should work for my purposes.

> But they might also look misleading, and indicate that we don't
> support the paren-using syntax intentionally.
> 
> (It's another omission, but AFAICS nobody uses attr_XXX without parens 
> in the context we're interested in.)

If it's important to support that, it should be hard to add.

> > But if you ever figure out how to do that with a less abnormal syntax,
> > feel free to update the test files.
> 
> The problem with me updating the tests is I can't revert the 
> corresponding fix and make sure that the test fails without it.

You can always leave that to me.

> > It could also be a good idea to add a Rakefile or a Thorfile to the
> > ruby-src directory (when I tested the change, I just renamed one of
> > the other files).  It could be that those present special challenges,
> > and in any case we should test the file-name rules.
> 
> I believe the file-name rules should be tested in a language-agnostic 
> way, or just with one language.

I don't see how: the file names that trigger recognition as a specific
language are part of the language-specific rules.  IOW, when etags
sees a file whose basename is "Rakefile", it should process it as a
Ruby file, and how can you test it does that without actually looking
at the tags it produces for that file?  E.g., the bug I fixed in
f6213ce caused Makefile's to be processed as Fortran sources...

> We're probably better in some things, and worse in others, than "Ripper 
> Tags" [3]. I haven't tried them yet myself.

Maybe etags should acquire a feature whereby it could run external
programs to help it find the tags.  Something to think of for future
projects, perhaps.

Re: [Emacs-diffs] emacs-25 504696d: Etags: yet another improvement in Ruby tags

[Eli Zaretskii]

> Date: Fri, 05 Feb 2016 13:15:45 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: emacs-devel@gnu.org
> 
> > But they might also look misleading, and indicate that we don't
> > support the paren-using syntax intentionally.
> > 
> > (It's another omission, but AFAICS nobody uses attr_XXX without parens 
> > in the context we're interested in.)
> 
> If it's important to support that, it should be hard to add.
                                        ^^^^^^
I meant "shouldn't", of course.

Sorry.

Re: [Emacs-diffs] emacs-25 504696d: Etags: yet another improvement in Ruby tags

[Dmitry Gutov]

On 02/05/2016 02:15 PM, Eli Zaretskii wrote:

>>       alias_method :qux, :tee, attr_accessor(:bogus)
>>
>> or
>>
>>       alias_method :qux, :tee, alias_method(:bogus, :bogus2)
>>
>> are the main options, I suppose.
>
> These should work for my purposes.

Great!

>> But they might also look misleading, and indicate that we don't
>> support the paren-using syntax intentionally.
>>
>> (It's another omission, but AFAICS nobody uses attr_XXX without parens
>> in the context we're interested in.)
>
> If it's important to support that, it should be hard to add.

I don't know how important it is, and I didn't want to bother you, but 
it would be nice to have, for completeness. (See debbugs.gnu.org/22563)

We, the Emacs community, have our share of eccentrics who won't hesitate 
to write code in an unusual way, and then be surprised anyway if some 
things don't work.

>> I believe the file-name rules should be tested in a language-agnostic
>> way, or just with one language.
>
> I don't see how: the file names that trigger recognition as a specific
> language are part of the language-specific rules.

I'd think we could mostly rely on the fact that the general facility 
works, and you haven't made a typo in Ruby_filenames or Ruby_suffixes.

Creating a bunch of files, and complicating etags's output just for 
that, seems like an overkill.

 > IOW, when etags
 > sees a file whose basename is "Rakefile", it should process it as a
 > Ruby file, and how can you test it does that without actually looking
 > at the tags it produces for that file?

If you could write unit tests for etags code, you could have some for 
the "detect language" function. Full-on integration tests, again, seem 
like an overkill to me.

 > E.g., the bug I fixed in
 > f6213ce caused Makefile's to be processed as Fortran sources...

That one could be handled by a language-agnostic regression test, I 
think. Or just test that for Makefile, but not worry about other languages.

Of course, I'm just speaking from my own experience, and could be 
discounting how error-prone and/or inconsistent C is.

>> We're probably better in some things, and worse in others, than "Ripper
>> Tags" [3]. I haven't tried them yet myself.
>
> Maybe etags should acquire a feature whereby it could run external
> programs to help it find the tags.  Something to think of for future
> projects, perhaps.

Not sure how ripper-tags would benefit from it (they can output in etags 
format already), but you can look at 
https://github.com/universal-ctags/ctags/blob/master/docs/xcmd.rst for 
inspiration.

Re: [Emacs-diffs] emacs-25 504696d: Etags: yet another improvement in Ruby tags

[Eli Zaretskii]

> Cc: emacs-devel@gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Fri, 5 Feb 2016 15:15:06 +0300
> 
> > I don't see how: the file names that trigger recognition as a specific
> > language are part of the language-specific rules.
> 
> I'd think we could mostly rely on the fact that the general facility 
> works, and you haven't made a typo in Ruby_filenames or Ruby_suffixes.

Well, I almost surrendered to relying on such stuff when I added
"Rakefile" and "Thorfile", and almost expected to see the same boring
output when I tried that.  It didn't, which is how the bug fixed in
f6213ce was discovered.  Which fits my general experience: there are
no tests one can dismiss, every good test will always find some
problem.

> If you could write unit tests for etags code, you could have some for 
> the "detect language" function. Full-on integration tests, again, seem 
> like an overkill to me.

I could agree with that, but we are a long way from having a unit test
suite for Emacs.

Re: [Emacs-diffs] emacs-25 504696d: Etags: yet another improvement in Ruby tags

[Eli Zaretskii]

> Cc: emacs-devel@gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Fri, 5 Feb 2016 15:15:06 +0300
> 
> On 02/05/2016 02:15 PM, Eli Zaretskii wrote:
> 
> >>       alias_method :qux, :tee, attr_accessor(:bogus)
> >>
> >> or
> >>
> >>       alias_method :qux, :tee, alias_method(:bogus, :bogus2)
> >>
> >> are the main options, I suppose.
> >
> > These should work for my purposes.
> 
> Great!

Used the former in the test file.