Refactor ForbidIncludeConstLiteral

[sambostock]

This makes the following changes:

• Migrates from deprecated Cop parent class to Base
• Improves the offense message
• Removes the unused used_names accessor and initialization
• Renames and alters the node matcher to describe what it matches, not what it doesn't
• Extracts named helpers to clarify the code's logic
• Leverages the expect_correction test helpers and replaces the autocorrection specific tests.

[paracycle]

This seems to be a bug-fix so I think we have no test cases for the explicit receiver not being self. Can we add one or two cases for that as well?

[sambostock]

Good catch.

I did some digging, and it looks like Sorbet doesn't handle include with an explicit receiver.

# typed: strict

module M
  extend T::Sig
  sig { void }
  def m; end
end

class C1
  include M
end

C1.new.m

class C2; end
C2.include M

C2.new.m
#      ^ Method m does not exist on C2

That said, it seems like a reasonable thing that might end up supported eventually (I'm not sure why it isn't already, if the receiver is a constant).

So I could:

• Simplify the code, by only looking for include, extend, & prepend with implicit receivers, or
• Keep the cop "smarter than (current) Sorbet", and add tests.

What do you think?

[paracycle]

Interesting! I would go for keeping the refactored implementation and add tests for those cases. After all, dynamic or not, those includes are not going to work with Sorbet either way. We could at least push people to use less dynamic mixins.

[sambostock]

I've added tests, although I should note that they look like

module M
  M.include(dynamic)
end

which this implementation detects, not

M.include(dynamic)

which this implementation does not detect. Do we want to do that? I'm unsure about false positives.

[sambostock]

Actually, I think my implementation introduces a false positive bug. Neither of these should be flagged, because we can't be sure they're operating on a module.

Config = []
Config.prepend(1)

class MyClass
  Config.prepend(2)
end

I think that is why in my example above Sorbet doesn't handle C2.include M. 🤔

I'm going to add test cases for this (and the equivalent include and extend cases) and make them pass.

[github-actions]

This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[sambostock]

@paracycle I've added these tests as per #153 (comment) to cover the edge cases you pointed out, and adjusted the implementation accordingly.