Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Performance/ConstantRegexp with a constant defined in a subclass #205

Closed
dvandersluis opened this issue Jan 11, 2021 · 3 comments · Fixed by #206
Closed

Performance/ConstantRegexp with a constant defined in a subclass #205

dvandersluis opened this issue Jan 11, 2021 · 3 comments · Fixed by #206

Comments

@dvandersluis
Copy link
Member

When a regexp depends on a constant defined in a subclass, neither of the fixes suggested by Performance/ConstantRegexp work:

class C
  def self.regexp
    %r{foo #{self::BASE_PATTERN}}
  end
end

class X < C
  BASE_PATTERN = /bar/
end

class Y < C
  BASE_PATTERN = /baz/
end

If the pattern is change to use the /o modifier, the first time the method is triggered the regexp will be interpolated for both subclasses:

X.regexp
=> /foo (?-mix:bar)/
Y.regexp
=> /foo (?-mix:bar)/

If the regexp is extracted to a constant, it needs to be duplicated in every subclass (and the regexp could be considerably more complicated than the example here).

RuboCop version

Include the output of rubocop -V or bundle exec rubocop -V if using Bundler. Here's an example:

$ [bundle exec] rubocop -V
rubocop -V
1.8.1 (using Parser 3.0.0.0, rubocop-ast 1.4.0, running on ruby 2.6.6 x86_64-darwin18)
  - rubocop-performance 1.9.2
  - rubocop-rails 2.9.1
  - rubocop-rspec 2.1.0
@marcandre
Copy link
Contributor

It would be more efficient to cache the result.

One "correct" code should probably be:

class C
  def self.regexp
    @regexp ||= %r{foo #{self::BASE_PATTERN}}
  end
end

Is that accepted by by ConstantRegexp?

Although this won't work from Ractors... (one should use my ractor_cache gem for that purpose).

Or maybe this would work?

def self.inherited(base)
  # Use the `o` flag because `BASE_PATTERN` isn't defined yet
  base.class_eval "REGEXP = /foo #{self::BASE_PATTERN}/o"
end

@dvandersluis
Copy link
Member Author

One "correct" code should probably be:

class C
  def self.regexp
    @regexp ||= %r{foo #{self::BASE_PATTERN}}
  end
end

I like this and changed my code to use this pattern, but it is still not accepted by the cop, no.

@marcandre
Copy link
Contributor

I think that memoized regexp like that should be acceptable 👍

dvandersluis added a commit to dvandersluis/rubocop-performance that referenced this issue Jan 11, 2021
dvandersluis added a commit to dvandersluis/rubocop-performance that referenced this issue Jan 11, 2021
@koic koic closed this as completed in #206 Jan 12, 2021
koic added a commit that referenced this issue Jan 12, 2021
[Fix #205] Update `Performance/ConstantRegexp` to allow memoized regexps
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants