Skip to content
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.

Sign up for GitHub

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

Only apply one document info change for linked documents #71220

Merged
merged 12 commits into from
Dec 29, 2023
Merged

Only apply one document info change for linked documents #71220

Changes from 6 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
c92305d
Put documents state change after project change
Cosifne Dec 12, 2023
7752c26
Add comment
Cosifne Dec 12, 2023
680014b
Fix analyzer warning
Cosifne Dec 12, 2023
2ba6479
Merge remote-tracking branch 'upstream/main' into dev/shech/DocumentI…
Cosifne Dec 12, 2023
dd66b72
Add test
Cosifne Dec 12, 2023
f25c710
Revert "Add test"
Cosifne Dec 13, 2023
1be7238
Merge remote-tracking branch 'upstream/main' into dev/shech/DocumentI…
Cosifne Dec 18, 2023
f8ff21f
Reapply "Add test"
Cosifne Dec 18, 2023
09b617d
Fix the flakyness in integration test
Cosifne Dec 18, 2023
a67c1ca
Delete a unnecessary change from branch checking
Cosifne Dec 18, 2023
35a0902
Remove the usage of All
Cosifne Dec 18, 2023
c122ea6
Address feedback
Cosifne Dec 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 37 additions & 15 deletions src/Workspaces/Core/Portable/Workspace/Workspace.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,7 @@
using System.Linq;
using System.Threading;
using System.Threading.Tasks;
using System.Xml;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CodeActions;
using Microsoft.CodeAnalysis.Diagnostics;
using Microsoft.CodeAnalysis.ErrorReporting;
using Microsoft.CodeAnalysis.Host;
Expand All @@ -21,7 +19,6 @@
using Microsoft.CodeAnalysis.PooledObjects;
using Microsoft.CodeAnalysis.Remote;
using Microsoft.CodeAnalysis.Shared.Extensions;
using Microsoft.CodeAnalysis.Shared.Utilities;
using Microsoft.CodeAnalysis.Text;
using Roslyn.Utilities;

Expand Down Expand Up @@ -1417,15 +1414,17 @@ internal virtual bool TryApplyChanges(Solution newSolution, IProgress<CodeAnalys
}

// changed projects
var projectChangesList = solutionChanges.GetProjectChanges().ToList();
progressTracker.AddItems(projectChangesList.Count);
var projectChangesList = solutionChanges.GetProjectChanges().ToImmutableArray();
progressTracker.AddItems(projectChangesList.Length);

foreach (var projectChanges in projectChangesList)
{
this.ApplyProjectChanges(projectChanges);
progressTracker.ItemCompleted();
}

this.ApplyDocumentsInfoChange(projectChangesList);

// changes in mapped files outside the workspace (may span multiple projects)
this.ApplyMappedFileChanges(solutionChanges);

Expand Down Expand Up @@ -1458,6 +1457,39 @@ internal virtual bool TryApplyChanges(Solution newSolution, IProgress<CodeAnalys
}
}

private void ApplyDocumentsInfoChange(ImmutableArray<ProjectChanges> projectChanges)
{
using var _1 = PooledHashSet<DocumentId>.GetInstance(out var infoChangedDocumentIds);
using var _2 = PooledHashSet<Document>.GetInstance(out var infoChangedNewDocument);
foreach (var projectChange in projectChanges)
{
foreach (var docId in projectChange.GetChangedDocuments())
{
var oldSolution = projectChange.OldProject.Solution;
var oldDoc = projectChange.OldProject.GetRequiredDocument(docId);
var newDoc = projectChange.NewProject.GetRequiredDocument(docId);
if (oldDoc.HasInfoChanged(newDoc))
{
var linkedDocumentIds = oldSolution.GetRequiredDocument(docId).GetLinkedDocumentIds();
// For linked documents, when info get changed (e.g. name/folder/filePath)
// only apply one document changed because it will update the 'real' file, causing the other linked documents get changed.
if (linkedDocumentIds.All(linkedDocId => !infoChangedDocumentIds.Contains(linkedDocId)))
{
infoChangedDocumentIds.Add(docId);
infoChangedNewDocument.Add(newDoc);
}
}
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like it could break things that should work. Say, for example, the user performs a fix-all on code like:

#if DEBUG

// code that is active in some project

#else

// other code that is not.

#endif

with the normal batch fixer, this would work, right? Or maybe it wouldn't? I'm not exactly sure.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel it won't be affected. This change would only affect things like file name, path change. For content change, each document would still be changed even they are linked.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good ot know! probably worth a test :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I imagine we might end up actually taking the ApplyChangedDocument path more than once, but we'll end up writing the same text each time -- the documents should have had merged texts. If we look under a debugger and see we are indeed doing that, that might be a nice potential win here to only process that once.

That said, I'm not sure how we exactly fit that in here -- we're just calling ApplyProjectChanges for both projects and we can't just pass additional parameters since that's a protected API. So either we have to break the API, or maybe pre-filter the ProjectChanges to remove the duplicate changes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, it sounds like a future work? (e.g. we have to see how things work for text changes)


foreach (var newDoc in infoChangedNewDocument)
{
// ApplyDocumentInfoChanged ignores the loader information, so we can pass null for it
ApplyDocumentInfoChanged(newDoc.Id,
new DocumentInfo(newDoc.DocumentState.Attributes, loader: null, documentServiceProvider: newDoc.State.Services));
}
}

internal virtual void ApplyMappedFileChanges(SolutionChanges solutionChanges)
{
return;
Expand Down Expand Up @@ -1830,16 +1862,6 @@ private void ApplyChangedDocument(
this.ApplyDocumentTextChanged(documentId, newText);
}
}

// Update document info if changed. Updating the info can cause files to move on disk (or have other side effects),
// so we do this after any text changes have been applied.
if (newDoc.HasInfoChanged(oldDoc))
{
// ApplyDocumentInfoChanged ignores the loader information, so we can pass null for it
ApplyDocumentInfoChanged(
documentId,
new DocumentInfo(newDoc.State.Attributes, loader: null, documentServiceProvider: newDoc.State.Services));
}
}

[Conditional("DEBUG")]
Expand Down