Re: trunk r114534: Get Ruby's SMIE code to pass the test suite.

Re: trunk r114534: Get Ruby's SMIE code to pass the test suite.

[Dmitry Gutov]

Stefan Monnier <monnier@iro.umontreal.ca> writes:
> ------------------------------------------------------------
> revno: 114534
> revision-id: monnier@iro.umontreal.ca-20131005183708-jfv7jyagzpkqkljx
> parent: dgutov@yandex.ru-20131005172122-cpmqj9dmeknecra6
> committer: Stefan Monnier <monnier@iro.umontreal.ca>
> branch nick: trunk
> timestamp: Sat 2013-10-05 14:37:08 -0400
> message:
>   Get Ruby's SMIE code to pass the test suite.
>   * lisp/progmodes/ruby-mode.el (ruby-use-smie): Change default.

Cool stuff.

> === modified file 'test/ChangeLog'
> --- a/test/ChangeLog  2013-10-04 21:45:37 +0000
> +++ b/test/ChangeLog  2013-10-05 18:37:08 +0000
> @@ -1,3 +1,8 @@
> +2013-10-05  Stefan Monnier  <monnier@iro.umontreal.ca>
> +
> +     * indent/ruby.rb: Port a few cases from automated/ruby-mode-tests.el.
> +     Adjust indentation of continued line to the new SMIE behavior.

I'm slightly concerned that this makes those tests "fail" in the default
engine. Shouldn't be too much of a problem, though, if SMIE works well.

> +
>  2013-10-04  Stefan Monnier  <monnier@iro.umontreal.ca>
>
>       * automated/completion-tests.el:
>
> === modified file 'test/indent/ruby.rb'
> --- a/test/indent/ruby.rb     2013-09-15 23:42:26 +0000
> +++ b/test/indent/ruby.rb     2013-10-05 18:37:08 +0000
> @@ -1,3 +1,10 @@
> +if something_wrong?             # ruby-move-to-block-skips-heredoc
> +  ActiveSupport::Deprecation.warn(<<-eowarn)
> +  boo hoo
> +  end
> +  eowarn
> +end

This "port" omits a significant part of the test. Specifically, its
automation. Do you intend to eventually remove it from
automated/ruby-mode-tests.el? I'd be firmly against that.

Otherwise, duplicating examples from there in this file, as a playground
of sorts, seems harmless.

> +foo = [1,                       # ruby-deep-indent
> +       2]
> +
> +foo = {                         # ruby-deep-indent-disabled
> +  a : b
> +}
> +
> +foo = [                         # ruby-deep-indent-disabled
> +  1
> +]
> +
> +foo(                            # ruby-deep-indent-disabled
> +  a
> +)

Being able to handle all the cases above without touching
`ruby-deep-indent' is nice.  I've been using an advice for
`ruby-indent-line' which does that, for quite some time.

>  # Bug#15369
>  MSG = 'Separate every 3 digits in the integer portion of a number' \
> -  'with underscores(_).'
> +      'with underscores(_).'

This one doesn't work for me. With SMIE, it indents like this:

MSG = 'Separate every 3 digits in the integer portion of a number' \
                                                                   'with underscores(_).'

Re: trunk r114534: Get Ruby's SMIE code to pass the test suite.

[Stefan Monnier]

>> +     * indent/ruby.rb: Port a few cases from automated/ruby-mode-tests.el.
>> +     Adjust indentation of continued line to the new SMIE behavior.
> I'm slightly concerned that this makes those tests "fail" in the default
> engine.

It's only one test.  And yes, the two indentation styles are different.
I obviously consider SMIE's to be superior, which is why I changed the
ruby.rb file, but I haven't bothered to fix the old engine (not
"default" any more).

> Shouldn't be too much of a problem, though, if SMIE works well.

That's the hope, indeed.

> automation. Do you intend to eventually remove it from
> automated/ruby-mode-tests.el? I'd be firmly against that.

Knowing that it's a sensitive issue, I just stay away from
ruby-mode-tests.el.

> This one doesn't work for me. With SMIE, it indents like this:

> MSG = 'Separate every 3 digits in the integer portion of a number' \
>                                                                    'with underscores(_).'

