dotnet add package updates version in-place even if it was initially defined in an MSBuild property

[kartheekp-ms]

NuGet Product Used

dotnet.exe

Product Version

I believe this issue exists in all the versions.

Worked before?

No response

Impact

It bothers me. A fix would be nice

Repro Steps & Context

Repro steps

1. Create a new class library project.
2. Add a package reference to Newtonsoft.Json as shown below

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
     <!-- existing props -->
    <NewtonsoftJsonVersion>13.0.2</NewtonsoftJsonVersion>
  </PropertyGroup>
  <ItemGroup>
    <PackageReference Include="Newtonsoft.Json" Version="$(NewtonsoftJsonVersion)" /> <!-- Use the version property -->
  </ItemGroup>
</Project>

3. Execute the dotnet add .\classlib.csproj package newtonsoft.json -v 13.0.3 command.

Actual
The version number is updated directly under the PackageReference element instead of updating the NewtonsoftJsonVersion MSBuild property value.

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
     <!-- existing props -->
    <NewtonsoftJsonVersion>13.0.2</NewtonsoftJsonVersion>
  </PropertyGroup>
  <ItemGroup>
    <PackageReference Include="Newtonsoft.Json" Version="13.0.3" /> <!-- Use the version property -->
  </ItemGroup>
</Project>

Expected
The version number should be updated in the corresponding MSBuild property.

Verbose Logs

[jeffkl]

Our code will need to parse the existing unexpanded value of $(Something) and then look for an MSBuild property to update. This can get complicated if a user has conditions as well:

<PropertyGroup>
  <SomePackageVersion Condition="'$(TargetFramework)' == 'net472'">1.0.0</SomePackageVersion>
  <SomePackageVersion Condition="'$(TargetFramework)' != 'net472'">2.0.0</SomePackageVersion>
</PropertyGroup>

<ItemGroup>
  <PackageReference Include="SomePackage" Version="$(SomePackageVersion)" />
</ItemGroup>

The dotnet add package command takes a target framework command-line argument. Ideally, we'd get all of the inner evaluations with the target frameworks and if the value is not the same, require the user to specify the target framework.

We could also log an error if we detect that the user has done something like this and indicate its not supported, if we don't think we can get it right. dotnet add package will need to be much more advanced to handle these advanced scenarios.

[jeffkl]

Here is a prototype I made which makes it work at https://github.com/NuGet/NuGet.Client/blob/313eecd3af442ee2eeed2e6decf310858934ab21/src/NuGet.Core/NuGet.CommandLine.XPlat/Utility/MSBuildAPIUtility.cs?plain=1#L546C1-L547C1:

foreach (var packageReferenceItem in packageReferencesItems)
{
    var packageVersion = libraryDependency.LibraryRange.VersionRange.OriginalString ??
        libraryDependency.LibraryRange.VersionRange.MinVersion.ToString();

    ProjectMetadata metadatum = packageReferenceItem.GetMetadata(VERSION_TAG);

    // Verify that the evaluated and unevaluated values are the same, meaning it is just a plain value like:
    // <PackageReference Include="SomePackage" Version="1.2.3" />
    if (string.Equals(metadatum.EvaluatedValue, metadatum.UnevaluatedValue))
    {
        packageReferenceItem.SetMetadataValue(VERSION_TAG, packageVersion);

        UpdateExtraMetadataInProjectItem(libraryDependency, packageReferenceItem);

        Logger.LogInformation(string.Format(CultureInfo.CurrentCulture,
            Strings.Info_AddPkgUpdated,
            libraryDependency.Name,
            packageVersion,
            packageReferenceItem.Xml.ContainingProject.FullPath));

        return;
    }

    // The declared Version attribute is different after evaluation, it might be something like:
    // <PackageReference Include="SomePackage" Version="$(SomeProperty)" />
    var match = Regex.Match(metadatum.UnevaluatedValue, @"^\$\((?<PropertyName>[\w-]+)\)$");

    if (!match.Groups["PropertyName"].Success)
    {
        // The Version metadata is something other than a simple property reference.  It could be something like:
        // <PackageReference Include="SomePackage" Version="1.2.$(MinorVersion)" />
        // Setting the version in this case would be unsafe, log an error.  Maybe we could add a --force argument to override this?
        return;
    }

    string propertyName = match.Groups["PropertyName"].Value;

    ProjectProperty property = packageReferenceItem.Project.GetProperty(propertyName);

    if (property == null)
    {
        // The property is not defined in the project, log an error
        return;
    }

    if (property.IsImported)
    {
        // The property is defined in some other imported file.  It could be unsafe to edit some random .props file, maybe have a --force argument?
        // Otherwise, log an error
        return;
    }

    if (!string.Equals(property.UnevaluatedValue, property.EvaluatedValue))
    {
        // The property is not just plain text and is defined in terms of other properties, log an error
        // <SomeProperty>$(MinorVersion)</SomeProperty>
        // We could see if its just another property and keep looking but that's probably too advanced, just log an error that its unsupported
        return;
    }

    // Update the in-memory property value, calling project.Save() will persist the change
    packageReferenceItem.Project.SetProperty(propertyName, packageVersion);
}