Make RegexParseException public

[danmoseley]

Right now when regexes fail to parse, we throw an ArgumentException with a string. However internally we have more information -- the offset into the pattern and in many cases the type of failure eg RegexParseError.ReversedCharRange. These are included in most or all cases in the exception, but only when cast to RegexParseException which is an internal type.

As noted here dotnet/corefx#29178 (comment) and #372 (comment) tooling could potentially use this data eg to squiggle the location in a regex.

(Not apparently @CyrusNajmabadi's Roslyn parser by that's used by VS -- although it obtains the offset by essentially reimplementing the parser, it apparently has other reasons it had to do this.)

If we did this we would have to be comfortable with sometimes changing the offset and/or error for particular invalid patterns, since this may be inevitable when changing and refactoring hte parser. We would not want to consider that a breaking change.

I have no opinion on whether this is worthwhile, just logging to record it.

[stephentoub]

@danmosemsft, unless there are known scenarios demanding this, I suggest it be closed. It's otherwise just going to sit open for a long time.

[danmoseley]

Anyone who thumbed this up want to make a case for having this? If not we will close it.

[abelbraaksma]

@danmosemsft, @stephentoub, actually, yes, I do like to make a strong case for this. I already use the internal error by hacking into it with reflection (it's been a while ago, I'd have to check what I did exactly).

My use case is a compiler and I need to give detailed, and sometimes altered information to the user. That's because the regex he gives is in a slightly different syntax (W3C syntax for XPath 3.1), and it must support surrogate pairs (which are totally unsupported in .NET regexes: try any class, or negated class, it will not properly match surrogate pairs as the single Unicode codepoint it represents). I modify the regex to properly support this (i.e., .{3} gets rewritten such that a surrogate pair is a single char, and much more). In case of errors, I need to translate the position etc back to the original string.

Also, it's just easier to read the detailed exception data and create a customized and localized message, than it is by parsing the returned exception message, which can be disastrous in different locales).

Another use case may be simply that source regexes are given as part of a script (in my case, they're inside XSLT 3.0 stylesheets, for which I've written a compiler), and that you need to calculate the proper offset of the error in the string.

Yet another situation is where a regex appears from a configuration file in, say, XML in an attribute. The user may have put newlines in the regex for readability, but the XML parser will collapse all whitespace for attributes. As with previous examples, in such cases the offset of the error needs to be adjusted to take this into account to give the user proper info.

[abelbraaksma]

If we did this we would have to be comfortable with sometimes changing the offset and/or error for particular invalid patterns, since this may be inevitable when changing and refactoring hte parser. We would not want to consider that a breaking change.

Anyone currently relying on this (like me) already expects that these internals can change. I don't believe, btw, that there is an MS policy that exceptions and their messages or data are required to remain consistent between releases. In the F# team I've seen many changes to existing errors and texts without that being considered breaking.

Of course, the type itself should remain binary compatible, but that shouldn't be too hard.

[danmoseley]

Either of you want to propose the public api? Would it include the enum?

[abelbraaksma]

I can certainly give it a go. At first glance, I think exposing the enum would be fine. We shouldn't be too afraid to add cases to it if future changes warrants that (I think that's within the range of compat guidelines).

[danmoseley]

There is some info about API proposals here https://github.com/dotnet/runtime/blob/master/docs/project/api-review-process.md

[danmoseley]

Before making it public probably most of the work would be to ensure that RegexParseError was always populated. If you look at RegexParserTests.cs, you will see many cases where they verify null, because there was no concerted effort to consistently set a parse error. In fact, this work should probably happen before making the API proposal, as it will help validate that the RegexParseError enum has the right set of values.

[abelbraaksma]

I had a quick look at the code, but I couldn't immediately verify your assessment, so I'll set up a test environment in VS and open the regex solution.

The api review process is clear, thanks.

[danmoseley]

@abelbraaksma my bad: all the error cases seem to set a RegexParseError - in fact, the RegexParseException constructor requires it. I guess you'd still want to satisfy myself that RegexParseError has the best set of values.

[abelbraaksma]

the RegexParseException constructor requires it

I noticed that, so I assumed you meant something like places where an ArgumentException was raised instead, which wouldn't have the internal field set that's used in testing.

The set of values appears pretty exhaustive, but I wouldn't be surprised if, while work is still ongoing on the performance, that new cases emerge for further fine tuning.

We'd also need documentation once the details are clear.

[danmoseley]

@abelbraaksma just to be sure, are you waiting on me, or are you going to write up a brief proposal? no obligation.