Improved tooltip and member doc display and other fixes#4024
Improved tooltip and member doc display and other fixes#4024MikhailArkhipov merged 16 commits intomicrosoft:masterfrom MikhailArkhipov:master
Conversation
|
|
||
| var displayText = result.ToString(); | ||
| if (displayOptions.trimDocumentationText && displayText.Length > displayOptions.maxDocumentationTextLength) { | ||
| displayText = displayText.Substring(0, |
There was a problem hiding this comment.
You can use result.Remove to avoid allocation of a displayText
| // Return module information | ||
| var contents = "{0} : module".FormatUI(modRef.Name); | ||
| if (!string.IsNullOrEmpty(modRef.Module?.Documentation)) { | ||
| contents += $"{Environment.NewLine}{Environment.NewLine}{modRef.Module.Documentation}"; |
There was a problem hiding this comment.
Do we need FormatUI here as well?
There was a problem hiding this comment.
It just does CurrentCulture... I am actually not sure that current culture is right. Here we should have invariant I believe. I.e. if we have programming language docs then numbers must remain 5.0 and should not be displayed as 5,0 in a culture-specific manner.
| _analyzer = CreateAnalyzer(@params.initializationOptions.interpreter).Result; | ||
| _testEnvironment = @params.initializationOptions.testEnvironment; | ||
| // Test environment needs predictable initialization. | ||
| if (@params.initializationOptions.asyncStartup && !_testEnvironment) { |
There was a problem hiding this comment.
Do we need testEnvironment if we already have asyncStartup?
There was a problem hiding this comment.
There are effectively 3 modes
- Test mode, all sync
- VS mode, as it used to work since I am not 100% confident VS part can handle all-async as I don't know all the entry points.
- VSC mode to minimize delays. VSC only need synchronization at the LS entry points. This is what asyncStartup controls.
VSC passes true for asyncStartup and true/false for tests. asyncStartup == false does not make it completely sync, VS still uses awaits in places. VSC creates analyzer async while VS does not.
if VS only uses LS entry points then we can just do (3). But bunch of methods are internal with visibility so I don't know.
| } | ||
|
|
||
| public override void DidChangeTextDocument(DidChangeTextDocumentParams @params) { | ||
| _analyzerCreationTcs.Task.Wait(); |
There was a problem hiding this comment.
Do we ever call this from UI thread?
There was a problem hiding this comment.
It is out-of-proc so technically there is no UI thread. OutOfProcProjectAnalyzer calls it in UpdateContent. In VS, however, this task completes in initialization so only VS Code may hit the wait.
|
|
||
| public override async Task DidOpenTextDocument(DidOpenTextDocumentParams @params) { | ||
| TraceMessage($"Opening document {@params.textDocument.uri}"); | ||
| await _analyzerCreationTcs.Task; |
There was a problem hiding this comment.
If there was an exception CreateAnalyzer, _analyzerCreationTcs will hold that exception. Is it correct to raise it every time API method is called?
There was a problem hiding this comment.
If analyzer creation failed, then the whole thing is broken. VSC restarts server automatically up to 3 times I believe.
| } else { | ||
| _analyzer = await CreateAnalyzer(@params.initializationOptions.interpreter); | ||
| OnAnalyzerCreated(@params); | ||
| _analyzerCreationTcs.TrySetResult(true); |
There was a problem hiding this comment.
Behavior here is different from when @params.initializationOptions.asyncStartup && !_testEnvironment is true. If CreateAnalyzer raises an exception, _analyzerCreationTcs will not complete. Is it by design?
There was a problem hiding this comment.
Let me try-catch that. Although if it throws, nothing is going to work, but at least exception may give client an idea that server may need to be restarted.
| } | ||
|
|
||
| await _analyzer.ReloadModulesAsync(); | ||
| _queue.Enqueue(new AnalysisQueueWorkItem(() => _analyzer.ReloadModulesAsync().WaitAndUnwrapExceptions()), AnalysisPriority.Normal); |
There was a problem hiding this comment.
This looks dangerous.
_queue.Enqueuewon't executeReloadModulesAsyncimmediately. If there is any code that awaitsDidChangeConfigurationto ensure that modules are reloaded, it will be brokenAnalysisQueuewill reorder project entries, but it won't reorderAnalysisQueueWorkItemcause they all are considered different. This is good in the way thatProjectEntry.Analyzewill be called always afterReloadModulesAsync, but has a side effect that several concurrent calls ofReloadModulesAsyncwill be executed one by one instead of swallowing all but first and last call.ReloadModulesAsyncmay start new tasks and await on them whileAnalysisQueueis thread-bound.AnalysisSynchronizationContextdoesn't support reentrancy andAnalysisQueuethread is blocked until method is completed, which means that if anywhere on the code path we have missedConfigureAwait(false)or provide any other way forAnalysisSynchronizationContextto be captured, we'll get a deadlock.
Also, ReloadModulesAsync should handle CancellationToken by itself, but that is a separate issue.
There was a problem hiding this comment.
Well, I am open to suggestions. Currently it is broken b/c ReloadModulesAsync is called from two threads while
/// This method should be called on the analysis thread and is usually invoked
/// when the interpreter signals that it's modules have changed.
So basically change is to queue item before calling _queue.Enqueue in DidChangeConfiguration. Nothing awaits on DidChangeConfiguration in reality, it is out of proc call, we can make it void.
ReloadModulesAsync should never be called in parallel AFAIU
There was a problem hiding this comment.
Well, I am open to suggestions
Let me think a bit :)
There was a problem hiding this comment.
I think analyzer should probably handle reload internally in its worker thread rather than caller know what is permitted and what not. This will be a bigger change.
There was a problem hiding this comment.
If it is queued, then _reloadLock.WaitAsync() prob not needed as there are no concurrent calls (apart of one at the creation at CreateAsync. I think this one can be queued too theoretically.
Now, we can also avoid doing anything in DidChangeConfiguration. Changing interpreter will actually restart the server.
There was a problem hiding this comment.
Ok, here is a version of small fix: AlexanderSher@bf068bd
DidChangeConfigurationwill wait for lastReloadModulesAsyncto be processedAnalysisQueuewill properly reorderReloadModulesAsynccall if it is already scheduledReloadModulesAsyncwill be started in thread-pool so reentrancy shouldn't be a problem.
AlexanderSher
left a comment
AlexanderSher
left a comment
There was a problem hiding this comment.
@huguesv , do you have any comments?
|
Is there enough test coverage for the documentation changes? Aside from spaces removed around operators, there doesn't seem to be much changed. |
|
Yes, I updated tests. Need to add tests for the new functionality (tooltips and docs for modules, see #4050). |
3 participants
Fixes #3849
Fixes #3869
Fixes #3882
Fixes #3977
Fixes #4031