add 'extends' schema support and surface config chain diagnostics in VSCode - #20198
add 'extends' schema support and surface config chain diagnostics in VSCode#20198Likhitha Kodali (Likhithakmsft) wants to merge 6 commits into
Conversation
|
Test this change out locally with the following install scripts (Action run 32311945655) VSCode
Azure CLI
|
| var lspDiagnostics = diagnostics.Select(d => new LspDiagnostic | ||
| { | ||
| Severity = ToLspSeverity(d.Level), | ||
| Code = new DiagnosticCode(d.Code), | ||
| Message = d.Message, | ||
| Range = new OmniSharp.Extensions.LanguageServer.Protocol.Models.Range(0, 0, 0, 0), | ||
| Source = "bicep" | ||
| }); | ||
|
|
||
| server.TextDocument.PublishDiagnostics(new PublishDiagnosticsParams | ||
| { | ||
| Uri = documentUri, | ||
| Diagnostics = new Container<LspDiagnostic>(lspDiagnostics) | ||
| }); | ||
| } |
There was a problem hiding this comment.
This publishes all diagnostics under the active config using documentUri and discards IDiagnostic.Uri. We might want to group the diagnostics by IDiagnostic.Uri and call PublishDiagnostics for each group, so that errors can be properly linked to the invalid bicepconfig.json files in the chain.
There was a problem hiding this comment.
Thanks for the suggestion! After taking a look, IDiagnostic.Uri is actually a documentation link ( https://aka.ms/bicep/core-diagnostics#BCPxxx ) set in DiagnosticBuilder.cs , not a file URI
Since BicepConfigurationChain already stores each layer with its own ConfigFileUri and GetDiagnostics() , we can implement either of these 2 approaches.
- Add a
GetDiagnosticsPerLayer()method toIBicepConfigurationChainreturning(IOUri? FileUri, IEnumerable<IDiagnostic>)per layer. - Expose Layers directly on
IBicepConfigurationChainand UpdatPublishConfigDiagnosticsto iterate per layer.
Any preference or any other suggestions?
There was a problem hiding this comment.
Ahh, I didn't realize IDiagnostic.Uri is for documentation purposes only. The name is a bit misleading. I'll change it to DocumentationUri later.
For the fixes, I prefer option 1. I think that's what we do when reporting Bicep source file diagnostics. I'd call the method something like GetDiagnosticsByConfigFile and have it return an immutable dictionary, similar to what we do for Bicep source diagnostics:
There was a problem hiding this comment.
On second thought, the dictionary abstraction and allocation may not be necessary since it is only used for iteration. I wonder if we could just define an alias in Bicep.Core/Diagnostics/GlobalUsings.cs and use it instead:
global using DiagnosticsPerFile =
System.Collections.Generic.IEnumerable<
System.Collections.Generic.KeyValuePair<
Bicep.IO.IOUri,
System.Collections.Immutable.ImmutableArray<IDiagnostic>>>;
fcec577 to
7f63083
Compare
| Diagnostics = new Container<LspDiagnostic>() | ||
| }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
I just took a look at the LSP protocol, and it seems that if we don't publish diagnostics for certain files, the client retains their previous diagnostics.
So if you first publish:
A.json -> diagnostics A
B.json -> diagnostics B
C.json -> diagnostics C
and later publish only:
A.json → diagnostics A'
then B and C retain their previous diagnostics. The client doesn't infer that they should be cleared just because they weren't included in the later notification.
To fix it I think we need to publish diagnostics for each config file regardless, even if the array is empty:
{
"uri": "file:///B.json",
"diagnostics": []
}
This might be worth testing to confirm the behavior.
There was a problem hiding this comment.
Updated PublishConfigDiagnostics to always publish to every config file in the chain, including with an empty diagnostics array. Added a test to verify.
| /// Returns diagnostics grouped by the config file URI they originated from. | ||
| /// Built-in default layers (no <see cref="IBicepConfiguration.ConfigFileUri"/>) are excluded. | ||
| /// </summary> | ||
| public DiagnosticsPerFile GetDiagnosticsByConfigFile() |
There was a problem hiding this comment.
Since this returns an IEnumerable and is intended to be consumed by iteration rather than lookup, would it make sense to call this EnumerateDiagnosticsPerFile()? I think the Enumerate prefix makes the lazy nature of the result more explicit and distinguishes it from an API that returns a materialized collection.
There was a problem hiding this comment.
Renamed GetDiagnosticsByConfigFile to EnumerateDiagnosticsPerFile.
…updated PublishConfigDiagnostics
2 participants
Description
Builds on the BicepConfigurationManager chain engine (PR3- #20196) to give users immediate feedback in the editor when their bicepconfig.json has a bad "extends" value.
Changes
Schema
"extends"property tobicepconfig.schema.json"extends"keySquiggles
BicepConfigLifecycleManagernow receivesIBicepConfigurationManagerandILanguageServerFacadevia constructor injectionPublishConfigDiagnostics()-- callsGetConfigurationChain()and publishesany BCP453/454/455 diagnostics as squiggles on the
bicepconfig.jsonfileChecklist
Microsoft Reviewers: Open in CodeFlow