I installed a fix to smie.el at the same time.  The indentation you show
is the one you get with the old smie.el.


        Stefan

Re: trunk r114534: Get Ruby's SMIE code to pass the test suite.

[Dmitry Gutov]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> It's only one test.

If you mean the example for Bug#15369, you're forgetting the
ruby-deep-indent-* examples, they can't all pass using the old engine,
with the same value of ruby-deep-indent-paren.

> old engine (not "default" any more).

Indeed.

>> automation. Do you intend to eventually remove it from
>> automated/ruby-mode-tests.el? I'd be firmly against that.
>
> Knowing that it's a sensitive issue, I just stay away from
> ruby-mode-tests.el.

Ok, thank you.

>> This one doesn't work for me. With SMIE, it indents like this:
>
>> MSG = 'Separate every 3 digits in the integer portion of a number' \
>>                                                                    'with underscores(_).'
>
> I installed a fix to smie.el at the same time.  The indentation you show
> is the one you get with the old smie.el.

Right, sorry. My smie.elc was stale.

Could you take a look at the examples I added in 114537 and 114538?

They're fairly characterisitc of real-life code (except the optional
"do", I guess).

Re: trunk r114534: Get Ruby's SMIE code to pass the test suite.

[Stefan Monnier]

> Could you take a look at the examples I added in 114537 and 114538?

See my answer patch.  You'll see that I only added support for - and +.
Other infix operators are easy to add, but I'd rather someone go through
the trouble of finding the precedence table (BTW, you could also use the
trick used in sml-mode, where the infix operators are not specified in
the BNF grammar but only in a precedence table).


        Stefan

Re: trunk r114534: Get Ruby's SMIE code to pass the test suite.

[Dmitry Gutov]

On 07.10.2013 08:05, Stefan Monnier wrote:
>> Could you take a look at the examples I added in 114537 and 114538?
>
> See my answer patch.  You'll see that I only added support for - and +.
> Other infix operators are easy to add, but I'd rather someone go through
> the trouble of finding the precedence table (BTW, you could also use the
> trick used in sml-mode, where the infix operators are not specified in
> the BNF grammar but only in a precedence table).

Thank you, done, in 114732. Please review when you have the time.

Some further questions:

1) Is `assoc' any different from `left' in practice?

2) `ruby-smie-grammar' has a FIXME at the top. What kind of Cucumber 
support do you suppose is missing? The "Given /toto/ do" example in 
ruby.rb looks fine, mostly due to the syntax-propertize-function, which 
hasn't changed.

3) Please take a look at the following example:

foo_bar_tee(1, 2, 3)
   .qux

What would be the best way to make it work (and also similar example 
with the dot on the first line)?

Add another two cases to `ruby-smie-rules', for the token "."? That 
wouldn't exactly, I think, work because `ruby-smie--forward-token' and 
its counterpart like to concatenate the dot with the identifier that 
goes after it.

Re: trunk r114534: Get Ruby's SMIE code to pass the test suite.

[Stefan Monnier]

> 1) Is `assoc' any different from `left' in practice?

Yes: with `assoc', when you have "a + b + c", smie first tries to align
"c" with "b", whereas with `left' it would skip "b" and go straight to
"a".  When there are only 3 elements, it rarely makes a difference, but
for things like "," or ";" where there can be tens or hundreds of
elements, the different can be very significant in terms of
indentation speed.  Also, it makes a difference if the user purposefully
"misindents" some of the elements, of course.

> 2) `ruby-smie-grammar' has a FIXME at the top. What kind of Cucumber support
> do you suppose is missing? The "Given /toto/ do" example in ruby.rb looks
> fine, mostly due to the syntax-propertize-function, which hasn't changed.

That fixme migth be out of date, indeed.

> 3) Please take a look at the following example:
> foo_bar_tee(1, 2, 3)
>   .qux
> What would be the best way to make it work (and also similar example with
> the dot on the first line)?
> Add another two cases to `ruby-smie-rules', for the token "."? That wouldn't
> exactly, I think, work because `ruby-smie--forward-token' and its
> counterpart like to concatenate the dot with the identifier that goes
> after it.

Maybe change the tokenizer so that a ".qux" *at smie-bolp* is tokenized
as "." and "qux"?


        Stefan

Re: trunk r114534: Get Ruby's SMIE code to pass the test suite.

[Dmitry Gutov]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> 1) Is `assoc' any different from `left' in practice?
>
> Yes: with `assoc', when you have "a + b + c", smie first tries to align
> "c" with "b", whereas with `left' it would skip "b" and go straight to
> "a".  When there are only 3 elements, it rarely makes a difference, but
> for things like "," or ";" where there can be tens or hundreds of
> elements, the different can be very significant in terms of
> indentation speed.  Also, it makes a difference if the user purposefully
> "misindents" some of the elements, of course.

