Add JSON Language Service features to the IDE.#25518
Add JSON Language Service features to the IDE.#25518CyrusNajmabadi wants to merge 194 commits intodotnet:masterfrom
Conversation
|
|
||
| public bool IsProbablyJson(SyntaxToken token, CancellationToken cancellationToken) | ||
| { | ||
| if (IsDefinitelyNotJson(token, _syntaxFacts)) |
There was a problem hiding this comment.
I think this is redundant. TryParseJson also checks
There was a problem hiding this comment.
good catch. removed.
| SyntaxToken stringLiteral, SyntaxNode argumentNode, | ||
| CancellationToken cancellationToken) | ||
| { | ||
| var parameter = _semanticFacts.FindParameterForArgument(_semanticModel, argumentNode, cancellationToken); |
There was a problem hiding this comment.
It looks like _semanticFacts is only used in this method. Consider passing in as a parameter.
There was a problem hiding this comment.
I prefer this approach. All the data that stays constant for the Document is held as fields of the class, and the only data that is passed in is the stuff that can actually vary from call to call.
| } | ||
|
|
||
|
|
||
| public bool IsProbablyJson(SyntaxToken token, CancellationToken cancellationToken) |
There was a problem hiding this comment.
This method doesn't seem to be used.
My bad. I had FAR window scoped to current project.
Nit: extra empty line above
There was a problem hiding this comment.
It is used in: AbstractJsonStringDetectorDiagnosticAnalyzer
There we check if it looks like it's probably json, and doesn't have any markers that indicate it's definitely json (i.e. it's not "JToken.Parse"). In that case, we offer to add the // lang=json comment to indicate that we should light-up language services here.
|
|
||
| HasJsonLanguageComment(token, _syntaxFacts, out var strict); | ||
|
|
||
| var chars = _virtualCharService.TryConvertToVirtualChars(token); |
There was a problem hiding this comment.
_virtualCharService is only used here. I think it could be passed as parameter instead.
One of the two callers is an unused method. The other caller can easily pass that service through.
There was a problem hiding this comment.
Same reason as above. i would prefer the only things passed in to be the information that varies. The data tthat is fixed (i.e. the semantic model, services, etc.) i would like to all be kept together and only be provided once.
| ISemanticFactsService semanticFacts, | ||
| IVirtualCharService virtualCharService) | ||
| { | ||
| // Do a quick non-allocating check first. |
There was a problem hiding this comment.
Once the type is trimmed down, would it be better to make it a struct and simply create a new struct whenever needed (without a cache)?
Never mind, we need the cached "types of interest"...
There was a problem hiding this comment.
Yup. Thta's why i went with thsi pattern.
| var name = GetNameOfInvokedExpression(invokedExpression); | ||
| if (_syntaxFacts.StringComparer.Equals(name, _methodNameOfInterest)) | ||
| { | ||
| // Is a string argument to a method that looks like it could be a Regex method. |
| return false; | ||
| } | ||
|
|
||
| public JsonTree TryParseJson(SyntaxToken token, CancellationToken cancellationToken) |
There was a problem hiding this comment.
I think this method is nice. It brings all the pieces together. I like it :-)
| return JsonParser.TryParse(chars, strict); | ||
| } | ||
|
|
||
| private bool AnalyzeStringLiteral( |
There was a problem hiding this comment.
Method name is odd. Maybe "ParameterNameIsJson"?
| } | ||
| #endregion | ||
|
|
||
| #region JSON |
There was a problem hiding this comment.
This PR should only have JSON changes, but has Regex changes too.
| @@ -1,5 +1,6 @@ | |||
| // Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | |||
|
|
|||
| using System; | |||
There was a problem hiding this comment.
This change can probably be reverted.
| using Microsoft.CodeAnalysis.Diagnostics; | ||
| using Microsoft.CodeAnalysis.ValidateJsonString; | ||
|
|
||
| namespace Microsoft.CodeAnalysis.CSharp.ValidateJsonString |
There was a problem hiding this comment.
This file is for Json, but the file name says "Regex"
| // diagnostic for when we detect a string that should have /*language=json*/ added to it. | ||
| public const string JsonDetectionDiagnosticId = "IDE0046"; | ||
| // diagnostic for when we have a known json string and we detect something wrong with it. | ||
| public const string JsonPatternDiagnosticId = "IDE0047"; |
There was a problem hiding this comment.
I wasn't able to get squiggles for bad Json in the IDE, with this error code. Do I need to turn some option on?
Update: I was able to see squiggles in string s = /*lang=json*/ "{a: 1, b: new()}"; under the open paren. But it's barely visible.
I was able to see the QuickInfo with the diagnostic, but it only appears if your cursor doesn't accidentally trigger any other QuickInfo. Is that by design?
There was a problem hiding this comment.
pdate: I was able to see squiggles in string s = /lang=json/ "{a: 1, b: new()}"; under the open paren. But it's barely visible.
What's the experience in the IDE elsewhere for this sort of error? I'm not sure how to necessarily make that better if we're only squiggling the open paren. That said, we could squiggle more, and thus make the squiggle more visible.
I was able to see the QuickInfo with the diagnostic, but it only appears if your cursor doesn't accidentally trigger any other QuickInfo. Is that by design?
Yeah, you can get the same QI behavior today in other areas of your code. However, things are very exacerbated with regex/json strings since you're always hovering over part of a string. I think i should probably suppress normal QI for these strings so you don't have the issue you've identified. Great catch!
| public Task<BraceMatchingResult?> FindBracesAsync(Document document, int position, CancellationToken cancellationToken) | ||
| => CommonJsonBraceMatcher.FindBracesAsync(document, position, cancellationToken); | ||
| } | ||
| } |
There was a problem hiding this comment.
In testing this, I noticed that VsVim must be implementing its own brace matching logic...
If I have a mismatched brace inside a string, the VsVim command for jumping to the other side (%) jumps to the wrong location. I'll let Jared know.
Update: Edit.GoToBrace (default binding: Ctrl+]) works better :-)
|
This PR currently has much content that will be reviewed in other PRs, and it is tedious to winnow the changes to review from the changelist. |
Agreed!
Of course. Sorry about that. I must have accidentally merged in the regex branch as that wasn't supposed to be in here. |
|
@jcouv I removed all the regex changes from this PR. I also rejiggered how IDE features work for embedded languages. Instead of requiring IDE features (like brace matching) to know about each embedded language, i've instead made it so that embedded langauges can register IDE features and they'll be picked up automatically. This means we can add more embedded languages in hte future (like regex and beyond), without needing to go update all these end features. |
|
Thanks. I'll look this weekend. |
Yup. It most likely would. Not taht this gets a bit more challenging as typing really wants to be as synchronous as possible, whereas a lot of this embedded language stuff is async (as we have to look to see if you're actually instantiating a regex or parsing json and whatnot). That's why i didn't do it originally. |
fbcc4ee to
87de235
Compare
…Json Updating features/embeddedJson to the latest Master.
…/embeddedJson Merge master to features/embeddedJson
…/embeddedJson Merge master to features/embeddedJson
|
@CyrusNajmabadi closing this PR as discussed previously. |
This is a followup to #24110 . That PR adds the actual json parser and corresponding tests. This PR adds support in the IDE for features such as classification and squiggles. It was broken out to make the PRs easier to review.
[jcouv]
Builds on top of common infrastructure for embedded language services (PR #25985)