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

Fix regex fixer to maintain string literal syntax #78172

Merged
merged 3 commits into from
Nov 11, 2022

Conversation

stephentoub
Copy link
Member

Fixes #78113

@ghost
Copy link

ghost commented Nov 10, 2022

Tagging subscribers to this area: @dotnet/area-system-text-regularexpressions
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #78113

Author: stephentoub
Assignees: -
Labels:

area-System.Text.RegularExpressions

Milestone: -

Copy link
Member

@Youssef1313 Youssef1313 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@joperezr
Copy link
Member

Unless I'm missing something, this will generate code that won't compile sometimes. For example, the case of:

using System.Text.RegularExpressions;

partial class Program
{
    static void Main(string[] args)
    {
        const string pattern = @"a|b\s\n";
        const string pattern2 = $"{pattern}2";

        Regex regex = new Regex(pattern2);
    }
}

If I'm following correctly, this will generate the new static method and pass in pattern2 as the argument to the GeneratedRegex attribute, which is not in scope so code won't compile. This is why today we instead have to evaluate the constant value and then use that as the argument to the attribute. If I'm correct with my assumption here, I'm not sure why we don't have a test that checks that, but we should add one if we don't.

@joperezr
Copy link
Member

joperezr commented Nov 10, 2022

Just to confirm, I just tried out the sample I had above locally against your changes and it indeed produces a compile error:

image

@stephentoub
Copy link
Member Author

if we don't

We don't :)

Copy link
Member

@joperezr joperezr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this @stephentoub and thanks for the help @Youssef1313!

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

Successfully merging this pull request may close these issues.

SYSLIB1045 Use GeneratedRegexAttribute replaces verbatim string with regular string
3 participants