linter icon indicating copy to clipboard operation
linter copied to clipboard

valid_regexps should warn about string literal

Open alescdb opened this issue 7 years ago • 9 comments

The regular expression new RegExp('\(([^\)]*)\)'); is not valid in dart, and (I think) we should be warned about it by lint.

ref : https://github.com/dart-lang/sdk/issues/33669

alescdb avatar Jun 28 '18 11:06 alescdb

Seems to be a valid regex according to the comments in the linked issue. What warning would you expect?

zoechi avatar Jun 28 '18 12:06 zoechi

If I'm understanding, I think the issue is not that it's an invalid regular expression but rather that it does not match what you think it should (e.g., in this case just a literal string match). I think I know what you mean though. Maybe something along the lines of a warning when you are using a regexp when a string match mould do? In that case, maybe another lint for unecessary_regexp or something? My worry here would be potential complexity and false positives.

Thanks for opening this issue!

pq avatar Jun 28 '18 12:06 pq

I interpreted it that it should warn about the missing r prefix.

zoechi avatar Jun 28 '18 12:06 zoechi

I interpreted it that it should warn about the missing r prefix.

Oh! (It's early here; need more ☕️.)

That's easier but likely to flag a bunch of false positives? For example, I count ~12 regexps in the linter package that are valid but not defined w/ raw strings...

pq avatar Jun 28 '18 12:06 pq

Sorry if my request is not clear :-) The regexp is indeed valid and it compiles, but it does not match as expected in other languages because as @lrhn says in it's comment \(\) is 'equivalent' to (). Maybe a warning when using escaped characters without raw string should be enough ?

alescdb avatar Jun 28 '18 12:06 alescdb

No worries @alescdb. This is a great conversation. Thanks for kicking it off!

Maybe a warning when using escaped characters without raw string should be enough ?

Interesting idea. My gut says that should be another lint. (These are not technically invalid and may, in odd cases, actually be what you have in mind? 🤔 ) The trick is in writing the write prescription. We're not checking for validity exactly. Maybe we just want something that flags regexp_literal_escapes?

Want to take a stab at writing a sentence or two description? 😄

pq avatar Jun 28 '18 13:06 pq

If escaping parenthesis (\(.?*)\) is 'translated' in an unescaped parenthesis ((.?*)) why would you escape it on purpose ? I thougth it was a RegExp bug because in my opinion this is not a normal behavior compared to other languages (C#, PHP, grep, ...)

alescdb avatar Jun 28 '18 15:06 alescdb

Ah, OK. I honestly have to look up the grammar for regexps every time I use them for exactly the reason you describe. 🙄 Definitely annoying and if analysis can help, all the better.

Flagging this one as "help wanted" since I'm lacking the required fluency and updating the title too.

Thanks!

/cc @bwilkerson

pq avatar Jun 28 '18 20:06 pq

For the curious, Dart regular expressions have the same syntax and semantics as JavaScript regular expressions. Here's the relevant bit of grammar:

http://ecma-international.org/ecma-262/5.1/#sec-15.10.1

pq avatar Jun 28 '18 20:06 pq