Extract out the VirtualChar portion of the Regex and JSON editor work. by CyrusNajmabadi · Pull Request #24111 · dotnet/roslyn

CyrusNajmabadi · 2018-01-09T00:50:31Z

From the #23984 PR:

The first subsystem is called the VirtualCharService deals with the following issue. To the final .net regex, the following snippets of code appear completely identical to it:

"\\1"       // In a normal string, we have to escape the \
@"\1"       // But not in a verbatim string
"\\\u0031"  // The '1' could be escaped
"\\u005c1"  // Even the backslash *itself* may be escaped

These are all ways of writing the \1 regex.

In other words, C# allows a wide variety of input strings to all compile down to the same final 'value' (or 'ValueText') that the regex engine will finally see. This is a major issue as it means that any data reported by the regex engine must be accurate with respect to the text as the user wrote it. For example, in all of the equivalent cases above, there is the same error "Reference to undefined group number 1". However, for each form the user wrote, it's necessary to understand what the right value is to highlight as the problem. i.e.

and

So, the purpose of the VirtualCharService is to translate all of the above pieces of user code to the same final set of characters the regex engine will see (specifically \ and 1) while also maintaining the knowledge of where those characters came from (for example, that 1 came from \u0031 in the last example). In essence, the VirtualCharService is able to produce the ValueText for any string literal, while having a mapping back from each character in the ValueText back to the original source span of the document that formed this.

With the VirtualCharService user code can be translated into a common format that then can be processed uniformly. This means that the part of the system that actually tries to understand the regex does not need to know about the differences between @"" and "" strings, or the differences between C# and VB. It also means that it can be used by any roslyn language (for example, F#) if that is so desired.

CyrusNajmabadi · 2018-01-09T00:51:08Z

@jasonmalinowski Pulled out 700 lines. That makes the other PR only 37k large. :D

CyrusNajmabadi · 2018-01-09T00:56:34Z

Tagging @dotnet/roslyn-ide . Review plz.

CyrusNajmabadi · 2018-01-09T04:03:14Z

Also, i'll likely refactor the regex and json PRs to pull out the tests into their own PR. That will help make the review more manageable (and less painful in the browser).

AdamSpeight2008 · 2018-01-09T18:43:27Z

Personally I'd like to see Document comments on all of the methods, both public and internal ones.

CyrusNajmabadi · 2018-01-09T22:32:19Z

@AdamSpeight2008 Anything in particular? My personal approach is to put on doc comments when i think it will provide substantive value for the consumer or maintainer of the code. If there's areas you feel would benefit, i'd be happy to add. But i'm not really going to take an approach of just adding doc comments to everything.

AdamSpeight2008 · 2018-01-10T11:00:03Z

src/Workspaces/CSharp/Portable/VirtualChars/CSharpVirtualCharService.cs

+            return (c >= '0' && c <= '9') ? c - '0' : (c & 0xdf) - 'A' + 10;
+        }
+
+        private static bool IsHexDigit(char c)


Doesn't cover fullwidth characters that are valid hex character in VB.net

This is the C# impl.

My apologise, thought it was in the base class used for both, ie the VirtualCharService

AdamSpeight2008 · 2018-01-10T11:09:08Z

src/Workspaces/Core/Portable/VirtualChars/AbstractVirtualCharService.cs