I see.

> Maybe change the tokenizer so that a ".qux" *at smie-bolp* is tokenized
> as "." and "qux"?

That will probably break the following modified example:

class C
  def foo
    self
      .end
    D.new
     .class
  end
end

But I guess we can make the tokenizer both return "." as a separate
token and include it in the token that goes after it.

Re: trunk r114534: Get Ruby's SMIE code to pass the test suite.

[Stefan Monnier]

>> Maybe change the tokenizer so that a ".qux" *at smie-bolp* is tokenized
>> as "." and "qux"?
> That will probably break the following modified example:
> class C
>   def foo
>     self
>       .end
>     D.new
>      .class
>   end
> end

Yes, the problem is that the set of desired indentation does not match
the structure of a fixed parsing.  So, some of the differences need to
be handled in ad-hoc ways in the ruby-smie-rules function.

E.g. for (:before . ".") we'd have to look at the previous token and if
it's of the form "foo.bar", then manually align with the "." of
"foo.bar".

> But I guess we can make the tokenizer both return "." as a separate
> token and include it in the token that goes after it.

I'm not sure I understand exactly what you're suggesting, but I'd
venture to say that you can't do that.


        Stefan

Re: trunk r114534: Get Ruby's SMIE code to pass the test suite.

[Dmitry Gutov]

On 21.10.2013 20:46, Stefan Monnier wrote:
>>> Maybe change the tokenizer so that a ".qux" *at smie-bolp* is tokenized
>>> as "." and "qux"?
>> That will probably break the following modified example:
>> class C
>>    def foo
>>      self
>>        .end
>>      D.new
>>       .class
>>    end
>> end
>
> Yes, the problem is that the set of desired indentation does not match
> the structure of a fixed parsing.  So, some of the differences need to
> be handled in ad-hoc ways in the ruby-smie-rules function.
>
> E.g. for (:before . ".") we'd have to look at the previous token and if
> it's of the form "foo.bar", then manually align with the "." of
> "foo.bar".

Indentation is not the only problem there. Tokenizing ".end" as [".", 
"end"] also breaks sexp movement and (for example) 
`smie--matching-block-data'. The dot concatenation logic is the result 
of you fixing that in revision 114545.

>> But I guess we can make the tokenizer both return "." as a separate
>> token and include it in the token that goes after it.
>
> I'm not sure I understand exactly what you're suggesting, but I'd
> venture to say that you can't do that.

I don't see why not. I've installed a patch along these lines in 114738, 
and it fixes the existing examples. Unfortunately, it broke the more 
complex situations:

foo do
   bar
     .tee
end

def bar
   foo
     .baz
end

...apparently because (goto-char (cadr (smie-indent--parent))), when 
called before "." in these examples, brings point to the end of the 
first line, and so (smie-rule-parent ruby-indent-level) returns a 
surprising result.

Any suggestions what to do about that?

Re: trunk r114534: Get Ruby's SMIE code to pass the test suite.

[Stefan Monnier]

> I don't see why not. I've installed a patch along these lines in 114738, and
> it fixes the existing examples. Unfortunately, it broke the more complex
> situations:

> foo do
>   bar
>     .tee
> end

> def bar
>   foo
>     .baz
> end

Oh, that was a mistake of mine: the `(:after . ";") should have been
`(:before . ";").

I installed that patch in trunk.


        Stefan

Re: trunk r114534: Get Ruby's SMIE code to pass the test suite.

[Stefan Monnier]

> Indentation is not the only problem there. Tokenizing ".end" as [".", "end"]

I wasn't suggesting that.  We'd turn ".end" into something like [".", ".end"]
or [".", "identifier"].


        Stefan