Show semantic errors for loose files with top-level statements #81326
Conversation
...r/Microsoft.CodeAnalysis.LanguageServer/FileBasedPrograms/CanonicalMiscFilesProjectLoader.cs
Outdated
Show resolved
Hide resolved
| await _workspaceFactory.MiscellaneousFilesWorkspaceProjectFactory.ApplyChangeToWorkspaceAsync(workspace => | ||
| { | ||
| workspace.OnProjectAdded(forkedProjectInfo); | ||
| }, cancellationToken); |
There was a problem hiding this comment.
What are the semantics of cancellation here? I definitely don't know what the intent is, or if it is ok that workspace mutation ops may not happen. @dibarbet what the general intuition here?
There was a problem hiding this comment.
This cancellation token here would be cancelled if the request gets cancelled by the client. Assuming that cancelling the ApplyChangeToWorkspaceAsync doesn't leave the workspace in an intermediate state, a cancellation here is fine. We won't add the project to the workspace, and that is totally fine. If another call comes in later asking about this document, we'll get here again and try and add it to the workspace.
That applies to AddDocumentToPrimordialProject_NoLockAsync as well. For CreatePrimordialProjectAndAddDocument_NoLock, that is not passed a token as it initiates a project loading operation behind the scenes which should not be cancelled even if the request itself is cancelled, as we'll re-use it for other requests.
| await _workspaceFactory.MiscellaneousFilesWorkspaceProjectFactory.ApplyChangeToWorkspaceAsync(workspace => | ||
| { | ||
| workspace.OnProjectAdded(forkedProjectInfo); | ||
| }, cancellationToken); |
There was a problem hiding this comment.
This cancellation token here would be cancelled if the request gets cancelled by the client. Assuming that cancelling the ApplyChangeToWorkspaceAsync doesn't leave the workspace in an intermediate state, a cancellation here is fine. We won't add the project to the workspace, and that is totally fine. If another call comes in later asking about this document, we'll get here again and try and add it to the workspace.
That applies to AddDocumentToPrimordialProject_NoLockAsync as well. For CreatePrimordialProjectAndAddDocument_NoLock, that is not passed a token as it initiates a project loading operation behind the scenes which should not be cancelled even if the request itself is cancelled, as we'll re-use it for other requests.
...r/Microsoft.CodeAnalysis.LanguageServer/FileBasedPrograms/CanonicalMiscFilesProjectLoader.cs
Outdated
Show resolved
Hide resolved
| assemblyName: canonicalProject.AssemblyName, | ||
| language: canonicalProject.Language, | ||
| compilationOutputInfo: default, | ||
| checksumAlgorithm: SourceHashAlgorithm.Sha1, |
There was a problem hiding this comment.
should we default to something other than sha1 here? or are we restricted to sha1
There was a problem hiding this comment.
This was extracted from the helper CreateMiscellaneousProjectInfoForDocument. I think this is likely used as part of the ecma-335 carveout.
| DocumentInfo.Create( | ||
| DocumentId.CreateNewId(forkedProjectId), | ||
| name: Path.GetFileName(documentPath) ?? "", | ||
| loader: TextLoader.From(TextAndVersion.Create(await document.GetTextAsync(cancellationToken).ConfigureAwait(false), VersionStamp.Create())), |
There was a problem hiding this comment.
is there a way to avoid realizing the text here? e.g. can we take the loader from the existing document?
There was a problem hiding this comment.
Possibly, but I didn't see the API for doing that on the document.
| { | ||
| // TODO: replace this method and the call site in LspWorkspaceManager, | ||
| // with a mechanism for "updating workspace state if needed" based on changes to a document. | ||
| // Perhaps this could be based on actually listening for changes to particular documents, rather than whenever an LSP request related to a document comes in. |
There was a problem hiding this comment.
will have to think about this. We don't want to handle it inside the lsp text change notification itself as that intentionally does as little work as possible. It could be done as an async handling triggered by lsp text changes - but would have to handle LSP requests only being eventually consistent in terms of which project its in.
but definitely space to refactor this to make it more clear how transitions happen.
There was a problem hiding this comment.
Eventually consistent seems consistent, no pun intended, with how document state works in general. DTBs take time and we will give you a different answer if you ask again later.
Here, we could theoretically see a change and fire off some async request to check the syntax and update project HasAllInformation state accordingly. And a future request will see the new value of that. I think that's ok.
RikkiGibson
left a comment
RikkiGibson
left a comment
There was a problem hiding this comment.
Currently working on addressing a test failure.
|
Going to extract out the impl change related to |
3 participants
Closes #81252 (second half of the change)
@dibarbet for review