Another way to correct generate Uri for new Document when resolve code action #68995

Cosifne · 2023-07-12T01:22:20Z

A different path to fix #68550
The reason is we don't provide folders in document in LSP server, so when https://sourceroslyn.io/#Microsoft.CodeAnalysis.Features/CodeRefactorings/MoveType/AbstractMoveTypeService.MoveTypeEditor.cs,148 is called, no folder is given, and we fall back to add the document under the project root.

Cosifne · 2023-07-12T01:22:28Z

cc @jasonmalinowski

jasonmalinowski · 2023-07-12T17:42:49Z

src/Workspaces/Core/MSBuild/MSBuild/ProjectFile/ProjectFile.cs

@@ -206,8 +206,11 @@ private DocumentFileInfo MakeDocumentFileInfo(MSB.Framework.ITaskItem documentIt
            var isLinked = IsDocumentLinked(documentItem);
            var isGenerated = IsDocumentGenerated(documentItem);
            var sourceCodeKind = GetSourceCodeKind(filePath);
+            var relativePath = PathUtilities.GetDirectoryName(
+                PathUtilities.GetRelativePath(_projectDirectory, filePath));


What is this going to do if the file path is outside the project directory cone?

🤔If it is not in the project directory, we might support things like .. so it could be ..\..\projectB\cool\folder.
Or we should just assume the document lives in the project directory and use null/ImmutableArray.Empty in all other cases?

Oh cool I found PathUtilites.GetRelativePath could give .. as the parent folder, so I guess I just need to add some tests to verify that.

I guess my question isn't "what does the method return" but "do we expect .. entries to appear in the folders list in the first place?"

🤔I feel it should be allowed in the legacy project, I have added a test for that case.

See MSBuildWorkspaceWithDocumentInParentFolders test, its referencing a document in ../MyDir

src/Workspaces/Core/MSBuild/MSBuild/ProjectFile/ProjectFile.cs

src/Features/LanguageServer/Protocol/Handler/CodeActions/CodeActionResolveHandler.cs

src/Workspaces/Core/MSBuild/MSBuild/ProjectFile/ProjectFile.cs

src/Features/LanguageServer/Protocol/Handler/CodeActions/CodeActionResolveHandler.cs

src/Features/LanguageServer/Protocol/Extensions/Extensions.cs

src/EditorFeatures/TestUtilities/Workspaces/TestWorkspace_XmlCreation.cs

src/Workspaces/Core/MSBuild/MSBuild/ProjectFile/ProjectFile.cs

src/EditorFeatures/TestUtilities/Workspaces/TestWorkspace_XmlCreation.cs

…lder

Cosifne · 2023-07-25T23:05:47Z

src/Workspaces/MSBuildTest/Resources/ProjectFiles/CSharp/WithClassNotInProjectFolder.csproj

+    </Compile>
+    <Compile Include="CSharpClass.cs" />
+    <Compile Include="Properties\AssemblyInfo.cs" />
+    <Compile Include="..\MyDir\MyClass.cs" />


This is the core test scenario in MSBuildWorkspaceWithDocumentInParentFolders

Cosifne · 2023-07-25T23:07:01Z

src/Workspaces/MSBuildTest/VisualStudioMSBuildWorkspaceTests.cs

+            using var workspace = CreateMSBuildWorkspace();
+            var project = await workspace.OpenProjectAsync(GetSolutionFileName(@"CSharpProject\CSharpProject.csproj"));
+            var linkedDocument = project.Documents.Single(d => d.Name == "Foo.cs");
+            Assert.Equal(new[] { "Blah" }, linkedDocument.Folders);


This is the linked location in our resources file. See here

roslyn/src/Workspaces/MSBuildTest/Resources/ProjectFiles/CSharp/WithLink.csproj

Line 49 in 015aec6

<Link>Blah\Foo.cs</Link>

Cosifne · 2023-07-25T23:15:27Z

@jasonmalinowski It's ready to be reviewed second time

jasonmalinowski

Overall looks good. I have a few questions just to double check a few behaviors, but assuming everything is matching up, let's .

src/EditorFeatures/TestUtilities/Workspaces/TestWorkspace_XmlCreation.cs

...es/TestUtilities/LanguageServer/AbstractLanguageServerProtocolTests.InitializationOptions.cs

src/Workspaces/MSBuildTest/VisualStudioMSBuildWorkspaceTests.cs

src/Workspaces/Core/MSBuild/MSBuild/ProjectFile/ProjectFile.cs

src/Features/LanguageServer/ProtocolUnitTests/CodeActions/CodeActionResolveTests.cs

…lder

Cosifne added 9 commits July 5, 2023 16:32

Add vscode side fix

0bcc358

Add test stub

c189c4f

Clean the warnings

fda628d

Modify the test

c8ff2e9

Fix test

51b89b5

Let the document to find folders

63bbf1c

Code clean up

b861692

Add comment

5710ead

Fix test

e583d7a

Cosifne requested a review from a team as a code owner July 12, 2023 01:22

dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Jul 12, 2023

Fix NRE

b19832b

jasonmalinowski reviewed Jul 12, 2023

View reviewed changes

jasonmalinowski mentioned this pull request Jul 24, 2023

Add filePath when generates new Document in MoveTypeEditor #68900

Closed

Cosifne added 7 commits July 25, 2023 12:06

Use link path when document is linked

764c7c6

Merge remote-tracking branch 'upstream/main' into dev/shech/addFileFo…

fc2ef1f

…lder

Add test

93cce6a

Add second test

014e205

Add third test

807e6f0

Address feedback

5c7b7f6

Address feedback

8abf2ca

Cosifne commented Jul 25, 2023

View reviewed changes

Cosifne requested a review from jasonmalinowski July 25, 2023 23:08

jasonmalinowski approved these changes Aug 10, 2023

View reviewed changes

Cosifne added 2 commits August 18, 2023 13:19

Merge remote-tracking branch 'upstream/main' into dev/shech/addFileFo…

fcd4809

…lder

Fix error after merge

fef2f19

Cosifne added 3 commits August 18, 2023 14:11

Add comments

3bfb6ed

Add assert

09ded2d

Address feedback

73cef9d

Cosifne merged commit 9a05dcc into dotnet:main Aug 21, 2023

Cosifne deleted the dev/shech/addFileFolder branch August 21, 2023 17:09

ghost added this to the Next milestone Aug 21, 2023

This was referenced Aug 21, 2023

Refactoring "move type to Typename.cs" now outputs the file to the root folder, rather than the folder of its current file. dotnet/vscode-csharp#6023

Closed

Move class to its own file refactoring creates file in wrong folder dotnet/vscode-csharp#5918

Closed

dibarbet modified the milestones: Next, 17.8 P2 Aug 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Another way to correct generate Uri for new Document when resolve code action #68995

Another way to correct generate Uri for new Document when resolve code action #68995

Cosifne commented Jul 12, 2023

Cosifne commented Jul 12, 2023

jasonmalinowski Jul 12, 2023

Cosifne Jul 24, 2023

Cosifne Jul 24, 2023

jasonmalinowski Jul 24, 2023

Cosifne Jul 25, 2023

Cosifne Jul 25, 2023

Cosifne Jul 25, 2023 •

edited

Loading

Cosifne Jul 25, 2023

Cosifne commented Jul 25, 2023

jasonmalinowski left a comment

Another way to correct generate Uri for new Document when resolve code action #68995

Another way to correct generate Uri for new Document when resolve code action #68995

Conversation

Cosifne commented Jul 12, 2023

Cosifne commented Jul 12, 2023

jasonmalinowski Jul 12, 2023

Choose a reason for hiding this comment

Cosifne Jul 24, 2023

Choose a reason for hiding this comment

Cosifne Jul 24, 2023

Choose a reason for hiding this comment

jasonmalinowski Jul 24, 2023

Choose a reason for hiding this comment

Cosifne Jul 25, 2023

Choose a reason for hiding this comment

Cosifne Jul 25, 2023

Choose a reason for hiding this comment

Cosifne Jul 25, 2023 • edited Loading

Choose a reason for hiding this comment

Cosifne Jul 25, 2023

Choose a reason for hiding this comment

Cosifne commented Jul 25, 2023

jasonmalinowski left a comment

Choose a reason for hiding this comment

Cosifne Jul 25, 2023 •

edited

Loading