Extract out the common portions of the Json and Regex parsing subsystems. by CyrusNajmabadi · Pull Request #25541 · dotnet/roslyn

CyrusNajmabadi · 2018-03-16T21:33:22Z

Followup to #24111 (Extract out the VirtualChar portion of the Regex and JSON editor work).
Precursor to #23984 (regex parser and corresponding tests) and #24110 (json parser and corresponding tests).

Overall map: #24111 (comment)

[jcouv edited]

jcouv · 2018-03-31T17:35:24Z

src/Workspaces/Core/Portable/EmbeddedLanguages/Common/EmbeddedDiagnostic.cs

+        }
+
+        public override bool Equals(object obj)
+            => obj is EmbeddedDiagnostic rd && Equals(rd);


Nit: why rd? I'd expect ed or d

Yeah, this used to be RegexDiagnostic before i made it EmbeddedDiagnostic. Will fix :)

jcouv · 2018-03-31T17:46:06Z

src/Workspaces/Core/Portable/EmbeddedLanguages/Common/EmbeddedSyntaxNode.cs

+
+        public bool Contains(VirtualChar virtualChar)
+        {
+            foreach (var child in this)


Nit: this is a bit odd. Why not simply do a for loop and forego the enumerator?
Is the enumerator used elsewhere?

yeah. we use the enumerator later on when we develop features. Note: we tehcnically could forgo the enumerator and just do the for-loop. But this is similar to how we walk normal SyntaxNodes in the IDE layer and it felt cleaner so that i would not be exposing the internal details of child-counts and slot lookups and whatnot.

jcouv · 2018-03-31T21:14:49Z

src/Workspaces/Core/Portable/EmbeddedLanguages/Common/EmbeddedSyntaxToken.cs

+            Value = value;
+        }
+
+        public bool IsMissing => VirtualChars.IsEmpty;


Is it safe to call .IsEmpty on a default ImmutableArray? Same question for .Length below.

In a SyntaxToken none of these will be empty. I will add checks for that in the constructor.

jcouv · 2018-03-31T21:18:51Z

src/Workspaces/Core/Portable/EmbeddedLanguages/Common/EmbeddedSyntaxToken.cs

+
+        public bool IsMissing => VirtualChars.IsEmpty;
+
+        public EmbeddedSyntaxToken<TSyntaxKind> AddDiagnosticIfNone(EmbeddedDiagnostic diagnostic)


~~It looks like this method is not used or tested. If this method is removed, then WithDiagnostics can probably be removed too and possibly With as well.~~

Update: Hum, I just realized the tests are not in this PR... Can the relevant tests be moved into this PR along with the code change?

The tests are in another PR because they destroy github. You can see the tests for regex and json in these PRs:

#25520

I would prefer to keep them separate.

Also note: this is hte common embedded languages subsystem part of the work. There are no explicit tests for this part as this is just an implementation detail of hte final regex parser/features.

jcouv · 2018-03-31T21:23:02Z

src/Workspaces/Core/Portable/EmbeddedLanguages/Common/EmbeddedSyntaxToken.cs

+        public readonly ImmutableArray<VirtualChar> VirtualChars;
+        public readonly ImmutableArray<EmbeddedSyntaxTrivia<TSyntaxKind>> TrailingTrivia;
+        internal readonly ImmutableArray<EmbeddedDiagnostic> Diagnostics;
+        public readonly object Value;


It's not obvious to me what the Value is for. Consider commenting.

jcouv · 2018-03-31T21:26:47Z

src/Workspaces/Core/Portable/EmbeddedLanguages/Common/EmbeddedSyntaxTree.cs

+    internal abstract class EmbeddedSyntaxTree<TSyntaxKind, TNode, TRoot>
+        where TSyntaxKind : struct
+        where TNode : EmbeddedSyntaxNode<TSyntaxKind, TNode>
+        where TRoot : TNode


Is there a scenario where TRoot is not literally TNode?

Yes. TRoot is never literally TNode. For regex and json it will be RegexCompilationUnit and JsonCompilationUnit respectively.

Renamed to be clearer. #resolved.

jcouv · 2018-03-31T21:57:35Z

src/Workspaces/Core/Portable/EmbeddedLanguages/Common/EmbeddedSyntaxToken.cs

