Obsolete some protected members of Regex{Runner}

[stephentoub]

Regex and RegexRunner have a relatively large protected surface area in support of the old CompileToAssembly, which is now obsolete. Some of that functionality is used by code generated by the source generator, but some of the surface area is completely defunct, and some was never used (who knows why it was exposed initially).

namespace System.Text.RegularExpressions;

public class Regex
{
+   [Obsolete(...)]
    protected void InitializeReferences();

+   [Obsolete(...)]
    protected bool UseOptionC()

+   [Obsolete(...)]
    protected bool UseOptionR();
}

public abstract class RegexRunner
{
+   [Obsolete(...)]
    protected Match? Scan(Regex regex, string text, int textbeg, int textend, int textstart, int prevlen, bool quick) => Scan(regex, text, textbeg, textend, textstart, prevlen, quick, regex.MatchTimeout);

+   [Obsolete(...)]
    protected internal Match? Scan(Regex regex, string text, int textbeg, int textend, int textstart, int prevlen, bool quick, TimeSpan timeout)

+   [Obsolete(...)]
    protected static bool CharInSet(char ch, string set, string category);
}

I would like to also make that last CharInSet not only obsolete but also throw new NotSupportedException(). This was never used by the implementation, and we've never generated assets that use whatever format it's trying to support.

[ghost]

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

Issue Details

---

Regex and RegexRunner have a relatively large protected surface area in support of the old CompileToAssembly, which is now obsolete. Some of that functionality is used by code generated by the source generator, but some of the surface area is defunct (at least for external consumption... some is still used internally). We should consider obsoleting:
RegexRunner.CharInSet
RegexRunner.Crawl
RegexRunner.DoubleCrawl
RegexRunner.DoubleStack
RegexRunner.DoubleTrack
RegexRunner.EnsureStorage
RegexRunner.Scan
Regex.UseOptionC
Regex.UseOptionR

Author:	stephentoub
Assignees:	-
Labels:	area-System.Text.RegularExpressions
Milestone:	7.0.0

[GrabYourPitchforks]

If there's any way to remove the RegexRunner.runtext* fields, that would help as well. I've been looking at possible codegen optimizations within the Regex type itself, but they're stymied by the fact that those fields are accessible by external code and thus can't be refactored in a way that would improve throughput.

[stephentoub]

If there's any way to remove the RegexRunner.runtext* fields, that would help as well.

Any in particular?

These are used by external Regex implementations, those created by CompileToAssembly and the source generator. Only some of them are used by the source generated implementations, but even if we found a different way to pass that state in and around, removing these fields would break any existing assemblies generated by CompileToAssembly.

That said, CompileToAssembly is obsoleted as of .NET 7, so we're talking about compat with assemblies generated on .NET Framework. At some point in the future, we could consider dropping that compat support.

We contemplated having the new Scan virtual method accept and return all the necessary state, but I'm not sure that would be a net win. It would also be complicated, e.g. various helpers like Capture need access to it.

Suggestions?

[GrabYourPitchforks]

Suggestions?

Honestly, I don't know. I'm mostly lamenting that the int fields are true fields rather than properties, since it's preventing me from moving the actual storage to a more optimal location. Maybe it's one of those things that we just accept as overhead for now and revisit if we ever make more radical changes to Regex in the future.

[stephentoub]

@joperezr, were you going to push on this for .NET 7?

[joperezr]

Sorry @stephentoub I missed the ping. Unless you think otherwise, since this is just obsoleting I'm fine with doing this on 8.0

[ghost]

Added needs-breaking-change-doc-created label because this issue has the breaking-change label.

1. Create and link to this issue a matching issue in the dotnet/docs repo using the breaking change documentation template, then remove this needs-breaking-change-doc-created label.

Tagging @dotnet/compat for awareness of the breaking change.

[bartonjs]

Video

Because the proposal mentions removing the body of CharInSet, I've marked it as a breaking change.

Since CharInSet is having its body removed, it probably warrants obsolete-as-error, the rest being obsolete-as-warning.

Obsoletions should just use the next available SYSLIB code. (The same code can apply to all of them, even though it's mixing warning and error)

EditorBrowsable-Never is also fine if it makes you happy.

namespace System.Text.RegularExpressions;

public class Regex
{
+   [Obsolete(...)]
    protected void InitializeReferences();

+   [Obsolete(...)]
    protected bool UseOptionC()

+   [Obsolete(...)]
    protected bool UseOptionR();
}

public abstract class RegexRunner
{
+   [Obsolete(...)]
    protected Match? Scan(Regex regex, string text, int textbeg, int textend, int textstart, int prevlen, bool quick) => Scan(regex, text, textbeg, textend, textstart, prevlen, quick, regex.MatchTimeout);

+   [Obsolete(...)]
    protected internal Match? Scan(Regex regex, string text, int textbeg, int textend, int textstart, int prevlen, bool quick, TimeSpan timeout)

+   [Obsolete(...)]
    protected static bool CharInSet(char ch, string set, string category);
}