From 2dbe99f51854b39d52aeb0979e619339d6e51d01 Mon Sep 17 00:00:00 2001 From: Drew Noakes Date: Tue, 2 Jul 2019 22:47:09 +1000 Subject: [PATCH] Fix snapshot's active target framework bug New validation on DependenciesSnapshot.ActiveTargetFramework could be hit before this change when changing a project's target framework(s). This change replaces the previous logic to: - prevent ActiveTargetFramework from not being present as one of the targets in the snapshot - eagerly populate the snapshot with any additional frameworks as soon as they're discovered rather than letting it happen lazily --- .../Snapshot/DependenciesSnapshotTests.cs | 71 +++++++++++++++++++ .../Snapshot/DependenciesSnapshot.cs | 27 +++++-- .../DependenciesSnapshotProvider.cs | 27 +------ .../Utilities/SetDiff.cs | 6 +- 4 files changed, 96 insertions(+), 35 deletions(-) diff --git a/src/Microsoft.VisualStudio.ProjectSystem.Managed.VS.UnitTests/ProjectSystem/VS/Tree/Dependencies/Snapshot/DependenciesSnapshotTests.cs b/src/Microsoft.VisualStudio.ProjectSystem.Managed.VS.UnitTests/ProjectSystem/VS/Tree/Dependencies/Snapshot/DependenciesSnapshotTests.cs index 242b8cac6ec..ee18737f780 100644 --- a/src/Microsoft.VisualStudio.ProjectSystem.Managed.VS.UnitTests/ProjectSystem/VS/Tree/Dependencies/Snapshot/DependenciesSnapshotTests.cs +++ b/src/Microsoft.VisualStudio.ProjectSystem.Managed.VS.UnitTests/ProjectSystem/VS/Tree/Dependencies/Snapshot/DependenciesSnapshotTests.cs @@ -251,6 +251,77 @@ public void FromChanges_ProjectPathAndTargetChange() Assert.Equal(@"tfm1\Xxx\dependency1", snapshot.DependenciesByTargetFramework[targetFramework].DependenciesWorld.First().Value.Id); } + [Fact] + public void SetTargets_FromEmpty() + { + const string projectPath = @"c:\somefolder\someproject\a.csproj"; + + ITargetFramework tfm1 = new TargetFramework("tfm1"); + ITargetFramework tfm2 = new TargetFramework("tfm2"); + + var snapshot = DependenciesSnapshot.CreateEmpty(projectPath) + .SetTargets(new[] { tfm1, tfm2 }.ToImmutableArray(), tfm1); + + Assert.Same(tfm1, snapshot.ActiveTargetFramework); + Assert.Equal(2, snapshot.DependenciesByTargetFramework.Count); + Assert.True(snapshot.DependenciesByTargetFramework.ContainsKey(tfm1)); + Assert.True(snapshot.DependenciesByTargetFramework.ContainsKey(tfm2)); + } + + [Fact] + public void SetTargets_SameMembers_DifferentActive() + { + const string projectPath = @"c:\somefolder\someproject\a.csproj"; + + ITargetFramework tfm1 = new TargetFramework("tfm1"); + ITargetFramework tfm2 = new TargetFramework("tfm2"); + + var before = DependenciesSnapshot.CreateEmpty(projectPath) + .SetTargets(new[] { tfm1, tfm2 }.ToImmutableArray(), tfm1); + + var after = before.SetTargets(new[] { tfm1, tfm2 }.ToImmutableArray(), tfm2); + + Assert.Same(tfm2, after.ActiveTargetFramework); + Assert.Same(before.DependenciesByTargetFramework, after.DependenciesByTargetFramework); + } + + [Fact] + public void SetTargets_SameMembers_SameActive() + { + const string projectPath = @"c:\somefolder\someproject\a.csproj"; + + ITargetFramework tfm1 = new TargetFramework("tfm1"); + ITargetFramework tfm2 = new TargetFramework("tfm2"); + + var before = DependenciesSnapshot.CreateEmpty(projectPath) + .SetTargets(new[] { tfm1, tfm2 }.ToImmutableArray(), tfm1); + + var after = before.SetTargets(new[] { tfm1, tfm2 }.ToImmutableArray(), tfm1); + + Assert.Same(before, after); + } + + [Fact] + public void SetTargets_DifferentMembers_DifferentActive() + { + const string projectPath = @"c:\somefolder\someproject\a.csproj"; + + ITargetFramework tfm1 = new TargetFramework("tfm1"); + ITargetFramework tfm2 = new TargetFramework("tfm2"); + ITargetFramework tfm3 = new TargetFramework("tfm3"); + + var before = DependenciesSnapshot.CreateEmpty(projectPath) + .SetTargets(new[] { tfm1, tfm2 }.ToImmutableArray(), tfm1); + + var after = before.SetTargets(new[] { tfm2, tfm3 }.ToImmutableArray(), tfm3); + + Assert.Same(tfm3, after.ActiveTargetFramework); + Assert.Equal(2, after.DependenciesByTargetFramework.Count); + Assert.True(after.DependenciesByTargetFramework.ContainsKey(tfm2)); + Assert.True(after.DependenciesByTargetFramework.ContainsKey(tfm3)); + Assert.Same(before.DependenciesByTargetFramework[tfm2], after.DependenciesByTargetFramework[tfm2]); + } + private static ImmutableDictionary CreateDependenciesByTargetFramework( string projectPath, IProjectCatalogSnapshot catalogs, diff --git a/src/Microsoft.VisualStudio.ProjectSystem.Managed/Tree/Dependencies/Snapshot/DependenciesSnapshot.cs b/src/Microsoft.VisualStudio.ProjectSystem.Managed/Tree/Dependencies/Snapshot/DependenciesSnapshot.cs index c3244b32415..4caf2d6ec67 100644 --- a/src/Microsoft.VisualStudio.ProjectSystem.Managed/Tree/Dependencies/Snapshot/DependenciesSnapshot.cs +++ b/src/Microsoft.VisualStudio.ProjectSystem.Managed/Tree/Dependencies/Snapshot/DependenciesSnapshot.cs @@ -151,14 +151,29 @@ bool SyncTargetFrameworks() } } - public DependenciesSnapshot RemoveTargets(IEnumerable targetToRemove) + public DependenciesSnapshot SetTargets( + ImmutableArray targetFrameworks, + ITargetFramework activeTargetFramework) { - ImmutableDictionary newTargets = DependenciesByTargetFramework.RemoveRange(targetToRemove); + bool activeChanged = !activeTargetFramework.Equals(ActiveTargetFramework); + + var map = DependenciesByTargetFramework; + + var diff = new SetDiff(map.Keys, targetFrameworks); + + map = map.RemoveRange(diff.Removed); + map = map.AddRange( + diff.Added.Select( + added => new KeyValuePair( + added, + TargetedDependenciesSnapshot.CreateEmpty(ProjectPath, added, null)))); + + if (activeChanged || !ReferenceEquals(map, DependenciesByTargetFramework)) + { + return new DependenciesSnapshot(ProjectPath, activeTargetFramework, map); + } - // Return this if no targets changed - return ReferenceEquals(newTargets, DependenciesByTargetFramework) - ? this - : new DependenciesSnapshot(ProjectPath, ActiveTargetFramework, newTargets); + return this; } // Internal, for test use -- normal code should use the factory methods diff --git a/src/Microsoft.VisualStudio.ProjectSystem.Managed/Tree/Dependencies/Subscriptions/DependenciesSnapshotProvider.cs b/src/Microsoft.VisualStudio.ProjectSystem.Managed/Tree/Dependencies/Subscriptions/DependenciesSnapshotProvider.cs index a9e7d5a988d..40ee2fcedb6 100644 --- a/src/Microsoft.VisualStudio.ProjectSystem.Managed/Tree/Dependencies/Subscriptions/DependenciesSnapshotProvider.cs +++ b/src/Microsoft.VisualStudio.ProjectSystem.Managed/Tree/Dependencies/Subscriptions/DependenciesSnapshotProvider.cs @@ -8,6 +8,7 @@ using System.Threading; using System.Threading.Tasks; using System.Threading.Tasks.Dataflow; + using Microsoft.Build.Execution; using Microsoft.VisualStudio.Build; using Microsoft.VisualStudio.Composition; @@ -489,31 +490,7 @@ void OnAggregateContextChanged( AggregateCrossTargetProjectContext? oldContext, AggregateCrossTargetProjectContext newContext) { - if (oldContext == null) - { - // all new rules will be sent to new context, we don't need to clean up anything - return; - } - - var targetsToClean = new HashSet(); - - ImmutableArray oldTargets = oldContext.TargetFrameworks; - - if (newContext == null) - { - targetsToClean.AddRange(oldTargets); - } - else - { - ImmutableArray newTargets = newContext.TargetFrameworks; - - targetsToClean.AddRange(oldTargets.Except(newTargets)); - } - - if (targetsToClean.Count != 0) - { - TryUpdateSnapshot(snapshot => snapshot.RemoveTargets(targetsToClean)); - } + TryUpdateSnapshot(snapshot => snapshot.SetTargets(newContext.TargetFrameworks, newContext.ActiveTargetFramework)); } } diff --git a/src/Microsoft.VisualStudio.ProjectSystem.Managed/Utilities/SetDiff.cs b/src/Microsoft.VisualStudio.ProjectSystem.Managed/Utilities/SetDiff.cs index 0e4ac82d02c..876cbeb03e5 100644 --- a/src/Microsoft.VisualStudio.ProjectSystem.Managed/Utilities/SetDiff.cs +++ b/src/Microsoft.VisualStudio.ProjectSystem.Managed/Utilities/SetDiff.cs @@ -4,8 +4,6 @@ using System.Collections; using System.Collections.Generic; -#nullable disable - namespace Microsoft.VisualStudio.ProjectSystem { internal sealed class SetDiff @@ -70,7 +68,7 @@ public PartEnumerator(Dictionary dic, byte flag) { _flag = flag; _enumerator = dic.GetEnumerator(); - Current = default; + Current = default!; } public bool MoveNext() @@ -89,7 +87,7 @@ public bool MoveNext() public T Current { get; private set; } - object IEnumerator.Current => Current; + object IEnumerator.Current => Current!; void IEnumerator.Reset() => throw new NotSupportedException();