Show diagnostic beneath unresolved package/project reference in Solution Explorer

[drewnoakes]

Bug

Fixes dotnet/project-system#8947
Fixes dotnet/project-system#8998

Regression? Last working version: No

Description

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.

Before

After

When one of the diagnostic nodes is focussed, the Properties window shows more information.

PR Checklist

• PR has a meaningful title

• PR has a linked issue.

• Described changes

• Tests

  • Automated tests added
  • OR
  • Test exception
  • OR
  • N/A

• Documentation

  • Documentation PR or issue filled
  • OR
  • N/A

[drewnoakes]

@kartheekp-ms any feedback on this PR? I'd like to get it in early for 17.7p2 if possible.

I'll probably have one or two more PRs for tree nodes coming for that release too.

[drewnoakes]

I'll probably have one or two more PRs for tree nodes coming for that release too.

Actually the first of these will conflict with this one, so I'll extend this PR.

[drewnoakes]

Actually the first of these will conflict with this one, so I'll extend this PR.

Done in fe0f605:

Don't include version in package identity

Changes to the Dependencies tree in 17.7 will mean that when changing a package's version, the tree node is no longer removed and re-added, but rather updated in place.

Before this change, the package's version was part of its identity. This meant new data for an updated version would not be applied to the node again. Previously this wasn't an issue, as we recreated the node. But now we re-use it, we want the data from a different version to be applied to the tree.

This change enables that.

[kartheekp-ms]

I tested the behavior introduced in this PR by launching the extension from your branch. To ensure that the code is reliable and robust, would it be possible to add some tests to this PR?

Also, I think you should rebase your branch on top of upstream dev branch to fix some CI failures.

[kartheekp-ms]

I think using Unknown can be confusing because the non-existant type is either Package or Project. To make the code easier to understand, we could consider adding a readonly property called LibraryType of type AssetsFileLibraryType to AssetsFileLogMessage. This would allow us to use AssetsFileLogMessage.LibraryType instead of Unknown when creating a placeholder or dummy AssetFileTargetLibrary object. By implementing this change, we could eliminate the need to check for Unknown in TryGetPackage or TryGetProject methods. What do you think? Please let me know if my understanding is incorrect.

[drewnoakes]

Unfortunately we still wouldn't know what value to put in a AssetsFileLogMessage.LibraryType property. The project.assets.json file doesn't contain enough information to model that. Hence the Unknown value.

For example, here's are the log messages for a package and project, respectively:

"logs": [
  {
    "code": "NU1101",
    "level": "Error",
    "message": "Unable to find package NonExistantPackage. No packages exist with this id in source(s): C:\\Program Files\\dotnet\\library-packs, Microsoft Visual Studio Offline Packages, NuGet local, nuget.org",
    "libraryId": "NonExistantPackage",
    "targetGraphs": [
      "net7.0"
    ]
  },
  {
    "code": "NU1104",
    "level": "Error",
    "message": "Unable to find project 'D:\\repos\\issues\\Issue8947UnresolvedPackageWarningMissing\\NonExistantProject\\NonExistantProject.csproj'. Check that the project reference is valid and that the project file exists.",
    "libraryId": "D:\\repos\\issues\\Issue8947UnresolvedPackageWarningMissing\\NonExistantProject\\NonExistantProject.csproj",
    "targetGraphs": [
      "net7.0"
    ]
  }
]

[kartheekp-ms]

Sorry for the confusion. I should have been clearer. Given that Path.IsRooted("libraryId") will be true for projects but not for packages, I think we can identify whether the library is a package or project easily.

    /// <summary>
    /// Models a diagnostic found in the assets file. Immutable.
    /// </summary>
    internal readonly struct AssetsFileLogMessage
    {
      // remaining code
        public AssetFileTargetLibrary LibraryType { get; } 
        public AssetsFileLogMessage(string projectFilePath, IAssetsLogMessage logMessage)
        {
            Code = logMessage.Code;
            Level = logMessage.Level;
            WarningLevel = logMessage.WarningLevel;
            Message = logMessage.Message;
            LibraryName = NormalizeLibraryName(logMessage.LibraryId, projectFilePath);
            LibraryType = Path.IsRooted(logMessage.LibraryId) ? AssetFileTargetLibrary.Project : AssetFileTargetLibrary.Package
        }
    //remaining code
    }

With these changes, when we are creating the dummy/placeholder object, we can pass logmessage.LibraryType instead of using Unknown as the LibraryType property for AssetFileTargetLibrary. This allows us to eliminate the need for checking if the library type is unknown.

        public static AssetsFileTargetLibrary CreatePlaceholder(string name, AssetFileTargetLibrary libraryType)
        {
            return new AssetsFileTargetLibrary(name, libraryType);
        }

        private AssetsFileTargetLibrary(string name, AssetFileTargetLibrary libraryType)
        {
            Name = name;
            Version = null;
            Type = libraryType;
            Dependencies = ImmutableArray<string>.Empty;
            FrameworkAssemblies = ImmutableArray<string>.Empty;
            CompileTimeAssemblies = ImmutableArray<string>.Empty;
            ContentFiles = ImmutableArray<AssetsFileTargetLibraryContentFile>.Empty;
            BuildFiles = ImmutableArray<string>.Empty;
            BuildMultiTargetingFiles = ImmutableArray<string>.Empty;
            DocumentationFiles = ImmutableArray<string>.Empty;
        }

[drewnoakes]

Oh I see. I guess it comes down to whether you think and absolute path is a good indicator that is a project or package. I don't know it to be, so prefer to capture that uncertainty in the data. It also means the unknown flag helps explain why so many other fields in the object have no value.

From a design point of view I prefer modelling that uncertainty explicitly, even if it means an extra check in two places.

A better fix overall would be for the lock file to include the library type in the log message JSON. That must be known by the writer of the file, but then that information is not serialised and is therefore lost to consumers.

[drewnoakes]

would it be possible to add some tests to this PR?

There's not much actual logic here to test as the attached collections stuff is quite declarative. The most valuable testing for these code paths would be integration tests, which my team is investigating at the moment.

I've added some tests for the new parsing behaviour.

Also, I think you should rebase your branch on top of upstream dev branch to fix some CI failures.

Done. I've pushed, but CI doesn't run automatically for my PRs unfortunately so I cannot tell whether it'll pass or not.

[kartheekp-ms]

The most valuable testing for these code paths would be integration tests, which my team is investigating at the moment.

Thank you. Is there any tracking issue?

[drewnoakes]

Is there any tracking issue?

Discussed offline.