Feature #6265

Remove 'useless' 'concatenation' syntax

Added by Rodrigo Rosenfeld Rosas about 2 years ago. Updated about 1 year ago.

[ruby-core:44156]
Status:Assigned
Priority:Normal
Assignee:Yusuke Endoh
Category:core
Target version:next minor

Description

What is wrong with this code:

some_method 'argument1', 'argument2' 'argument3'

Yes, the missing colon, but it is not always easy to notice that...

What is this ('concatenation' 'syntax') useful for?

Why writing ('some ' 'concatenation') instead of 'some concatenation'?

A missing colon between string arguments can lead to some bugs that may be hard to find, specially if the arguments are optional.

And I can't see any useful case where this allowed syntax for concatenation would help.

Associated revisions

Revision 37317
Added by Usaku NAKAMURA over 1 year ago

  • parse.y: show a warning for concatenating string literals because it will be deprecated in the future. patched by mame (Yusuke Endoh) at . [Feature #6265]

Revision 37327
Added by Yui NARUSE over 1 year ago

Revert r37316

The commit introduces too many failures and disturbs release engineering.
Re-commit it with fixed tests.

Thu Oct 25 13:09:01 2012 NAKAMURA Usaku usa@ruby-lang.org

   * parse.y: show a warning for concatenating string literals because
     it will be deprecated in the future.
     patched by mame (Yusuke Endoh) at .
      [Feature #6265]

Revision 38230
Added by Eric Hodel over 1 year ago

  • lib/rubygems/command_manager.rb: Removed string concatenation syntax. [Bug #6265]
  • lib/rubygems/commands/install_command.rb: ditto
  • lib/rubygems/commands/uninstall_command.rb: ditto
  • lib/rubygems/indexer.rb: ditto
  • lib/rubygems/security/policy.rb: ditto
  • lib/rubygems/security.rb: ditto
  • lib/rubygems/uninstaller.rb: ditto
  • test/rubygems/testgemcommandscertcommand.rb: ditto
  • test/rubygems/testgempackage.rb: ditto
  • test/rubygems/testgemsecurity.rb: ditto
  • test/rubygems/testgemsecurity_policy.rb: ditto

History

#1 Updated by Nikolai Weibull about 2 years ago

On Fri, Apr 6, 2012 at 14:53, rosenfeld (Rodrigo Rosenfeld Rosas)
rr.rosas@gmail.com wrote:

What is this ('concatenation' 'syntax') useful for?

raise ArgumentError,
'they are useful for long strings that are so long that they need ' \
'to span multiple lines to fit within a reasonable line width'

#2 Updated by Marc-Andre Lafortune about 2 years ago

  • Target version changed from Next Major to 2.0.0

Hi,

now (Nikolai Weibull) wrote:

On Fri, Apr 6, 2012 at 14:53, rosenfeld (Rodrigo Rosenfeld Rosas)
rr.rosas@gmail.com wrote:

What is this ('concatenation' 'syntax') useful for?

raise ArgumentError,
'they are useful for long strings that are so long that they need ' \
'to span multiple lines to fit within a reasonable line width'

In this example, replacing the "\" with a "+" will give basically the same result without using the concatenation rule.

Are there valid use cases where two strings are concatenated on the same line? I think that's really what the request is all about, i.e. forbid that kind of concatenation for two strings on the same line.

#3 Updated by Yusuke Endoh about 2 years ago

  • Status changed from Open to Assigned
  • Target version changed from 2.0.0 to Next Major

msg = 'put "\n" here -> ' "\n"

I found an actual example in lib/mkmf.rb:

hdr = ['#include "ruby.h"' "\n"]

Anyway, 2.0 should remain all existing features unless there is a good reason.

Yusuke Endoh mame@tsg.ne.jp

#4 Updated by Rodrigo Rosenfeld Rosas about 2 years ago

I found an actual example in lib/mkmf.rb:

hdr = ['#include "ruby.h"' "\n"]

Wouldn't this read better this way?

hdr = [%Q{#include "ruby.h"\n}]

#5 Updated by Yusuke Endoh about 2 years ago

=begin

I don't think the current code in mkmf.rb is good.
I just meant that the syntax is actually used and thus we cannot remove it from 2.0.
I'm not against this proposal in the future.

$ cat concatquine.rb
eval$s="s='eval$s='+$s.split.join.dump;('%0382b'%(('40i3tgbj1q4n'+
'" "n3" "t7" "1py" "1ur" "h'" "+
'" "xtb74az" "4" "ym" "2" "03" "fx542i6" "e" "udoj" "').#
t" "o
i(36)" "<" "<7" "0" "))" ".gsub(/" "0" "(?=1" ")|xx
|" "1(?=0)/" ")" "{$" "&" "<<" "34}.tr(" "'1'," "''<<
3" "2)" ".g" "s" "ub" "('" "0" "'){;" ";;s.
slice!(0,1)}.scan(/.{66}/){puts$&}#CONCATQUINE(c)Y.Endoh_2012"

$ ruby concatquine.rb
eval$s="s='eval$s='+$s.split.join.dump;('%0382b'%(('40i3tgbj1q4n'+
'" "n3" "t7" "1py" "1ur" "h'" "+
'" "xtb74az" "4" "ym" "2" "03" "fx542i6" "e" "udoj" "').#
t" "o
i(36)" "<" "<7" "0" "))" ".gsub(/" "0" "(?=1" ")|xx
|" "1(?=0)/" ")" "{$" "&" "<<" "34}.tr(" "'1'," "''<<
3" "2)" ".g" "s" "ub" "('" "0" "'){;" ";;s.
slice!(0,1)}.scan(/.{66}/){puts$&}#CONCATQUINE(c)Y.Endoh_2012"
=end

