Json parsing #42864
Json parsing #42864
Conversation
…/embeddedJson Merge master to features/embeddedJson
…/embeddedJson Merge master to features/embeddedJson
…/embeddedJson Merge master to features/embeddedJson
…/embeddedJson Merge master to features/embeddedJson
…/embeddedJson Merge master to features/embeddedJson
…/embeddedJson Merge master to features/embeddedJson
…/embeddedJson Merge master to features/embeddedJson
…/embeddedJson Merge master to features/embeddedJson
…/embeddedJson Merge master to features/embeddedJson
…/embeddedJson Merge master to features/embeddedJson
…/embeddedJson Merge master to features/embeddedJson
…/embeddedJson Merge master to features/embeddedJson
…/embeddedJson Merge master to features/embeddedJson
…/embeddedJson Merge master to features/embeddedJson
…/embeddedJson Merge master to features/embeddedJson
…/embeddedJson Merge master to features/embeddedJson
…/embeddedJson Merge master to features/embeddedJson
…/embeddedJson Merge master to features/embeddedJson
…/embeddedJson Merge master to features/embeddedJson
…/embeddedJson Merge master to features/embeddedJson
|
@CyrusNajmabadi Would you mind squashing commits? CodeFlow does not handle more than 255 |
I will die. Not kidding. I have several branches off of this. It will kill me to try to squash. @KirillOsenkov can you make codeflow handle more commits plz :( |
|
No worries, I'll deal with github |
src/Features/Core/Portable/EmbeddedLanguages/Json/JsonOptions.cs
Outdated
Show resolved
Hide resolved
src/EditorFeatures/CSharpTest/EmbeddedLanguages/Json/CSharpJsonParserTests_TestGeneration.cs
Outdated
Show resolved
Hide resolved
ryzngard
left a comment
ryzngard
left a comment
There was a problem hiding this comment.
Mostly LGTM. Follow up questions and some suggested changes
| return _regex.Replace(name, "_"); | ||
| } | ||
|
|
||
| //private void Test(string stringText, string expected, bool runJsonNetCheck = true, bool runJsonNetSubTreeTests = true, [CallerMemberName]string name = "") |
There was a problem hiding this comment.
Same here, is this all part of some generation of tests that needs to be uncommented? If so... is there a readme or something about this?
| using JsonTrivia = EmbeddedSyntaxTrivia<JsonKind>; | ||
| using JsonSeparatedList = EmbeddedSeparatedSyntaxNodeList<JsonKind, JsonNode, JsonValueNode>; | ||
|
|
||
| public partial class CSharpJsonParserTests |
There was a problem hiding this comment.
Yea this would be great to have some sort of Readme.md or a giant comment on how this stuff works. It's very hard to follow in a PR :(
It looks really impressive though, and I can confirm that there are a lot of tests.
|
|
||
| private JsonTree ParseTree(bool strict) | ||
| { | ||
| var arraySequence = this.ParseSequence(); |
There was a problem hiding this comment.
nit/style question: Do we now advocate for not using this? Is that a style enforced/recommended anywhere?
There was a problem hiding this comment.
we leave that entirely at the discretion of the dev. i personally use either depending on how i feel about it aesthetically.
| // we then check for any incorrect tree structure (that would be incorrect for both json.net or | ||
| // strict-mode). If we don't run into any problems, we'll then perform specific json.net or strict-mode | ||
| // checks. | ||
| var diagnostic1 = GetFirstDiagnostic(root); |
There was a problem hiding this comment.
Should we avoid computing all diagnostics and instead conditionally computer?
var diagnostic = GetFirstDiagnostic(root);
diagnostic ??= CheckTopLevel(_lexer.Text, root);
diagnostic ??= strict ? ... : ... ;There was a problem hiding this comment.
oh I see, it's still determined based on start location. The comment indicates different than the behavior. First, we ... if not, we ... indicates conditional computation but the code always tries to compute them.
There was a problem hiding this comment.
indeed. comments are bogus. f ixed.
| var sequence = compilationUnit.Sequence; | ||
| if (sequence.IsEmpty) | ||
| { | ||
| // json is not allowed to be just whitespace. |
There was a problem hiding this comment.
but it is allowed to be empty?
There was a problem hiding this comment.
with empty we don't even want to get here (use has just an empty literal, so there are no features we can offer). i explicitly now test for thsi.
src/Features/Core/Portable/EmbeddedLanguages/Json/JsonParser.cs
Outdated
Show resolved
Hide resolved
|
|
||
| private static bool Matches(JsonToken token, string val) | ||
| { | ||
| var chars = token.VirtualChars; |
There was a problem hiding this comment.
I wonder if this should be an extension/method on VirtualChars. I'm pretty sure I had to do similar things for stack parsing.
There was a problem hiding this comment.
if you link me, i can check!
src/Features/Core/Portable/EmbeddedLanguages/Json/JsonParser.cs
Outdated
Show resolved
Hide resolved
| // This approach of keeping track of the parse contexts we're in, and using them to | ||
| // determine if we should consume or pop-out when encountering an error token, mirrors the | ||
| // same approach that we use in the C# and TS/JS parsers. | ||
| private bool _inObject; |
There was a problem hiding this comment.
since I'm using a browser I'm just going to pose this as question since it's hard to verify:
Are there tests for embedded sequences to make sure we still get the behavior we want? [ 1, [ 2, [ 3]] ] and other weird things like [ 1, { 2, [ 3, { 4 }]}] and various permutations of the closing tokens
There was a problem hiding this comment.
there are def tests for the former. but i'll beef both sorts up now.
ghost
added this to the
12 participants
Adds json parsing support for both Ecma404 json, as well as the json extensions supported by json.net.
There will be two followup PRs for this change. One adding all the test, and one adding the IDE feature. They are broken up to make things easier to review.
Tests are in: #42873
Todo: