mutant
mutant copied to clipboard
mbj
Replace empty literal with literal.freeze
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.
I presume you will have a way of turning that off once Ruby freezes strings by default?
Will this include [].freeze and {}.freeze? I would be quite interested in that mutation.
Will this include [].freeze and {}.freeze? I would be quite interested in that mutation.
Yes these are literals.
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.
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.
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.
In my case I have a lot of literal strings which are inline in code for internationalization.
I18n.t('class_title', klass: klass)
I18n.t('class_title', klass: klass)
Would it be a bug if some library mutates the 'class_title' ?
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
Maybe this is also finally a case where I'd support adding conditionally disabling a mutation operator.
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.
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.
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.
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.
I think that is a very good approach given MY code. Not sure how universal that would appeal.
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.
@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.
Maybe we can even start with freezing (EDIT: only) empty hash and array literals.
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.
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 https://github.com/rspec/rspec-mocks/issues/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?
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 ;)
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.
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.
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.
I do not like this upstream API. Its now even harder to differentiate between number and string aritmetic.
Yeah I agree.