#6 Updated by Nikolai Weibull about 2 years ago

On Fri, Apr 6, 2012 at 16:52, marcandre (Marc-Andre Lafortune)
ruby-core@marc-andre.ca wrote:

 > What is this ('concatenation' 'syntax') useful for?

 raise ArgumentError,
   'they are useful for long strings that are so long that they need ' \
   'to span multiple lines to fit within a reasonable line width'

In this example, replacing the "\" with a "+" will give basically the same result without using the concatenation rule.

The “basically the same” being that the first happens at compile time
and the second at run time, for anyone wondering.

#7 Updated by Rodrigo Rosenfeld Rosas about 2 years ago

Yusuke, am I supposed to understand what you meant with the code in your last comment? If so, I didn't get it...

Nikolai, good point, I haven't thought about this before and I wasn't wondering :)

But do you really think that this could have some meaningful impact in the performance of replacing '\' with '+'?

#8 Updated by Yukihiro Matsumoto about 2 years ago

  • Assignee changed from Yukihiro Matsumoto to Yusuke Endoh

I agree with removing string concatenation in 3.0.

#9 Updated by Yusuke Endoh about 2 years ago

=begin
Matz,

I agree with removing string concatenation in 3.0.

Okay. Then, is the syntax deprecated in 2.0?
If so, it would be good to warn it.

