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.

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

[DCR]: NuGet causes many ArgumentExceptions to be thrown & caught in VS #11535

Closed
zivkan opened this issue Jan 28, 2022 · 0 comments · Fixed by NuGet/NuGet.Client#5834
Closed
Assignees
Labels
Product:VS.Client Type:DCR Design Change Request
Milestone

Comments

@zivkan
Copy link
Member

zivkan commented Jan 28, 2022

NuGet Product(s) Affected

Visual Studio Package Management UI, Visual Studio Package Manager Console

Current Behavior

src/NuGet.Clients/NuGet.PackageManagement.VisualStudio/ProjectServices/VsProjectBuildProperties.cs tries to get MSBuild properties from IVsBuildPropertyStorage, and if it's unable to get the property (even if the API returns a code saying the property does not exist), NuGet then tries DTE.Project.Properties.Item(string). However, DTE throws an InvalidArgumentException when the property does not exist in the project.

While the exception is caught, it has two problems:

  1. There's a minor perf problem. Not only are we pointlessly requesting a property we already know doesn't exist, but throwing and catching exceptions has non-trivial overhead, which is why many .NET APIs have Try* methods.
  2. VS extension authors (which includes the NuGet team and everyone in Microsoft writing components that ship in VS) have these first chance exceptions appear in Visual Studio's debugging diagnostics view when debugging VS. This has even affected me personally, when trying to debug some bug fix or new feature, and it's difficult to figure out which exceptions are relevant to my scenario, and which exceptions are just noise.

Several times internal partners, including the VS perf team, have opened bugs on NuGet for throwing these exceptions, despite the fact that DTE is actually throwing them, due to technicalities of how COM interop work.

Desired Behavior

NuGet should only use DTE.Project.Properties when the project system doesn't support IVsBuildPropertyStorage.

Additional Context

This change has medium risk, as it makes the assumption that project systems that implement IVsbuildPropertyStorage have done so correctly, and that using DTE.Project.Properties.Item(string) will also not find the property. It's out of the NuGet team's control whether or project systems implement themselves correctly, and since NuGet has been doing this fallback behaviour "forever", if NuGet changes and this exposes a project system bug, it will appear as a NuGet regression.

Internal issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment