Add Lint/ConstantReassignment cop

[lovro-bikic]

This PR adds a cop for checking constant reassignments in the same file, essentially trying to emulate the already initialized constant X warning for simple use-cases.

Background

In production code, I've seen duplicate constant assignments a fair amount of times, especially when files have a lot of constants, e.g:

class AbTester
  TEST_NAMES = [
    TEST_FOO = :foo,
    TEST_BAR = :bar,
    TEST_BAZ = :baz,
    # ... tens of other constants ...
    TEST_FOO = :foo, # already assigned before
  ]
end

These issues can happen for various reasons: bad merge conflict resolutions, inattention, anything else humans are susceptible to. One module I've seen in production code has hundreds of constants; it's very easy to mess something up, and it has happened before.

The idea behind this cop is to report such trivial issues so they're caught as soon as possible.

Additional information

This cop can:

• register an offense when NAME = value syntax is used
• track when constant is removed with remove_const :NAME
• differentiate constant namespaces (e.g. A::X and B::X assigned in the same file would not be reported as a reassignment)

This cop cannot register an offense when:

• a constant is assigned in one file and reassigned in another
• Module#const_set is used (though I guess that simple support could be added)
• Module#remove_const is used with anything that's not a symbol

Basically, in this first iteration, this cop is only useful for the most blatant constant reassignments that happen locally (i.e. in the same file) and trivially (i.e. using the NAME = value syntax). As it cannot catch all issues, I'm not sure how useful the community would find this cop, but I'm opening this PR to at least get a sense of that.

Lastly, credit where credit is due, tagging @mostlyobvious as the one who prompted the idea for this cop. Thanks!

---

Before submitting the PR make sure the following are checked:

• The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
• Wrote good commit messages.
• Commit message starts with [Fix #issue-number] (if the related issue exists).
• Feature branch is up-to-date with master (if not - rebase it).
• Squashed related commits together.
• Added tests.
• Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
• Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.

[Earlopain]

This is not such an easy cop to implement correctly. if/unless and other control flow make it very hard to correctly handle. For example, this is an offense at the moment:

if something
  FOO = 123
else
  FOO = 455
end

I think, limiting the cop only to the most simple form of top-level constants, and disregarding anything else for safety would still help with the majority of cases this cop is looking for.

[lovro-bikic]

For example, this is an offense at the moment

Yes, unfortunately; what about limiting the cop to constant assignments that don't happen within a conditional? node, as long as that condition happens before a class or module definition? I've updated the PR to show what I mean.

Case:

if something
  FOO = 123
else
  FOO = 456
end

is no longer registered as an offense, but if there's a condition outside the namespace, a duplicate assignment would still be an offense, e.g.:

class A
  FOO = 123
  FOO = 456
# ^^^^^^^^^ Constant `FOO` is already assigned in this namespace.
end unless defined?(A)

[Earlopain]

Yes, I think that will work. I doubt your second example is very common but its whatever.

I did find another false positive:

begin
  REV = File.read("#{Rails.root}/REVISION").strip
rescue StandardError
  REV = nil
end

and also some false positives around remove_const in tests with strings. Strings work, in addition to symbols. I think those false positives should go away if you handle strings as well.

This code errors:

self::FOO = 137
# => undefined method 'short_name' for an instance of RuboCop::AST::Node

I also found code like this:

silence_warnings { ALREADY_EXISTING_CONST = @foo }

which will be impossible to correctly handle with this cop. Or two different rake tasks where only one will surely be executed so there is no risk of redefinition, even though there are two code paths that assign to the same constant.

Personally, I would have this cop only check these constants:

class Foo
  # ok, toplevel
  BAR = 123
  
  foo do
   # ignore, nested
    BAZ  = 456
  end

  class Bat
    # ok, toplevel again
    FOOBAR = 789
  end
end

There are just too many variations where you can do weird things that are impossible to analyse simply through the ast. I think it will still catch the large majority of cases, even if you make it a bit simpler.

[lovro-bikic]

This code errors: self::FOO = 137

Nice catch, thanks! I think I've fixed it correctly, but would appreciate a double-check.

some false positives around remove_const in tests with strings. Strings work, in addition to symbols. I think those false positives should go away if you handle strings as well.

This should be handled as well now.

There are just too many variations where you can do weird things that are impossible to analyse simply through the ast. I think it will still catch the large majority of cases, even if you make it a bit simpler.

I agree, static analysis is hardly the right fit for a runtime warning, unless the offense is obvious. I'm okay with reducing the cop's scope like this initially, even though it might result in more false negatives. We can always expand the cop's scope later if there are cases which wouldn't result in false positives, or if false positives would be rare. I'll make this change, will send another update when it's done.

[lovro-bikic]

I've updated the PR and reduced its scope to the simplest assignments. Note that I've preserved the case when a constant is assigned within another constant as an offense as it shouldn't result in false positives, for example:

class A
  ALL = [
    FOO = 123,
    FOO = 456,
  ]
end

Cases such as the next example won't be reported as an offense anymore, even though they technically are:

FOO = 123

Class.new do
  FOO = 456
end

but such code is also handled by Lint/ConstantDefinitionInBlock, so the user will be aware that in both assignments FOO is assigned on top-level.

EDIT: I've added support for frozen constants, this example will also be registered as an offense:

class A
  ALL = [
    FOO = 123,
    FOO = 456,
  ].freeze
end

[Earlopain]

I think I've fixed it correctly, but would appreciate a double-check.

It looks OK to me. For the purpose of this cop, self shouldn't make a difference. When using inheritance it can but it's out of scope. This for example shows two warnings when run:

class A
  FOO = 123
end

class B < A
  self::FOO = 456
  FOO = 789
end
# => test.rb:7: warning: already initialized constant B::FOO
# => test.rb:6: warning: previous definition of FOO was here

Cases such as the next example won't be reported as an offense anymore, even though they technically are:
Class.new example ...

Yes, that seems fine. You don't know when a block will run most of the time (although for Class.new and a few other we do). I would not special-case it here and just let the other cop catch it like you said.

---

Something weird is going on with your conditional check. If there is only one expression it looks fine but registers offense for this:

if true
  FOO = 1
  2
else
  FOO = 3
  3
end

This as well, probably coming from the same cause?

silence_warnings do
  orig = FOO
  FOO = 2
  FOO = orignal
end

This triggers an error (sorry, missed beforehand):

some_method::FOO = 1

lvar = BAR
lvar::BAZ = 2

Instead of filtering out self_type, you should probably select only const_type. I don't think any other type of node should be considered, and they also don't respond to short_name.

[lovro-bikic]

Something weird is going on with your conditional check. If there is only one expression it looks fine but registers offense for this

Sorry, this was due to my poor understanding of rubocop-ast. Multiple expressions are wrapped in a begin node, I didn't know that before. It should be fixed now.

This as well, probably coming from the same cause? silence_warning example

Yes, the cause was the same. It should be fixed as well now (I also added a test for this specific example).

This triggers an error (sorry, missed beforehand):
Instead of filtering out self_type, you should probably select only const_type.

This should be fixed now as well, thanks!

[bbatsov]

I'm guessing some "nested namespace" checking logic here would be useful (I think Lint/Void does this), as some re-assignment in a conditional or a block will likely be a oversight most of the time, but I guess the current code is a decent (and fairly safe) starting point.

[lovro-bikic]

Thanks! And thank you @Earlopain for all the help, it's much appreciated. ❤️