diff --git a/parse.y b/parse.y
index 0393dc1..1726117 100644
--- a/parse.y
+++ b/parse.y
@@ -3916,6 +3916,7 @@ string : tCHAR
| string string1
{
/%%%/
+ rbwarning0("string concatenation syntax is deprecated");
$$ = literal
concat($1, $2);
/*%
$$ = dispatch2(string_concat, $1, $2);

Rodorigo,

Yusuke, am I supposed to understand what you meant with the code in your last comment? If so, I didn't get it...

Sorry for the confusion. The code is just a joke.
I can write such a code much easily without the syntax.

eval$s=%q(puts(%(eval$s=%q(#$s)))#################################
## #### #### ###### ##### #### ##
## ######### ### #### ## ##### ######### ### ###### #####
## ######### ### #### ## ##### ######### ### ###### #####
## ######### ### #### ## ##### ######### ###### #####
## #### #### ## ##### #### #l# ###### #####
#################################################################)

Yusuke Endoh mame@tsg.ne.jp
=end

#10 Updated by Yukihiro Matsumoto about 2 years ago

Hi,

In message "Re: [ruby-trunk - Feature #6265] Remove 'useless' 'concatenation' syntax"
on Mon, 9 Apr 2012 12:43:58 +0900, "mame (Yusuke Endoh)" mame@tsg.ne.jp writes:

|> I agree with removing string concatenation in 3.0.
|
|Okay. Then, is the syntax deprecated in 2.0?
|If so, it would be good to warn it.

I agree with making it deprecated. It is up to you to make it warn or
not.

                        matz.

#11 Updated by Rodrigo Rosenfeld Rosas about 2 years ago

Thank you very much! I do also agree that we should warn users so that they have some time to change their "\" style to "+".

Also, while on the subject, don't you think that Ruby specs should state that ("some " + "string") would concatenate the strings in compiling time and that they shouldn't expect the :+ method to be called? That way we wouldn't have any perfomance degradation using this style...

#12 Updated by Rodrigo Rosenfeld Rosas about 2 years ago

Hi Yusuke, I guessed it was a joke, but I couldn't get it at that time. Actually I had a hard time reading "CONCAT" in your last example. After that, I could read it too in the first ones, which is much harder :) I guess I have some problem with my brain! :D

#13 Updated by Nobuyoshi Nakada about 2 years ago

Hi,

(12/04/09 22:46), rosenfeld (Rodrigo Rosenfeld Rosas) wrote:

Also, while on the subject, don't you think that Ruby specs should
state that ("some " + "string") would concatenate the strings in
compiling time and that they shouldn't expect the :+ method to be
called? That way we wouldn't have any perfomance degradation using
this style...

It can't, because String#+ might be overridden.

--
Nobu Nakada

#14 Updated by Rodrigo Rosenfeld Rosas about 2 years ago

Em 09-04-2012 12:53, Nobuyoshi Nakada escreveu:

Hi,

(12/04/09 22:46), rosenfeld (Rodrigo Rosenfeld Rosas) wrote:

Also, while on the subject, don't you think that Ruby specs should
state that ("some " + "string") would concatenate the strings in
compiling time and that they shouldn't expect the :+ method to be
called? That way we wouldn't have any perfomance degradation using
this style...
It can't, because String#+ might be overridden.

I know, that is why I proposed this to be changed in a specification level.

For instance, what would be the issue with an overridden String#+ not
being called in such cases?

#15 Updated by Koichi Sasada about 2 years ago

(2012/04/09 14:19), Yukihiro Matsumoto wrote:

I agree with making it deprecated. It is up to you to make it warn or
not.

I prefer current behavior because it is same as C and it is easy to
remember.

Why you remove it? I think the proposed reasons before don't make sense
for me.

--
// SASADA Koichi at atdot dot net

#16 Updated by Rodrigo Rosenfeld Rosas about 2 years ago

Em 09-04-2012 16:06, SASADA Koichi escreveu:

(2012/04/09 14:19), Yukihiro Matsumoto wrote:

I agree with making it deprecated. It is up to you to make it warn or
not.
I prefer current behavior because it is same as C and it is easy to
remember.

Why you remove it? I think the proposed reasons before don't make sense
for me.

Hi Koichi, I created this issue because I did have this problem.

I've hit some key by mistake in my Vim editor and it deleted the comma
separating two string arguments and it took me a while to figure it out
what have happened.

#17 Updated by Koichi Sasada about 2 years ago

(2012/04/10 7:01), Rodrigo Rosenfeld Rosas wrote:

Hi Koichi, I created this issue because I did have this problem.

I've hit some key by mistake in my Vim editor and it deleted the comma
separating two string arguments and it took me a while to figure it out
what have happened.

On the other hand, I have never met such mistake. So I want to know
such mistake should be considered or not.

--
// SASADA Koichi at atdot dot net

#18 Updated by Martin Dürst about 2 years ago

ko1 (Koichi Sasada) wrote:

I prefer current behavior because it is same as C and it is easy to
remember.

It is more important in C because it can help with macros (#define). Fortunately, we don't have macros in Ruby, so it's less important in Ruby.

Why you remove it? I think the proposed reasons before don't make sense
for me.

I agree that there's no urgent need to remove it.

#19 Updated by Alexey Muranov about 2 years ago

I think it is nice to have a syntax to concatenate strings in compile time. I do not think that missing commas are hard to notice.

#20 Updated by Thomas Sawyer about 2 years ago

=begin
The only time I have found it useful is with \ for multi-line string, for things like entry in Ruby based-config file. E.g.

self.description = 'This is a multi-line ' \
                   'description of something.'

=end

#21 Updated by Thomas Sawyer about 2 years ago

trans (Thomas Sawyer) wrote:

The only time I have found it useful is with \ for multi-line string, for things like entry in Ruby based-config file. E.g.

self.description = 'This is a multi-line' \
                   'description of something.'

#22 Updated by Boris Stitnicky almost 2 years ago

I agree with Alex that when I first saw it I thought it was cool, but then I never really needed to use it, like trans says.

#23 Updated by Rodrigo Rosenfeld Rosas almost 2 years ago

I'm glad to see that I'm not the only one to experience this issue :)

https://github.com/frodsan/rails/commit/1c5722cdf675fce6c7f69bfb5ca773ce3db8fe19

#24 Updated by Marc-Andre Lafortune almost 2 years ago

  • Category set to core
  • Target version changed from Next Major to 2.0.0

Changing target for 2.0 so that we add the deprecation warning.

A warning is good practice and should happen at the best moment: at parse time.

#25 Updated by Usaku NAKAMURA over 1 year ago

  • Status changed from Assigned to Closed
  • % Done changed from 0 to 100

This issue was solved with changeset r37317.
Rodrigo, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.


  • parse.y: show a warning for concatenating string literals because it will be deprecated in the future. patched by mame (Yusuke Endoh) at . [Feature #6265]

#26 Updated by Usaku NAKAMURA over 1 year ago

  • Status changed from Closed to Assigned
  • Assignee changed from Yusuke Endoh to Eric Hodel

Now I've committed mame-san's patch (I was asked by him to commit instead of him).

But this change introduced some failures to make test-all, because rubygems
uses string concatenation at lib/rubygems.rb:1246-1249.
Eric, could you change these lines?

#27 Updated by Yui NARUSE over 1 year ago

  • Status changed from Assigned to Closed

This issue was solved with changeset r37327.
Rodrigo, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.


Revert r37316

The commit introduces too many failures and disturbs release engineering.
Re-commit it with fixed tests.

Thu Oct 25 13:09:01 2012 NAKAMURA Usaku usa@ruby-lang.org

   * parse.y: show a warning for concatenating string literals because
     it will be deprecated in the future.
     patched by mame (Yusuke Endoh) at .
      [Feature #6265]

#28 Updated by Yui NARUSE over 1 year ago

  • Status changed from Closed to Assigned

I reverted r37316 because such too many failures prevents watching CI.
Please re-commit it with fixes for those tests.
http://u32.rubyci.org/~chkbuild/ruby-trunk/log/20121025T110301Z.log.html.gz

#29 Updated by Yui NARUSE over 1 year ago

  • Status changed from Assigned to Feedback
  • Assignee deleted (Eric Hodel)

Since affected libraries are so many, the fix should be done by the proposer.

Moreover I think this insists that string concatenation is not useless.

#30 Updated by Nobuyoshi Nakada over 1 year ago

I also strongly object this change, since it needed many escapes which were unnecessary otherwise.

#31 Updated by Rodrigo Rosenfeld Rosas over 1 year ago

Right now I'm in the middle of some very tight deadlines in my job and won't have time to fix all usages of this kind of string concatenation but I'd be happy to do so next year, although I'm afraid I wouldn't be on time for Ruby 2.0 release :(

#32 Updated by Eric Hodel over 1 year ago

I don't think it is appropriate to include this change in the ruby 2.x series without any deprecation period. I have many libraries that depend on it.

Removing it in 3.0 with a warning in 2.x would be more appropriate.

#33 Updated by Rodrigo Rosenfeld Rosas over 1 year ago

I indeed agree

#34 Updated by Koichi Sasada over 1 year ago

  • Assignee set to Yusuke Endoh

#35 Updated by Yusuke Endoh over 1 year ago

Eric,

drbrain (Eric Hodel) wrote:

Removing it in 3.0 with a warning in 2.x would be more appropriate.

We are actually trying to do so. r37316 is a warning patch for that.
And the warning, not changing syntax, caused many test failures in rubygems.

Yusuke Endoh mame@tsg.ne.jp

#36 Updated by Yusuke Endoh over 1 year ago

  • Status changed from Feedback to Assigned
  • Assignee changed from Yusuke Endoh to Eric Hodel

drbrain, do you have no intention of fixing rubygems? We cannot add the warning.

(Or please make matz change his mind ;-)

Yusuke Endoh mame@tsg.ne.jp

#37 Updated by Eric Hodel over 1 year ago

I will fix RubyGems and RDoc, please stand by.

#38 Updated by Yusuke Endoh over 1 year ago

Thanks drbrain!

Yusuke Endoh mame@tsg.ne.jp

#39 Updated by Eric Hodel over 1 year ago

  • Status changed from Assigned to Closed

This issue was solved with changeset r38230.
Rodrigo, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.


  • lib/rubygems/command_manager.rb: Removed string concatenation syntax. [Bug #6265]
  • lib/rubygems/commands/install_command.rb: ditto
  • lib/rubygems/commands/uninstall_command.rb: ditto
  • lib/rubygems/indexer.rb: ditto
  • lib/rubygems/security/policy.rb: ditto
  • lib/rubygems/security.rb: ditto
  • lib/rubygems/uninstaller.rb: ditto
  • test/rubygems/testgemcommandscertcommand.rb: ditto
  • test/rubygems/testgempackage.rb: ditto
  • test/rubygems/testgemsecurity.rb: ditto
  • test/rubygems/testgemsecurity_policy.rb: ditto

#40 Updated by Eric Hodel over 1 year ago

  • Status changed from Closed to Assigned
  • Assignee changed from Eric Hodel to Yusuke Endoh

Oops, it was accidentally closed.

I have removed the string concatenation syntax from rdoc and rubygems.

#41 Updated by Yusuke Endoh about 1 year ago

  • Subject changed from Remove 'useless' 'concatenation' syntax to Remove &#x27;useless&#x27; &#x27;concatenation&#x27; syntax
  • Target version changed from 2.0.0 to next minor

drbrain, thanks!

But sorry, it is too late to change 2.0.0. My bad.

Yusuke Endoh mame@tsg.ne.jp

Also available in: Atom PDF