Stop disabling Style/MutableConstant

[casperisfine]

I couldn't find any discussion about this cop, but I believe it's a positive one.

First there's the correctness argument. If a constant is mutable is it really a constant?

Then there's a performance argument. Hashes unless frozen are mutated uppon iteration
(their internal iter_level attribute is incremented). This cause constant hashes that
are in a clean CoW region to invalidate the entire 4kiB memory page.

cc @rafaelfranca as I think you have some strong opinions on this one.

[rafaelfranca]

Yeah, I have strong opinions about freeze everywhere. I really don't buy the argument of mutability. Even if you freeze the object you can still mutate the constant. This freeze is just unnecessary complexity. If we really want immutability constant we need to change Ruby to now allow constant reassignment.

I'm ok with the performance argument though, specially for hashes, but of course this cop will not help much since you can have nested hashes and I doubt people will remember to freeze those.

Should we define a constant like this now?

SOME_HASH = { options: { filter: ["something"].freeze }.freeze }.freeze

That is too much pollution, and I'm pretty sure the cop doesn't catch it.

If we only free the outer hash we will have a bunch of inner hashes invalidating the pages.

I think even if we enable we will not have that much improvement in performance, so not sure it is worth the trouble.

[casperisfine]

Yeah, I have strong opinions about freeze everywhere.

That's what I remembered 😄

I really don't buy the argument of mutability. Even if you freeze the object you can still mutate the constant.

Sure, but the argument is about preventing mistake.

If you override a constant you'll get a warning (which acts as an error on CI) it's unlikely you won't notice.

However if you have something like:

DEFAULT_ARGS = { foo: "bar" }

...

do_something(DEFAULT_ARGS)

...

def do_something(options)
  if something = options.delete(:something)
    ...
end

It's easy to accidentally mutate global state. Either by passing it to a method you don't realize mutate its argument, or either by modifying a method to start mutating its arguments.

That's the advantage of freezing constants, it helps catch mistakes, just like constant warnings.

I'm pretty sure the cop doesn't catch it.

Seems like you're right, it could be improved though.

I think even if we enable we will not have that much improvement in performance

I wish I had a way to measure how effective it would be, but it's super tricky. I can tell how many unfrozen hashes we have post boot, but can't tell which are linked to a constant :/

[rafaelfranca]

I'm ok with enable this cop and try it out. It might show some improvement in performance.

I can also see the increased safety and being consistent with my position on introducing types in the language, that increased safety should not be ignored.

I updated the branch to pass all the CI. I'll wait other people opinions.

@sambostock @dougedey-shopify @kmcphillips mind to take a look on this proposal?

[dougedey-shopify]

I was actually thinking about this earlier on one of Kevin's PRs, I have seen the lack of freeze cause issues in tests, but that's about it.

[casperisfine]

To be fair, since I can't really predict the impact on CoW (and even if I could it's heavily dependent on the localtions in the heap, so it would be very variable) I don't want to make the memory argument the main one.

This is first and foremost about catching mistakes.

That being said, since it only freeze the top level object it's not as useful as it could be, so maybe it would be worth improving the cop before enabling it.

[kmcphillips]

Hashes unless frozen are mutated uppon iteration

TIL

I'm in favour of this. Mostly for the reasons stated. It can catch mistakes and accidental bugs and misuse.

I agree with @rafaelfranca 's criticism on it being a pain to add a .freeze predicate to every nested object. I had mistakenly though there was a .deep_freeze that works like .deep_symbolize_keys, but there is not. We could implement one or use a gem if we really want.

Even freezing the top level is an improvement for now.

[casperisfine]

Alternatively there's the # shareable_constant_value: literal pragma, that would freeze all literals assigned to a constant.

On one side it's much simpler, on the other I'm not a big fan to have yet another magic comment across the codebase.

[kmcphillips]

I'm low opinion on which is better. But if we do use that magic comment we would probably want a cop to prevent redundant freezes on already frozen consts.

[rafaelfranca]

Alternatively there's the # shareable_constant_value: literal pragma, that would freeze all literals assigned to a constant.

I actually thing that is better. There is no need for users to have to remember to call freeze when declaring new constants, only when creating the file the first time, but I'm ok with the freeze mode as well.

[sambostock]

I like the safety and signalling aspect of this. It's the same reason my team uses private_constant: not so much to make it impossible to access or modify the constant, but to signal that it's an implementation detail.

---

Does Sorbet impact this rule at all? Presumably the cop would enforce

-CONST = T.let(["abc"], T::Array[String])
+CONST = T.let(["abc"], T::Array[String]).freeze

whereas a developer might naturally choose to write something like

-CONST = T.let(["abc"], T::Array[String])
+CONST = T.let(["abc"].freeze, T::Array[String])

which the cop would reject. I assume they are both equivalent though?

---

I've always been surprised that Ruby doesn't have Object#deep_freeze built in. Seems like a logical extension to .freeze.

---

Incidentally, what is iter_level? If it's possible to iterate over frozen hashes without mutating iter_level, then what is being accomplished by mutating it on un-frozen hashes?

[casperisfine]

Incidentally, what is iter_level?

It's what records that the hash is being iterated:

>> (h = {foo: 1}).each { |k, v| h[:bar] = 2}
(irb):1:in `block in <main>': can't add a new key into hash during iteration (RuntimeError)

It's basically a semaphore, e.g. pseudo-code:

class Hash
  def each
    @iter_level += 1 unless frozen?
    #...
    @iter_level -= 1 unless frozen?
  end
end

[st0012]

I think a recent Sorbet PR (sorbet/sorbet#7993) makes this a more appealing change: Sorbet can now infer types of literal arrays, which is often used in constants. And if we can also freeze those constants, like CONST = [1, 2, 3].freeze, then Sorbet can also know the number of items CONST holds, which makes splatting CONST possible without adding T.unsafe.

Additionally, that inferred type information surfaced a few cases where a constant got mutated indirectly by a method (e.g. got called map!(&:to_s) underneath), which could've been caught if the constant were frozen.

Unfortunately, Sorbet doesn't seem to support # shareable_constant_value: literal yet. So we can only benefit from direct freeze calls.