Replace empty literal with literal.freeze

[mbj]

This might upset people outside the core mutant user group.

But it'll expose cases where a literal is intend not to be mutated but not marked (via freeze) that way.

[tjchambers]

Seems appropriate test for next version of Ruby as well.

[tjchambers]

I presume you will have a way of turning that off once Ruby freezes strings by default?

[tjchambers]

Will this include [].freeze and {}.freeze? I would be quite interested in that mutation.

[mbj]

Will this include [].freeze and {}.freeze? I would be quite interested in that mutation.

Yes these are literals.

[mbj]

And another thought pro this mutation:

Mutant is about to reduce unused semantics in the code. If mutability of a build-in datastructure is seen as a "feature that is by default turned on, but likely unused", it should be reduced.

[tjchambers]

I guess my concern is the effort to add .freeze to literal strings now to kill mutations, when the strings will be frozen AFAIK in Ruby 3 by default.

[mbj]

I guess my concern is the effort to add .freeze to literal strings now to kill mutations, when the strings will be frozen AFAIK in Ruby 3 by default.

In that case we'll remove the mutation from literal strings and mutant shows you "".freeze is equivalent to "" and forces you to write "" again.

In the meantime you avoid bugs in your program, introduced by unintentionally mutating things.

[tjchambers]

In my case I have a lot of literal strings which are inline in code for internationalization.

I18n.t('class_title', klass: klass)

[mbj]

I18n.t('class_title', klass: klass)

Would it be a bug if some library mutates the 'class_title' ?

[mbj]

You can alwas whitelist these via rspec:

before do
  allow(I18n).to receive(:t) do |selctor, options|
    expect(selector).not_to be_frozen
  end.and_call_original
end

[mbj]

Maybe this is also finally a case where I'd support adding conditionally disabling a mutation operator.

[tjchambers]

Yes it would be a bug, but one that would not encourage me to add .freeze to hundreds of them with Ruby 3 under development.

[mbj]

Yes it would be a bug, but one that would not encourage me to add .freeze to hundreds of them with Ruby 3 under development.

You can use a global before hook to whitelist all interactions with I18n.t.

[mbj]

*whitelist for accepting non frozen strings I mean.

[tjchambers]

Still wrapping my head around that, but if I understand your code above, I would be telling rspec to expect that the I18n.t string is NEVER frozen, and if it gets frozen during a mutation that is considered a fail of the spec.

[mbj]

Still wrapping my head around that, but if I understand your code above, I would be telling rspec to expect that the I18n.t string is NEVER frozen, and if it gets frozen during a mutation that is considered a fail of the spec.

Yes. It would essentailly make sure this mutation would never be an alive when used with I18n.t. Where at other places we get the benefit.

[tjchambers]

I think that is a very good approach given MY code. Not sure how universal that would appeal.

[mbj]

I think we want to start with freezing array and hash literals. Here its very unlikely the amount of call sides to fix is overwhelming.

[tjchambers]

@mbj - for me the freezing of array and hashes are going to be very helpful in focusing bugs. I have started to uses constants for the ones I deem frozen by default and it has been beneficial.

[mbj]

Maybe we can even start with freezing (EDIT: only) empty hash and array literals.

[tjchambers]

I think freezing empty strings as well is for me also valuable because if they are frozen and then never modified it could be a spec gap.

[mbj]

I think freezing "any empty literal" is the way to start this.

[tjchambers]

I concur with that.

[tjchambers]

before do
  allow(I18n).to receive(:t) do |selector, options|
    expect(selector).not_to be_frozen
  end.and_call_original
end

appears not to be valid syntax according to @myronmarston
rspec/rspec-mocks#774

myronmarston commented on Sep 2, 2014
The warning is perhaps a bit cryptic and poorly written, but it is accurate. It's happening because you are combining a block implementation (your block with expect(env['foo.bar']).to eq 42) and and_call_original. and_call_original can't be used with a block implementation and essentially replaces/overrides it -- hence the warning. The block in your case is never called. You can change it to:

expect_any_instance_of(detector).to receive(:call).with(hash_including('foo.bar' => 42)).and_call_original

I am attempting to see what I can replace the matcher with re: .frozen?

[mbj]

My code example was not meant to be 1:1 copy & pastable. Sorry for not labeling it as untested proactively.

I think you could get around this limitation via monkey patching I18n.t via a module prepend that raises in case the string is frozen.

As we reduced this ticket to only cover empty literalls this discussion is even partially redundant ;)

[tjchambers]

I didn't expect such a rapid snippet of code to be tested at all. Amazing how fast you came up with it. :)

That's why I tested it in my environment. Wanted to capture that it was not working in RSpec context. I would however like something similar for other purposes, so I am exploring options. Wanted to capture that the above code was not usable as is in this thread as well.

It is irrelevant while this enhancement stays with empty literals.

[tjchambers]

FYI here is what I came up with to define expectations of parameters to I18n.t that does the job for me:

 def unfrozen_string
      satisfy { |*arg| !arg.first.frozen? }
 end

 allow(I18n).to receive(:t).with(unfrozen_string, any_args).and_call_original

I put that in my rails_helper config section in a before(:each) block. Does what I needed.

[backus]

For ruby 2.3 this mutation (for strings) could be String#-@ which will return a frozen string

[mbj]

I do not like this upstream API. Its now even harder to differentiate between number and string aritmetic.

I personally think the mutation should use "str" into "str".freeze which is very explicit.

[backus]

I do not like this upstream API. Its now even harder to differentiate between number and string aritmetic.

Yeah I agree.

[tjchambers]

Is that format supposed to be intuitive?

[mbj]

Is that format supposed to be intuitive?

The -"foo" syntax @backus referenced is likely intend to be intuitive. IMO its just dangerous, as it introduces another dimension of possible misunderstanding. AFAIK it also works on -foo where foo evaluates to a string. This introduces another indirection for reviewers.

[backus]

I think it is supposed to be intuitive but I don't think it is. Quoting feature discussion:

Matz said

In fact, my best choice is introducing String#+ that returns a mutable copy of a string.
[Ruby trunk - Bug #11759]

So that this is a ticket for that.
I'll commit it ASAP to check this methods before 2.3.

Specification:

• +'foo' returns modifiable string.
• -'foo' returns frozen string (because wasters will freeze below 0 degree in Celsius).

[mbj]

Haha, you know can possibly write C in ruby a ++"foo" :P

[tjchambers]

Wow. If I had to memorize that syntax I still don't think I could recall it.