From 933f87ddaaa6d0ebbbb4d0400397b975070409cb Mon Sep 17 00:00:00 2001 From: Drew Noakes Date: Mon, 24 Apr 2023 18:37:12 +1000 Subject: [PATCH] Show diagnostic beneath unresolved package/project reference in Solution Explorer Previously, if a project referenced a non-existent package or project, the dependency would appear with a yellow triangle without any further explanation. A log message would be present in the `project.assets.json` file, however we would not attach it to the tree. This is because unresolved packages/projects such as these do not have entries in the `libraries` section of the lock file. This meant that we would not create an `IRelatableItem` for the library, and would then not be able to attach child nodes (such as diagnostics). This change ensures that libaries mentioned in log messages have at least a dummy entry in the `LibraryByName` collection, so that tree construction completes successfully. Another required change here was the normalization of unresolved project paths. In some instances these were identified with the relative path used in the project file (such as `..\OtherProject\OtherProject.csproj`), yet in the log messages the identity would be the full path. To fix this issue and allow diagnostics to be attached to unresolved project references, absolute library IDs are made relative to the project file during snapshot construction. It's possible to author a project file for which this would not work correctly, but that seems unlikely to occur very often in practice. --- .../Items/PackageAssemblyItem.cs | 4 +- .../Items/PackageBuildFileItem.cs | 2 +- .../Items/PackageDocumentItem.cs | 2 +- .../Items/PackageReferenceItem.cs | 8 ++-- .../Models/AssetsFileDependenciesSnapshot.cs | 25 +++++++++--- .../Models/AssetsFileLibraryType.cs | 18 ++++++++- .../Models/AssetsFileLogMessage.cs | 23 +++++++++-- .../Models/AssetsFileTarget.cs | 6 +-- .../Models/AssetsFileTargetLibrary.cs | 39 ++++++++++++++++++- 9 files changed, 105 insertions(+), 22 deletions(-) diff --git a/src/NuGet.Clients/NuGet.VisualStudio.Implementation/SolutionExplorer/Items/PackageAssemblyItem.cs b/src/NuGet.Clients/NuGet.VisualStudio.Implementation/SolutionExplorer/Items/PackageAssemblyItem.cs index 1c275e4d31f..615660dc3e7 100644 --- a/src/NuGet.Clients/NuGet.VisualStudio.Implementation/SolutionExplorer/Items/PackageAssemblyItem.cs +++ b/src/NuGet.Clients/NuGet.VisualStudio.Implementation/SolutionExplorer/Items/PackageAssemblyItem.cs @@ -67,7 +67,9 @@ public string? Path { get { - return _item.Target.TryResolvePackagePath(_item.Library.Name, _item.Library.Version, out string? fullPath) + return + _item.Library.Version is not null && + _item.Target.TryResolvePackagePath(_item.Library.Name, _item.Library.Version, out string? fullPath) ? System.IO.Path.GetFullPath(System.IO.Path.Combine(fullPath, _item.Path)) : null; } diff --git a/src/NuGet.Clients/NuGet.VisualStudio.Implementation/SolutionExplorer/Items/PackageBuildFileItem.cs b/src/NuGet.Clients/NuGet.VisualStudio.Implementation/SolutionExplorer/Items/PackageBuildFileItem.cs index 5f5a4a90e3c..656011750e1 100644 --- a/src/NuGet.Clients/NuGet.VisualStudio.Implementation/SolutionExplorer/Items/PackageBuildFileItem.cs +++ b/src/NuGet.Clients/NuGet.VisualStudio.Implementation/SolutionExplorer/Items/PackageBuildFileItem.cs @@ -40,7 +40,7 @@ public PackageBuildFileItem(AssetsFileTarget target, AssetsFileTargetLibrary lib _fileOpener = fileOpener; } - public string? FullPath => Target.TryResolvePackagePath(Library.Name, Library.Version, out string? fullPath) + public string? FullPath => Library.Version is not null && Target.TryResolvePackagePath(Library.Name, Library.Version, out string? fullPath) ? System.IO.Path.GetFullPath(System.IO.Path.Combine(fullPath, Path)) : null; diff --git a/src/NuGet.Clients/NuGet.VisualStudio.Implementation/SolutionExplorer/Items/PackageDocumentItem.cs b/src/NuGet.Clients/NuGet.VisualStudio.Implementation/SolutionExplorer/Items/PackageDocumentItem.cs index 17188920730..6419c7c8e2f 100644 --- a/src/NuGet.Clients/NuGet.VisualStudio.Implementation/SolutionExplorer/Items/PackageDocumentItem.cs +++ b/src/NuGet.Clients/NuGet.VisualStudio.Implementation/SolutionExplorer/Items/PackageDocumentItem.cs @@ -68,7 +68,7 @@ public bool Invoke(IEnumerable items, InputSource inputSource, bool prev return result; } - public string? FullPath => Target.TryResolvePackagePath(Library.Name, Library.Version, out string? fullPath) + public string? FullPath => Library.Version is not null && Target.TryResolvePackagePath(Library.Name, Library.Version, out string? fullPath) ? System.IO.Path.GetFullPath(System.IO.Path.Combine(fullPath, Path)) : null; diff --git a/src/NuGet.Clients/NuGet.VisualStudio.Implementation/SolutionExplorer/Items/PackageReferenceItem.cs b/src/NuGet.Clients/NuGet.VisualStudio.Implementation/SolutionExplorer/Items/PackageReferenceItem.cs index 353bbdf940c..540cf555c7d 100644 --- a/src/NuGet.Clients/NuGet.VisualStudio.Implementation/SolutionExplorer/Items/PackageReferenceItem.cs +++ b/src/NuGet.Clients/NuGet.VisualStudio.Implementation/SolutionExplorer/Items/PackageReferenceItem.cs @@ -44,7 +44,7 @@ internal bool TryUpdateState(AssetsFileTarget target, AssetsFileTargetLibrary li return true; } - private static string GetCaption(AssetsFileTargetLibrary library) => $"{library.Name} ({library.Version})"; + private static string GetCaption(AssetsFileTargetLibrary library) => library.Version is not null ? $"{library.Name} ({library.Version})" : library.Name; public override object Identity => Library.Name; @@ -71,7 +71,7 @@ private sealed class BrowseObject : LocalizableProperties public BrowseObject(PackageReferenceItem item) => _item = item; - public override string GetComponentName() => $"{_item.Library.Name} ({_item.Library.Version})"; + public override string GetComponentName() => _item.Library.Version is not null ? $"{_item.Library.Name} ({_item.Library.Version})" : _item.Library.Name; public override string GetClassName() => VsResources.PackageReferenceBrowseObjectClassName; @@ -81,11 +81,11 @@ private sealed class BrowseObject : LocalizableProperties [BrowseObjectDisplayName(nameof(VsResources.PackageReferenceVersionDisplayName))] [BrowseObjectDescription(nameof(VsResources.PackageReferenceVersionDescription))] - public string Version => _item.Library.Version; + public string? Version => _item.Library.Version; [BrowseObjectDisplayName(nameof(VsResources.PackageReferencePathDisplayName))] [BrowseObjectDescription(nameof(VsResources.PackageReferencePathDescription))] - public string? Path => _item.Target.TryResolvePackagePath(_item.Library.Name, _item.Library.Version, out string? fullPath) ? fullPath : null; + public string? Path => _item.Library.Version is not null && _item.Target.TryResolvePackagePath(_item.Library.Name, _item.Library.Version, out string? fullPath) ? fullPath : null; } } } diff --git a/src/NuGet.Clients/NuGet.VisualStudio.Implementation/SolutionExplorer/Models/AssetsFileDependenciesSnapshot.cs b/src/NuGet.Clients/NuGet.VisualStudio.Implementation/SolutionExplorer/Models/AssetsFileDependenciesSnapshot.cs index 4e77dc64026..9da0d88a966 100644 --- a/src/NuGet.Clients/NuGet.VisualStudio.Implementation/SolutionExplorer/Models/AssetsFileDependenciesSnapshot.cs +++ b/src/NuGet.Clients/NuGet.VisualStudio.Implementation/SolutionExplorer/Models/AssetsFileDependenciesSnapshot.cs @@ -90,13 +90,15 @@ private AssetsFileDependenciesSnapshot(LockFile? lockFile, AssetsFileDependencie previous.DataByTarget.TryGetValue(lockFileTarget.Name, out AssetsFileTarget? previousTarget); + ImmutableArray logMessages = ParseLogMessages(lockFile, previousTarget, lockFileTarget.Name); + dataByTarget.Add( lockFileTarget.Name, new AssetsFileTarget( this, lockFileTarget.Name, - ParseLogMessages(lockFile, previousTarget, lockFileTarget.Name), - ParseLibraries(lockFile, lockFileTarget))); + logMessages, + ParseLibraries(lockFile, lockFileTarget, logMessages))); } DataByTarget = dataByTarget.ToImmutable(); @@ -123,14 +125,14 @@ static ImmutableArray ParseLogMessages(LockFile lockFile, j++; - if (j < previousLogs.Length && previousLogs[j].Equals(logMessage)) + if (j < previousLogs.Length && previousLogs[j].Equals(logMessage, lockFile.PackageSpec.FilePath)) { // Unchanged, so use previous value builder.Add(previousLogs[j]); } else { - builder.Add(new AssetsFileLogMessage(logMessage)); + builder.Add(new AssetsFileLogMessage(lockFile.PackageSpec.FilePath, logMessage)); } } @@ -138,7 +140,7 @@ static ImmutableArray ParseLogMessages(LockFile lockFile, } } - internal static ImmutableDictionary ParseLibraries(LockFile lockFile, LockFileTarget lockFileTarget) + internal static ImmutableDictionary ParseLibraries(LockFile lockFile, LockFileTarget lockFileTarget, ImmutableArray logMessages) { ImmutableDictionary.Builder builder = ImmutableDictionary.CreateBuilder(StringComparer.OrdinalIgnoreCase); @@ -150,6 +152,19 @@ internal static ImmutableDictionary ParseLibrar } } + // If a non-existent library is referenced, it will have an error log message, but no entry in "libraries". + // We want to show a diagnostic node beneath such nodes in the tree, so need to create a dummy library entry, + // otherwise there's nothing to attach that diagnostic to. + foreach (AssetsFileLogMessage message in logMessages) + { + string libraryName = message.LibraryName; + + if (!builder.ContainsKey(libraryName)) + { + builder.Add(libraryName, AssetsFileTargetLibrary.CreatePlaceholder(libraryName)); + } + } + return builder.ToImmutable(); } diff --git a/src/NuGet.Clients/NuGet.VisualStudio.Implementation/SolutionExplorer/Models/AssetsFileLibraryType.cs b/src/NuGet.Clients/NuGet.VisualStudio.Implementation/SolutionExplorer/Models/AssetsFileLibraryType.cs index f2068997920..3908b720872 100644 --- a/src/NuGet.Clients/NuGet.VisualStudio.Implementation/SolutionExplorer/Models/AssetsFileLibraryType.cs +++ b/src/NuGet.Clients/NuGet.VisualStudio.Implementation/SolutionExplorer/Models/AssetsFileLibraryType.cs @@ -11,6 +11,22 @@ namespace NuGet.VisualStudio.SolutionExplorer.Models internal enum AssetsFileLibraryType : byte { Package, - Project + + Project, + + /// + /// When it is unknown whether the library represents a package or a project. + /// + /// + /// + /// This occurs, for example, when a referenced item is not found. It will be present in the lock file's + /// messages, but not elsewhere in the lock file. + /// + /// + /// This enum member exists only to support attaching diagnostic items to their package or project reference + /// node in the tree. + /// + /// + Unknown } } diff --git a/src/NuGet.Clients/NuGet.VisualStudio.Implementation/SolutionExplorer/Models/AssetsFileLogMessage.cs b/src/NuGet.Clients/NuGet.VisualStudio.Implementation/SolutionExplorer/Models/AssetsFileLogMessage.cs index d2e0d388b42..04480f551f6 100644 --- a/src/NuGet.Clients/NuGet.VisualStudio.Implementation/SolutionExplorer/Models/AssetsFileLogMessage.cs +++ b/src/NuGet.Clients/NuGet.VisualStudio.Implementation/SolutionExplorer/Models/AssetsFileLogMessage.cs @@ -3,6 +3,7 @@ #nullable enable +using System.IO; using NuGet.Common; using NuGet.ProjectModel; @@ -13,13 +14,13 @@ namespace NuGet.VisualStudio.SolutionExplorer.Models /// internal readonly struct AssetsFileLogMessage { - public AssetsFileLogMessage(IAssetsLogMessage logMessage) + public AssetsFileLogMessage(string projectFilePath, IAssetsLogMessage logMessage) { Code = logMessage.Code; Level = logMessage.Level; WarningLevel = logMessage.WarningLevel; Message = logMessage.Message; - LibraryName = logMessage.LibraryId; + LibraryName = NormalizeLibraryName(logMessage.LibraryId, projectFilePath); } public NuGetLogCode Code { get; } @@ -28,13 +29,27 @@ public AssetsFileLogMessage(IAssetsLogMessage logMessage) public string Message { get; } public string LibraryName { get; } - public bool Equals(IAssetsLogMessage other) + public bool Equals(IAssetsLogMessage other, string projectFilePath) { return other.Code == Code && other.Level == Level && other.WarningLevel == WarningLevel && other.Message == Message - && other.LibraryId == LibraryName; + && NormalizeLibraryName(other.LibraryId, projectFilePath) == LibraryName; + } + + private static string NormalizeLibraryName(string libraryName, string projectFilePath) + { + // If we have a rooted path for the library in the messages, it is an unresolved project reference. + // Other identifiers for this item will be a relative path with respect to the project. + // So, we compute that relative path here such that it will match correctly. This enables us to + // display diagnostic items beneath unresolved project references in Solution Explorer. + if (Path.IsPathRooted(libraryName)) + { + return PathUtility.GetRelativePath(projectFilePath, libraryName); + } + + return libraryName; } public override string ToString() => $"{Level} {Code} ({LibraryName}) {Message}"; diff --git a/src/NuGet.Clients/NuGet.VisualStudio.Implementation/SolutionExplorer/Models/AssetsFileTarget.cs b/src/NuGet.Clients/NuGet.VisualStudio.Implementation/SolutionExplorer/Models/AssetsFileTarget.cs index 9ac0bd1c660..df339493b00 100644 --- a/src/NuGet.Clients/NuGet.VisualStudio.Implementation/SolutionExplorer/Models/AssetsFileTarget.cs +++ b/src/NuGet.Clients/NuGet.VisualStudio.Implementation/SolutionExplorer/Models/AssetsFileTarget.cs @@ -151,8 +151,8 @@ public bool TryGetPackage(string packageId, string version, [NotNullWhen(returnV Requires.NotNull(version, nameof(version)); if (LibraryByName.TryGetValue(packageId, out assetsFileLibrary) && - assetsFileLibrary.Type == AssetsFileLibraryType.Package && - assetsFileLibrary.Version == version) + assetsFileLibrary.Type is AssetsFileLibraryType.Package or AssetsFileLibraryType.Unknown && + (assetsFileLibrary.Version == version || assetsFileLibrary.Version is null)) { return true; } @@ -166,7 +166,7 @@ public bool TryGetProject(string projectId, [NotNullWhen(returnValue: true)] out Requires.NotNull(projectId, nameof(projectId)); if (LibraryByName.TryGetValue(projectId, out assetsFileLibrary) && - assetsFileLibrary.Type == AssetsFileLibraryType.Project) + assetsFileLibrary.Type is AssetsFileLibraryType.Project or AssetsFileLibraryType.Unknown) { return true; } diff --git a/src/NuGet.Clients/NuGet.VisualStudio.Implementation/SolutionExplorer/Models/AssetsFileTargetLibrary.cs b/src/NuGet.Clients/NuGet.VisualStudio.Implementation/SolutionExplorer/Models/AssetsFileTargetLibrary.cs index 891aa5e8de9..44d68620c69 100644 --- a/src/NuGet.Clients/NuGet.VisualStudio.Implementation/SolutionExplorer/Models/AssetsFileTargetLibrary.cs +++ b/src/NuGet.Clients/NuGet.VisualStudio.Implementation/SolutionExplorer/Models/AssetsFileTargetLibrary.cs @@ -39,6 +39,33 @@ public static bool TryCreate(LockFile lockFile, LockFileTargetLibrary lockFileLi return true; } + /// + /// Creates a dummy placeholder for a library in cases where we only know the name of the library. + /// + /// + /// This is useful, for example, when a referenced package does not exist. It will not have an entry in "libraries", + /// yet we want to model its presence in our snapshot so that we can display diagnostic nodes to it. For such libraries, + /// the version is unknown, which we represent with a value. + /// + public static AssetsFileTargetLibrary CreatePlaceholder(string name) + { + return new AssetsFileTargetLibrary(name); + } + + private AssetsFileTargetLibrary(string name) + { + Name = name; + Version = null; + Type = AssetsFileLibraryType.Unknown; + Dependencies = ImmutableArray.Empty; + FrameworkAssemblies = ImmutableArray.Empty; + CompileTimeAssemblies = ImmutableArray.Empty; + ContentFiles = ImmutableArray.Empty; + BuildFiles = ImmutableArray.Empty; + BuildMultiTargetingFiles = ImmutableArray.Empty; + DocumentationFiles = ImmutableArray.Empty; + } + private AssetsFileTargetLibrary(LockFileLibrary? library, LockFileTargetLibrary targetLibrary, AssetsFileLibraryType type) { Name = targetLibrary.Name; @@ -103,7 +130,15 @@ static bool IsDocumentationFile(string path) } public string Name { get; } - public string Version { get; } + + /// + /// Gets the version of the library, or if it is unknown. + /// + /// + /// The version can be unknown for packages that fail to resolve at all, for example when the package + /// name is not found. For resolved packages however, this value will always be present. + /// + public string? Version { get; } public AssetsFileLibraryType Type { get; } public ImmutableArray Dependencies { get; } public ImmutableArray FrameworkAssemblies { get; } @@ -113,6 +148,6 @@ static bool IsDocumentationFile(string path) public ImmutableArray BuildMultiTargetingFiles { get; } public ImmutableArray DocumentationFiles { get; } - public override string ToString() => $"{Type} {Name} ({Version}) {Dependencies.Length} {(Dependencies.Length == 1 ? "dependency" : "dependencies")}"; + public override string ToString() => $"{Type} {Name} ({Version ?? "Unknown"}) {Dependencies.Length} {(Dependencies.Length == 1 ? "dependency" : "dependencies")}"; } }