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

Unnecessary warnings about unsupported regexp flags #3121

Closed
rabbiveesh opened this issue Mar 3, 2024 · 3 comments · Fixed by #3154
Closed

Unnecessary warnings about unsupported regexp flags #3121

rabbiveesh opened this issue Mar 3, 2024 · 3 comments · Fixed by #3154
Labels

Comments

@rabbiveesh
Copy link

Problem

In my perl parser, i use a regex that uses the unicodeSets feature, which requires the v flag.

Generating the parser gives me the following warning, Warning: unsupported flag v, which is spurious, b/c the unicodeSet syntax IS supported and the parser works as expected.

This flag requires node 20 or up, and is not on by default (at least on node 20), so there's no way to use these features without the warning. It's also not feasible to rewrite the regex without the unicodeSets, b/c it would come out very very long + unpleasant.

Steps to reproduce

git clone --depth=1 https://github.com/tree-sitter-perl/tree-sitter-perl
cd tree-sitter-perl
tree-sitter generate

Expected behavior

I would expect it to output no warnings, b/c it does actually work.

Tree-sitter version (tree-sitter --version)

tree-sitter 0.21.0 (1c55abb)

Operating system/version

Linux/Ubuntu 20.04

@rabbiveesh rabbiveesh added the bug label Mar 3, 2024
@WillLillis
Copy link
Contributor

WillLillis commented Mar 4, 2024

I did a little testing and found the code printing the error (Line 171 of cli/src/generate/parse_grammar.rs):

fn parse_rule(json: RuleJSON) -> Rule {
match json {
RuleJSON::ALIAS {
content,
value,
named,
} => Rule::alias(parse_rule(*content), value, named),
RuleJSON::BLANK => Rule::Blank,
RuleJSON::STRING { value } => Rule::String(value),
RuleJSON::PATTERN { value, flags } => Rule::Pattern(
value,
flags.map_or(String::new(), |f| {
f.chars()
.filter(|c| {
if *c == 'i' {
*c != 'u' // silently ignore unicode flag
} else {
eprintln!("Warning: unsupported flag {c}");
false
}
})
.collect()
}),
),

The comment suggests that the unicode flag is expected to be u, instead of v as your unicodeSets source suggests. I'm not sure what's leading to the discrepancy. I'm also a little confused by the if block in the code, as it appears this expression should always evaluate to true when *c == 'i':

if *c == 'i' {
    *c != 'u' // 'i' != 'u'
}

I threw some #[derive(Debug, Clone)]'s on and have the following for a debug print of the RuleJSON object causing the error just to be sure:

LINE: PATTERN { value: "[[_\\p{XID_Start}]--[\\u{b7}\\u{387}\\u{1369}-\\u{1370}\\u{19da}\\u{2118}\\u{212e}]][[\\p{XID_Continue}]--[\\u{b7}\\u{387}\\u{1369}-\\u{1370}\\u{19da}\\u{2118}\\u{212e}]]*", flags: Some("v") }

Should the logic be changed to actually silently ignore the unicode flag? And is that flag 'u' or 'v'? I believe a fix would be relatively straightforward, maybe something like:

flags.map_or(String::new(), |f| {
    f.chars()
        .filter(|c| {
            if *c == 'i' || *c == 'u' /* or *c == 'v' ? */ { // silently ignore unicode flag
                true
            } else {
                eprintln!("Warning: unsupported flag {c}");
                false
            }
        })
        .collect()
}),

Happy to open up a PR if that helps :)

@rabbiveesh
Copy link
Author

rabbiveesh commented Mar 4, 2024 via email

@amaanq
Copy link
Member

amaanq commented Mar 10, 2024

I'm also a little confused by the if block in the code, as it appears this expression should always evaluate to true when *c == 'i':

Whoops good catch, seems to be an oversight that was more obvious to see it was redundant when I applied some aggressive clippy lints (inverted the if statement and swapped the body blocks, the old one had this *c != 'u' in the else block)

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

Successfully merging a pull request may close this issue.

3 participants