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

Style/CaseLikeIf unsafe if conditionally executed code needs MatchData #8541

Closed
JoeCohen opened this issue Aug 15, 2020 · 1 comment · Fixed by #8504
Closed

Style/CaseLikeIf unsafe if conditionally executed code needs MatchData #8541

JoeCohen opened this issue Aug 15, 2020 · 1 comment · Fixed by #8504

Comments

@JoeCohen
Copy link
Contributor

JoeCohen commented Aug 15, 2020

Style/CaseLikeIf is unsafe in edge cases. It gives false positives if both:

  • if or elsif includes a regular expression, and
  • the code that's executed when the condition is true depends on something in the match other than the existence or numerical position of a match.

Expected behavior

I expect RuboCop to not generate a Style/CaseLikeIf offense.

Actual behavior

RuboCop generates a Style/CaseLikeIf offense.

Steps to reproduce the problem

Create an if-elsif where the if or elsif includes =~ or .match and the conditionally executed code depends on something in the match other than true/false or the position of the match.

In my case the problematic code was

      if /^(?<yr>\d{4})$/ =~ val
        yyyymmdd([yr, 1, 1], [yr, 12, 31])
      elsif /^(?<yr>\d{4})-(?<mo>\d\d?)$/ =~ val
        yyyymmdd([yr, mo, 1], [yr, mo, 31])
      elsif /^(?<yr>\d{4})-(?<mo>\d\d?)-(?<day>\d\d?)$/ =~ val
        yyyymmdd([yr, mo, day], [yr, mo, day])
      elsif /^(?<yr1>\d{4})-(?<yr2>\d{4})$/ =~ val
        yyyymmdd([yr1, 1, 1], [yr2, 12, 31])
      elsif /^(?<yr1>\d{4})-(?<mo1>\d\d?)-(?<yr2>\d{4})-(?<mo2>\d\d?)$/ =~ val
        yyyymmdd([yr1, mo1, 1], [yr2, mo2, 31])
      elsif /^(?<yr1>\d{4})-(?<mo1>\d\d?)-(?<dy1>\d\d?)-
             (?<yr2>\d{4})-(?<mo2>\d\d?)-(?<dy2>\d\d?)$/x =~ val
        yyyymmdd([yr1, mo1, dy1], [yr2, mo2, dy2])
      elsif /^(?<mo>\d\d?)$/ =~ val
        mmdd([mo, 1], [mo, 31])
      elsif /^(?<mo1>\d\d?)-(?<mo2>\d\d?)$/ =~ val
        mmdd([mo1, 1], [mo2, 31])
      elsif /^(?<mo1>\d\d?)-(?<dy1>\d\d?)-(?<mo2>\d\d?)-(?<dy2>\d\d?)$/ =~ val
        mmdd([mo1, dy1], [mo2, dy2])
      else
        raise BadDateRangeError.new(var: var, val: val)
      end

Each if or elsif has a named capture group that's used in the following conditionally executed code.
But if you re-write the above as a case statement, , e.g.:

      case val
      when /^(?<yr>\d{4})$/ 
        yyyymmdd([yr, 1, 1], [yr, 12, 31])
      ...
      end

then the named capture group (yr)is not available to the conditionally executed code.
This is due to the fact that when uses ===, which is not equivalent to the =~ used in if and elsif. Compare

$ /(?<yr>\d{4})/ === "20190815"; yr
true
nil

with

$ /(?<yr>\d{4})/ =~ "20190815"; yr
0
"2019"

Comment:
Some options:

  1. Mark Style/CaseLikeIf as unsafe.
  2. Make Style/CaseLikeIf safe by catching all the edge cases. Strikes me as very difficult or impossible to accomplish.
  3. Make Style/CaseLikeIf safe by ignoring conditionals where an if or elsif includes =~ or .match. (I don't like this. Style/CaseLikeIf does work for the most common regex use case, and I wouldn't like to throw that away.)

RuboCop version

0.88.0 (using Parser 2.7.1.4, rubocop-ast 0.3.0, running on ruby 2.6.6 x86_64-linux)

@dmytro-savochkin
Copy link

Yeah, I already prepared a PR to fix this: #8504. Hopefully it will be merged soon. There I am going to prevent this cop from throwing an offense if there is a regexp with named captures inside and =~ or match is used. It isn't ideal though, it won't throw an offense if there is a regexp with named captures which are not used later. But probably this would be a very edgy case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants