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

Provide Basic Syntax Check for Regular Expression Literals #54744

Closed
graphemecluster opened this issue Jun 22, 2023 · 7 comments · Fixed by #55600
Closed

Provide Basic Syntax Check for Regular Expression Literals #54744

graphemecluster opened this issue Jun 22, 2023 · 7 comments · Fixed by #55600
Labels
Duplicate An existing issue was already created

Comments

@graphemecluster
Copy link
Contributor

graphemecluster commented Jun 22, 2023

Edit: Since someone is complaining about the failure to follow issue templates, I copied the content to #3432 (comment). Please continue the discussion in that issue.

Original content

Previously worked on #51837, I found that TypeScript gives almost no syntax errors for regular expressions. I would like to file a PR about it, thus I am opening this issue for easier tracking.

But before I work on the issue, I would like to hear some opinions from the Team. I am not sure, but I suppose the job of providing syntax check should not be shirked to linters right?

Something I would like to do are:

  • Check for duplicated or unknown flags
  • Check for unbalanced parentheses, which is the most common mistake people make
  • Check for invalid escapes
    But this should be done only for RegExps with u or v flag, i.e. in UnicodeMode, that means if we encounter a u or v flag we will need to rescan the whole RegExp again (!!) (i.e. redoing what is done in the current reScanSlashToken method)
    And to check for invalid DecimalEscapes and k<GroupName>s we will also need to count the number of capture groups and record the names of all named capture groups along the way.

Am I doing too much or too less? I know doing too much may cause serious performace regressions (well, luckily regular expression literals are not that common compared with string literals). It should be better than doing nothing after all though.

@MartinJohns
Copy link
Contributor

You forgot to fill out the issue template. Duplicate of #3432.

@RyanCavanaugh RyanCavanaugh added the Duplicate An existing issue was already created label Jun 22, 2023
@graphemecluster
Copy link
Contributor Author

It does look like a duplicate, but this issue does cover something more than #3432.
I just want some feedback from the members before I work on a PR. It’s better than working in vain.
And of course I am not forgetting the issue templates, but none of the templates seem appropriate in this case. I don’t think this kind of things should be made mandatory as long as the cause and effect is clearly conveyed. Team members sometimes don’t use them either.

@fatcerberus
Copy link

fatcerberus commented Jun 22, 2023

I don’t think [the issue templates] should be made mandatory as long as the cause and effect is clearly conveyed.

Nonetheless, they are (note the canned response followed by issue closure). Maintainers will almost invariably ignore issues that don't follow the template.

@graphemecluster
Copy link
Contributor Author

I don’t mean to start a dispute, but this is called Quod licet Iovi, non licet bovi.
Anyway, since comments to issues do not need to follow any templates, I’ve just moved the whole thing to #3432.

@RyanCavanaugh
Copy link
Member

@graphemecluster the reason we ask people to fill out the template is to ensure that people have done their due diligence working through necessary background research. Generally if someone is not willing to do this minimum amount of legwork, they are filing a very low-quality issue, or a duplicate of an existing issue, as occurred here.

@RyanCavanaugh RyanCavanaugh closed this as not planned Won't fix, can't repro, duplicate, stale Jun 23, 2023
@graphemecluster
Copy link
Contributor Author

@RyanCavanaugh I apologize for my faults. I did search for existing things beforehand. To be specific, I firstly searched if there are any PRs related to RegExp. Since no significant PRs exist, I looked over the source code and started to work on it while encountering something to inquiry. So I quickly wrote the issue without other checks. This is the reason why I made the duplication. Please pardon me and comment on #3432.

@RyanCavanaugh
Copy link
Member

No worries, I file dupes sometimes too 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Duplicate An existing issue was already created
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants