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

valid_regexps should warn about string literal #1049

Open
alescdb opened this issue Jun 28, 2018 · 9 comments
Open

valid_regexps should warn about string literal #1049

alescdb opened this issue Jun 28, 2018 · 9 comments
Labels
contributions-welcome Contributions welcome to help resolve this (the resolution is expected to be clear from the issue) false-negative P3 A lower priority bug or feature request set-core Affects a rule in the core Dart rule set type-enhancement A request for a change that isn't a bug

Comments

@alescdb
Copy link

alescdb commented Jun 28, 2018

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

ref : dart-lang/sdk#33669

@zoechi
Copy link

zoechi commented Jun 28, 2018

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

@pq
Copy link
Member

pq commented Jun 28, 2018

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!

@zoechi
Copy link

zoechi commented Jun 28, 2018

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

@pq
Copy link
Member

pq commented Jun 28, 2018

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...

@alescdb
Copy link
Author

alescdb commented Jun 28, 2018

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 ?

@pq
Copy link
Member

pq commented Jun 28, 2018

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 pq added lint-request contributions-welcome Contributions welcome to help resolve this (the resolution is expected to be clear from the issue) labels Jun 28, 2018
@alescdb
Copy link
Author

alescdb commented Jun 28, 2018

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, ...)

@pq
Copy link
Member

pq commented Jun 28, 2018

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 pq added type-enhancement A request for a change that isn't a bug and removed lint-request labels Jun 28, 2018
@pq
Copy link
Member

pq commented Jun 28, 2018

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 pq added the set: core label Jun 30, 2021
@pq pq added set-core Affects a rule in the core Dart rule set and removed set: core labels Jul 18, 2022
@srawlins srawlins added the P3 A lower priority bug or feature request label Sep 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contributions-welcome Contributions welcome to help resolve this (the resolution is expected to be clear from the issue) false-negative P3 A lower priority bug or feature request set-core Affects a rule in the core Dart rule set type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

4 participants