+        internal readonly ImmutableArray<EmbeddedDiagnostic> Diagnostics;
+        public readonly object Value;
+
+        public EmbeddedSyntaxToken(


From usage in subsequent PR, it seems that you never put default arrays for those parameters (but rather use empty arrays). Perhaps assert that immutable arrays in various types are not default?

correct. i will do that.

CyrusNajmabadi · 2018-04-01T21:21:35Z

@jcouv Have addressed all feedback.

jcouv

LGTM Thanks

jcouv · 2018-04-01T21:51:59Z

Don't forget to retarget the PR to feature branch.

jcouv · 2018-04-01T21:55:35Z

Also, any plans to implement adapt the Roslyn C# parser on top of VirtualChar so we can properly highlight embedded C#? ;-)

CyrusNajmabadi · 2018-04-01T22:11:14Z

@jcouv Have rebased to the new feature branch.

@dotnet/roslyn-ide could i get a quick pair of eyes on this small 300 line PR relating to the regex/json features?

CyrusNajmabadi · 2018-04-01T22:12:38Z

Also, any plans to implement adapt the Roslyn C# parser on top of VirtualChar so we can properly highlight embedded C#? ;-)

Yes! Definitely. I also want ot make it work for the special strings we use for C# in our tests :)

Note: i don't think this will actually be difficult. I don't need to make the C# parser run on top fo virtual chars. I just need to feed it the .ValueText of the token. Then, any spans it passes back, i will map back myself using VirtualChar to the actual span.

CyrusNajmabadi · 2018-04-01T22:13:24Z

@jcouv If you feel pretty good aobut this one, we can merge in as well.

jcouv · 2018-04-02T04:27:23Z

I think PRs into feature branches follow the same process as other PRs, so they can be merged back into master without review of a massive PR.
@jmarolf @dotnet/roslyn-ide for a second review. Thanks

CyrusNajmabadi · 2018-04-02T17:06:23Z

I think PRs into feature branches follow the same process as other PRs, so they can be merged back into master without review of a massive PR.
@jmarolf @dotnet/roslyn-ide for a second review. Thanks

Makes sense to me.

jmarolf · 2018-04-02T17:20:29Z

src/Workspaces/Core/Portable/EmbeddedLanguages/Common/EmbeddedSyntaxNode.cs

+            return TextSpan.FromBounds(start, end);
+        }
+
+        private void GetSpan(ref int start, ref int end)


Seems like this could be a local function or this logic just get rolled up into GetSpan(). Is there a reason we want a separate private method here?

tried it out. Didn't think it helped. Plus, either you have the locals conflicting with the params, or hte local function mutates outer state. I didn't like either :)

jmarolf

CyrusNajmabadi · 2018-04-02T17:59:38Z

@jcouv or @jmarolf to merge in.

CyrusNajmabadi added 11 commits March 16, 2018 11:06

Move VirtualChar code into a new namespace.

1b35d32

Add common code.

e6c25f1

Add common code.

4a2a34d

Add common code.

94736b8

Add common code.

2970897

Move downstream files.

6a50fef

Add helpers.

14f4788

Make kind part of the siganture.

be593fc

Use TSyntaxKind in trivia as well.

f1882f7

remove unnecessary constructors.

d937606

Move helpers to instance methods.

1262584

CyrusNajmabadi requested a review from a team as a code owner March 16, 2018 21:33

CyrusNajmabadi mentioned this pull request Mar 16, 2018

Extract out the VirtualChar portion of the Regex and JSON editor work. #24111

Merged

Merge branch 'virtualChars' into embeddedLanguages

a8ea100

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 branch 'virtualChars' into embeddedLanguages

9100e63

jcouv reviewed Mar 31, 2018

View reviewed changes

CyrusNajmabadi added 3 commits March 31, 2018 11:44

Merge branch 'virtualChars' into embeddedLanguages

3566c41

Rename variable.

62beab8

Make members internal.

62690d4

jcouv reviewed Mar 31, 2018

View reviewed changes

jcouv added the Area-IDE label Mar 31, 2018

jcouv reviewed Mar 31, 2018

View reviewed changes

CyrusNajmabadi added 3 commits April 1, 2018 14:17

Add comment.

9f049cd

Add checks

a1d4a91

Rename type parametres.

e60b10d

jcouv approved these changes Apr 1, 2018

View reviewed changes

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

jmarolf reviewed Apr 2, 2018

View reviewed changes

jmarolf approved these changes Apr 2, 2018

View reviewed changes

jmarolf merged commit 68daa3f into dotnet:features/embeddedRegex Apr 2, 2018

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

This was referenced Apr 4, 2018

[Feature] Language service for embedded JSON #25939

Closed

[Feature] Language service for embedded Regex #25940

Closed

CyrusNajmabadi mentioned this pull request Jul 28, 2018

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

Merged


		public bool IsMissing => VirtualChars.IsEmpty;

		public EmbeddedSyntaxToken<TSyntaxKind> AddDiagnosticIfNone(EmbeddedDiagnostic diagnostic)

Conversation

CyrusNajmabadi commented Mar 16, 2018 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

jcouv Mar 31, 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!

jcouv Mar 31, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Apr 1, 2018

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

jcouv commented Apr 1, 2018

Uh oh!

jcouv commented Apr 1, 2018

Uh oh!

CyrusNajmabadi commented Apr 1, 2018

Uh oh!

CyrusNajmabadi commented Apr 1, 2018

Uh oh!

CyrusNajmabadi commented Apr 1, 2018

Uh oh!

jcouv commented Apr 2, 2018

Uh oh!

CyrusNajmabadi commented Apr 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmarolf left a comment

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Apr 2, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

CyrusNajmabadi commented Mar 16, 2018 •

edited by jcouv

Loading

jcouv Mar 31, 2018 •

edited

Loading

jcouv Mar 31, 2018 •

edited

Loading