-
Notifications
You must be signed in to change notification settings - Fork 4.2k
File based programs IDE support #78208
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
...gs/ConvertLocalFunctionToMethod/CSharpConvertLocalFunctionToMethodCodeRefactoringProvider.cs
Outdated
Show resolved
Hide resolved
src/Features/CSharp/Portable/CodeRefactorings/SyncNamespace/CSharpChangeNamespaceService.cs
Outdated
Show resolved
Hide resolved
...ageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/LanguageServerProjectSystem.cs
Outdated
Show resolved
Hide resolved
...Server/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/LanguageServerWorkspaceFactory.cs
Outdated
Show resolved
Hide resolved
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/LspFileBasedProgramWorkspace.cs
Outdated
Show resolved
Hide resolved
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/LspFileBasedProgramWorkspace.cs
Outdated
Show resolved
Hide resolved
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/LspFileBasedProgramWorkspace.cs
Outdated
Show resolved
Hide resolved
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/LspFileBasedProgramWorkspace.cs
Outdated
Show resolved
Hide resolved
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/LspFileBasedProgramWorkspace.cs
Outdated
Show resolved
Hide resolved
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/LspFileBasedProgramWorkspace.cs
Outdated
Show resolved
Hide resolved
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/LspFileBasedProgramWorkspace.cs
Outdated
Show resolved
Hide resolved
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/LspFileBasedProgramWorkspace.cs
Outdated
Show resolved
Hide resolved
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/LspFileBasedProgramWorkspace.cs
Outdated
Show resolved
Hide resolved
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/LspFileBasedProgramWorkspace.cs
Outdated
Show resolved
Hide resolved
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/LspFileBasedProgramWorkspace.cs
Outdated
Show resolved
Hide resolved
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/LspFileBasedProgramWorkspace.cs
Outdated
Show resolved
Hide resolved
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/LspFileBasedProgramWorkspace.cs
Outdated
Show resolved
Hide resolved
| var root = tree.GetRoot(); | ||
| return | ||
| //root.DescendantNodes(descendIntoChildren: node => node.IsKind(SyntaxKind.CompilationUnit)).OfType<GlobalStatementSyntax>().Any() || | ||
| root.GetLeadingTrivia().FirstOrDefault().IsKind(SyntaxKind.ShebangDirectiveTrivia); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this feels expensive. if we're just checking leading trivial, we can get hte text, and just check if it literally starts with a #!. because you're getting the leading trivia, only checking the first, and forcing it to ahve that. now it's just a two char check, instead of a full lex/parse.
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/LspFileBasedProgramWorkspace.cs
Outdated
Show resolved
Hide resolved
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/LspFileBasedProgramWorkspace.cs
Outdated
Show resolved
Hide resolved
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/LspFileBasedProgramWorkspace.cs
Outdated
Show resolved
Hide resolved
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/LspFileBasedProgramWorkspace.cs
Outdated
Show resolved
Hide resolved
| </PropertyGroup> | ||
| <ItemGroup> | ||
| <Compile Include="{documentFilePath}" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not at all safe. (effectively, this is like sql injection). We should not be manually making xml. We should use an xml creation lib to get to this to happen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is going to be deleted and replaced with usage of API described in #78159.
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/LspFileBasedProgramWorkspace.cs
Outdated
Show resolved
Hide resolved
src/LanguageServer/Protocol/Workspaces/ILspFileBasedProgramWorkspace.cs
Outdated
Show resolved
Hide resolved
| internal interface ILspFileBasedProgramWorkspace : ILspService | ||
| { | ||
| public Task<Document?> AddMiscellaneousDocumentAsync(Uri uri, SourceText documentText, string languageId, ILspLogger logger, CancellationToken cancellationToken); | ||
| public void TryRemoveMiscellaneousDocument(Uri uri, bool removeFromMetadataWorkspace); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doc the heck out of these. esp. threading concerns.
dfc63d5 to
2558d31
Compare
2558d31 to
ed23c0c
Compare
| var preferredBuildHostKind = GetKindForProject(projectPath); | ||
| var (buildHost, actualBuildHostKind) = await buildHostProcessManager.GetBuildHostWithFallbackAsync(preferredBuildHostKind, projectPath, cancellationToken); | ||
| if (preferredBuildHostKind != actualBuildHostKind) | ||
| preferredBuildHostKindThatWeDidNotGet = preferredBuildHostKind; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still need to move this block into the TryLoadProjectAsync() method, since the GetKindForProject is implicitly looking at the project file contents. In the case of the file-based programs, the selection is trivially "just use .NET Core".
| foreach (var project in projectsToRemove) | ||
| { | ||
| project.Dispose(); | ||
| _loadedProjects.AddOrUpdate(projectPath, addValueFactory: _ => throw new InvalidOperationException(), updateValueFactory: (_, arr) => arr.Remove(project)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a comment obviously that we only expect this getting the update path.
| } | ||
|
|
||
| /// <summary> | ||
| /// Creates a 'LoadedProject' which has bare minimum information, but, which documents can be added to and obtained from. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use a cref.
| // We should never be changing the fundamental identity of this project; if this happens we really should have done a full unload/reload. | ||
| Contract.ThrowIfFalse(newProjectInfo.FilePath == _mostRecentFileInfo.FilePath); | ||
| Contract.ThrowIfFalse(newProjectInfo.TargetFramework == _mostRecentFileInfo.TargetFramework); | ||
| Contract.ThrowIfFalse(_mostRecentFileInfo.TargetFramework == null || newProjectInfo.TargetFramework == _mostRecentFileInfo.TargetFramework); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be removed now.
| ContentFilePaths = [], | ||
| FileGlobs = [] | ||
| }; | ||
| await loadedProject.UpdateWithNewProjectInfoAsync(projectFileInfo, _logger); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still need to remember to set the "HasAllInformation" flag here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering the following scenarios:
- ordinary project. HasAllInformation should always be true, when this method runs, I think.
- loose file, not part of a file-based program. HasAllInformation should be false.
- file-based program entry point (currently). HasAllInformation should be true.
So I was wondering if we should pass in a flag like forMiscellaneousFiles when creating the LoadedProject, and finally say HasAllInformation = !forMiscellaneousFiles || checkHeuristic() within this method (since it's FBP-ness can change from edit-to-edit.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
ed23c0c to
6150e84
Compare
| { | ||
| const BuildHostProcessKind buildHostKind = BuildHostProcessKind.NetCore; | ||
| var buildHost = await buildHostProcessManager.GetBuildHostAsync(buildHostKind, cancellationToken); | ||
| var documentPath = projectToLoad.Path; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We only ever need the path from projectToLoad, right? Should we just pass that in?
...ageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/LanguageServerProjectLoader.cs
Outdated
Show resolved
Hide resolved
| else | ||
| { | ||
| newProjects.Add(project); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're going to re-create the array all the time, just do ImmutableArray.RemoveRange().
869120b to
d57b93f
Compare
|
"Support unloading projects" is a bit gnarly, I am going to make a pass tomorrow to clean it up and add comments to make our assumptions clear. |
...ageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/LanguageServerProjectLoader.cs
Show resolved
Hide resolved
8384a07 to
b00d343
Compare
Design doc: https://github.com/rikkigibson/roslyn/blob/file-based-programs-ide/docs/features/file-based-programs-vscode.md
See also dotnet/vscode-csharp#8253