+    {
+        protected abstract ImmutableArray<VirtualChar> TryConvertToVirtualCharsWorker(SyntaxToken token);
+
+        public ImmutableArray<VirtualChar> TryConvertToVirtualChars(SyntaxToken token)


@CyrusNajmabadi
Add a doc comment, so the consumer understands what aspect of the syntax node is being worked on. eg. Is it the actual text from source, or value text, or displayed text?

Comment added to IVirtualCharService.

AdamSpeight2008 · 2018-01-10T11:13:04Z

src/Workspaces/Core/Portable/VirtualChars/VirtualCharExtensions.cs

+        {
+            var builder = PooledStringBuilder.GetInstance();
+
+            foreach (var vc in chars)


Wouldn't the following remove an instance of the enumerator.

for(var idx = 0; idx < chars.length; idx++) { builder.Builder.Append(chars[idx].char); }.

the enumerator is a struct. so removing it is unnecessary.

AdamSpeight2008 · 2018-01-10T11:16:34Z

src/Workspaces/Core/Portable/VirtualChars/VirtualChar.cs

+        {
+            if (span.IsEmpty)
+            {
+                throw new ArgumentException();


Use ArguementOutOfRangeException( $"{ nameof( span ) } was expected to be non empty.") or at least add which parameter it is.

AdamSpeight2008 · 2018-01-10T11:19:12Z

src/Workspaces/CSharp/Portable/VirtualChars/CSharpVirtualCharService.cs

+            return true;
+        }
+
+        private bool TryAddComplexEscape(


Add a <summary> giving a quick overview of what a "complex escape" is.

Sure. I can doc that.

Changed the name to be something liek TryAddSingleCharEscape and TryAddMultiCharEscape.

AdamSpeight2008 · 2018-01-10T11:29:16Z

@CyrusNajmabadi I've added a few comments, mainly around clarifying usage.
A simple <summary> on methods, giving a quick summary of what its purpose is, would help the consumer determine if that method is the one need. Intellisense discovery and exploration

AdamSpeight2008 · 2018-01-11T13:25:23Z

src/Workspaces/Core/Portable/VirtualChars/VirtualChar.cs

+namespace Microsoft.CodeAnalysis.VirtualChars
+{
+    /// <summary>
+    /// The Regex parser wants to work over an array of characters, however this array of characters


Maybe the references to the Regex parser should be altered, as VirtualChar is know independent for the other pull request?

AdamSpeight2008 · 2018-01-11T13:26:03Z

@CyrusNajmabadi Thank you that much better.

CyrusNajmabadi · 2018-01-11T21:00:24Z

@jmarolf @dotnet/roslyn-ide Anyone want to take a looksie? Thanks!

jmarolf · 2018-01-11T21:07:33Z

src/Workspaces/Core/Portable/VirtualChars/IVirtualCharService.cs

+        /// multiple chars, <see langword="default"/> will also be returned in those cases. All these
+        /// cases could be relaxed in the future.  But they greatly simplify the implementation.
+        /// </summary>
+        ImmutableArray<VirtualChar> TryConvertToVirtualChars(SyntaxToken token);


I am normally fine with the pattern of returning null for failure cases but returning a default for a struct seems odd. Perhaps use optional to indicate whether the conversion succeeded or failed?

I'm very torn about this issue with ImmutableArray's. I tried going that route one time and it felt pretty weird given that ImmutableArray then still has a state needed to check for. Given that ImmutableArray<T> is really just a nicer form of T[] and checking T[] against default is pretty sane, i'm not sure i want to change this. But if you feel strongly, i can.

It stood out as odd to me on first pass. I don't feel too strongly about it, though. ImmutableArray is a weird class so if we just call out that we return default for error cases in the doc comments I think I'm satisfied.

I'll leave as is. I think the name (TryXXX) and docs explain things acceptably well :)

CyrusNajmabadi · 2018-03-16T23:02:25Z

@jcouv Could you take a look if you have time? Tnx!

jcouv · 2018-03-31T04:29:35Z

src/Workspaces/Core/Portable/VirtualChars/VirtualChar.cs

+    /// ( '\' and 'z' ) back to the  ranges of characters the user wrote.  
+    /// 
+    /// VirtualChar serves this purpose.  It contains the interpretted value of any language character/
+    /// character-escape-sequence, as well as the original SourceText span where that interpretted 


Typo: interpreted

jcouv · 2018-03-31T04:35:59Z

src/Workspaces/Core/Portable/VirtualChars/AbstractVirtualCharService.cs

+                    }
+
+                    var lastVC = result.Last();
+                    Debug.Assert(lastVC.Span.End == token.Span.End - 1, "Last span has to end right before the end of hte string token (including its trailing delimeter).");


jcouv · 2018-03-31T04:37:10Z

src/Workspaces/Core/Portable/VirtualChars/AbstractVirtualCharService.cs

+        /// Helper to convert simple string literals that escape quotes by doubling them.  This is 
+        /// how normal VB literals and c# verbatim string literals work.
+        /// </summary>
+        protected ImmutableArray<VirtualChar> TryConvertSimpleDoubleQuoteString(


jcouv · 2018-03-31T04:38:56Z

src/Workspaces/Core/Portable/VirtualChars/AbstractVirtualCharService.cs

+        {
+            Debug.Assert(!token.ContainsDiagnostics);
+
+            var tokenText = token.Text;


Why .Text here, but .ValueText above?

Very intentional. We use .ValueText because we want to know that the interpretted VirtualChar chars sum up to the .ValueText. However, we want to use .Text here becaue we want to ensure that the uninterpretted VirtualChar spans completely span it and hold for certain invariants.

In a very real sense VirtualChar is the bridge API between .Text and .ValueText. The Spans of the VirtualChars come from us breaking up .Text into pieces. And each VirtualChar has a char that is then each char of .ValueText in order.

Does that make sense?

Makes sense. Thanks for the explanation

@CyrusNajmabadi Could you document this in the code, so that future you can remember,

Added to docs on hte interface.

jcouv · 2018-03-31T04:41:22Z

src/Workspaces/Core/Portable/VirtualChars/AbstractVirtualCharService.cs

+        /// how normal VB literals and c# verbatim string literals work.
+        /// </summary>
+        protected ImmutableArray<VirtualChar> TryConvertSimpleDoubleQuoteString(
+            SyntaxToken token, string startDelimeter, string endDelimeter)


Typo: startDelimiter and endDelimiter

Consider documenting the delimiter parameters.

jcouv · 2018-03-31T05:44:10Z

src/Workspaces/CSharp/Portable/VirtualChars/CSharpVirtualCharService.cs

+                    {
+                        if (!TryAddEscape(result, tokenText, offset, index))
+                        {
+                            return default;


~~Should we release the result array builder here?~~
Never mind :-)

jcouv · 2018-03-31T05:44:34Z

src/Workspaces/CSharp/Portable/VirtualChars/CSharpVirtualCharService.cs

+                    }
+                }
+
+                return result.ToImmutable();


~~.ToImmutableAndFree()~~
Never mind ;-)

jcouv · 2018-03-31T06:10:57Z

src/Workspaces/VisualBasicTest/VirtualChars/VisualBasicVirtualCharServiceTests.vb

+            Return "'" + c + "'"
+        End Function
+    End Class
+End Namespace


Since the whole feature is split into a few PRs, it feels like this should go into a feature branch.

sure. i can do that.

jcouv · 2018-03-31T06:22:56Z

src/Workspaces/CSharp/Portable/VirtualChars/CSharpVirtualCharService.cs

+
+        private ImmutableArray<VirtualChar> TryConvertStringToVirtualChars(SyntaxToken token)
+        {
+            const string StartDelimeter = "\"";


More places with typo for "delimiter"

jcouv

LGTM modulo remaining "delimiter" spelling.
Check with IDE team on creating a feature branch (since multiple PRs for this feature).

CyrusNajmabadi · 2018-03-31T18:47:07Z

@jcouv I can't make a branch in roslyn. Could you make a branch for me off of master called "features/embeddedRegex"? I can then make PRs against that. Thanks!

CyrusNajmabadi · 2018-04-01T20:38:13Z

@jcouv This now targets the new feature branch. This PR has two signoffs, so can you merge in?

Add the VirtualChar component.

78e18a2

CyrusNajmabadi requested a review from a team as a code owner January 9, 2018 00:50

CyrusNajmabadi added 6 commits January 8, 2018 16:57

Remove unused field.

6758c70

Make it possible to convert to a string without linq.

b7f9a8d

Use unchecked math.

71cf5ac

Share source.

7005196

Add comment

39cb76f

Fix method name.

83b86a0

sharwell self-requested a review January 9, 2018 15:53

Add asserts.

7e9210d

AdamSpeight2008 reviewed Jan 10, 2018

View reviewed changes

CyrusNajmabadi added 3 commits January 10, 2018 13:40

Add comment.

0c1b2a3

Add error message.

c67093a

Extract methods to make names clearer.

a5319a1

AdamSpeight2008 reviewed Jan 11, 2018

View reviewed changes

CyrusNajmabadi added 2 commits January 11, 2018 11:17

Merge remote-tracking branch 'upstream/master' into virtualChars

3e4bd65

Don't just refer to Regex.

7686bb5

jmarolf reviewed Jan 11, 2018

View reviewed changes

Simplify.

53de0a7

CyrusNajmabadi mentioned this pull request Mar 21, 2018

Add in a regex parser so that the IDE can provide services around regex editing. #23984

Merged

19 tasks

Merge remote-tracking branch 'upstream/master' into virtualChars

a5b725d

jcouv reviewed Mar 31, 2018

View reviewed changes

CyrusNajmabadi added 6 commits March 30, 2018 22:13

Merge branch 'master' into virtualChars

b9ba4d9

Spelling

3bd7a6d

Simplify contract

d978545

Rename.

30dc32e

Fix spelling.

68ecac0

Indent switches

e9748c0

jcouv reviewed Mar 31, 2018

View reviewed changes

jcouv approved these changes Mar 31, 2018

View reviewed changes

Adding comment.

47f6346

jcouv added the Area-IDE label Apr 1, 2018

CyrusNajmabadi changed the base branch from master to features/embeddedRegex April 1, 2018 20:37

jcouv merged commit e60c450 into dotnet:features/embeddedRegex Apr 1, 2018

CyrusNajmabadi deleted the virtualChars branch April 1, 2018 22:09

CyrusNajmabadi mentioned this pull request Apr 4, 2018

Add in a json parser so that the IDE can provide services around json editing. #24110

Closed

CyrusNajmabadi mentioned this pull request Jul 28, 2018

Add support for colorizing escape sequences in string literals. #28927

Merged

AdamSpeight2008 mentioned this pull request Aug 6, 2018

Annotated Text for C# and VB dotnet/csharplang#1775

Closed

Conversation

CyrusNajmabadi commented Jan 9, 2018

Uh oh!

CyrusNajmabadi commented Jan 9, 2018

Uh oh!

CyrusNajmabadi commented Jan 9, 2018

Uh oh!

CyrusNajmabadi commented Jan 9, 2018

Uh oh!

AdamSpeight2008 commented Jan 9, 2018

Uh oh!

CyrusNajmabadi commented Jan 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AdamSpeight2008 Jan 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Jan 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AdamSpeight2008 commented Jan 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AdamSpeight2008 commented Jan 11, 2018

Uh oh!

CyrusNajmabadi commented Jan 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Mar 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

AdamSpeight2008 Jan 10, 2018 •

edited

Loading

CyrusNajmabadi Jan 10, 2018 •

edited

Loading

jcouv Mar 31, 2018 •

edited

Loading

jcouv Mar 31, 2018 •

edited

Loading

jcouv Mar 31, 2018 •

edited

Loading