Add tests for implicit ASP.NET versions, and fix behavior

src/Tasks/Microsoft.NET.Build.Tasks/ApplyImplicitVersions.cs

@@ -6,9 +6,6 @@
 
 namespace Microsoft.NET.Build.Tasks
 {
-
-    //  TODO: Provide way to opt out of warning when Version is specified (possibly with the DisableImplicitFrameworkReferences property)
-    //  TODO: Add behavior (and test) for duplicate PackageReferences
     public sealed class ApplyImplicitVersions : TaskBase
     {
         public string TargetFrameworkVersion { get; set; }

src/Tasks/Microsoft.NET.Build.Tasks/CheckForImplicitPackageReferenceOverrides.cs

@@ -9,8 +9,6 @@ namespace Microsoft.NET.Build.Tasks
 {
     public class CheckForImplicitPackageReferenceOverrides : TaskBase
     {
-        const string MetadataKeyForItemsToRemove = "IsImplicitlyDefined";
-
         [Required]
         public ITaskItem [] PackageReferenceItems { get; set; }
 
@@ -20,21 +18,44 @@ public class CheckForImplicitPackageReferenceOverrides : TaskBase
         [Output]
         public ITaskItem[] ItemsToRemove { get; set; }
 
+        [Output]
+        public ITaskItem[] ItemsToAdd { get; set; }
+
         protected override void ExecuteCore()
         {
             var duplicateItems = PackageReferenceItems.GroupBy(i => i.ItemSpec, StringComparer.OrdinalIgnoreCase).Where(g => g.Count() > 1);
-            var duplicateItemsToRemove = duplicateItems.SelectMany(g => g.Where(
-                item => item.GetMetadata(MetadataKeyForItemsToRemove).Equals("true", StringComparison.OrdinalIgnoreCase)));
 
-            ItemsToRemove = duplicateItemsToRemove.ToArray();
-
-            foreach (var itemToRemove in ItemsToRemove)
+            if (duplicateItems.Any())
             {
-                string message = string.Format(CultureInfo.CurrentCulture, Strings.PackageReferenceOverrideWarning,
-                    itemToRemove.ItemSpec,
-                    MoreInformationLink);
-
-                Log.LogWarning(message);
+                List<ITaskItem> itemsToRemove = new List<ITaskItem>();
+                List<ITaskItem> itemsToAdd = new List<ITaskItem>();
+                foreach (var duplicateItemGroup in duplicateItems)
+                {
+                    foreach (var item in duplicateItemGroup)
+                    {
+                        if (item.GetMetadata(MetadataKeys.IsImplicitlyDefined).Equals("true", StringComparison.OrdinalIgnoreCase))
+                        {
+                            itemsToRemove.Add(item);
+                            string message = string.Format(CultureInfo.CurrentCulture, Strings.PackageReferenceOverrideWarning,
+                                item.ItemSpec,
+                                MoreInformationLink);
+
+                            Log.LogWarning(message);
+                        }
+                        else
+                        {
+                            //  For the explicit items, we want to add metadata to them so that the ApplyImplicitVersions task
+                            //  won't generate another error.  The easiest way to do this is to add them both to a list of
+                            //  items to remove, and then a list of items which gets added back.
+                            itemsToRemove.Add(item);
+                            item.SetMetadata(MetadataKeys.AllowExplicitVersion, "true");
+                            itemsToAdd.Add(item);
+                        }
+                    }
+                }
+
+                ItemsToRemove = itemsToRemove.ToArray();
+                ItemsToAdd = itemsToAdd.ToArray();
             }
         }
     }

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.DefaultItems.targets

@@ -198,16 +198,14 @@ Copyright (c) .NET Foundation. All rights reserved.
         PackageReferenceItems="@(PackageReference)"
         MoreInformationLink="$(ImplicitPackageReferenceInformationLink)">
       <Output TaskParameter="ItemsToRemove" ItemName="_PackageReferenceToRemove" />
+      <Output TaskParameter="ItemsToAdd" ItemName="_PackageReferenceToAdd" />
     </CheckForImplicitPackageReferenceOverrides>
 
     <ItemGroup>
-      <!-- Note that the condition here is important, otherwise the Remove will operate based just on item identity and remove all items
-           that had duplicates, instead of leaving the ones without IsImplicitlyDefined set to true. -->
-      <PackageReference Remove="@(_PackageReferenceToRemove)" Condition="'%(PackageReference.IsImplicitlyDefined)' == 'true' "/>
-
-      <!-- If we've already warned that an implicit PackageReference was overridden, don't also warn that the version was explicitly
-           specified -->
-      <PackageReference Update="@(_PackageReferenceToRemove)" AllowExplicitVersion="true"/>
+      <!-- Remove and add the PackageReference items according to the output from the task -->
+      <PackageReference Remove="@(_PackageReferenceToRemove)" />
+
+      <PackageReference Include="@(_PackageReferenceToAdd)" />
     </ItemGroup>
 
     <!-- If any implicit package references were overridden, then don't check that RuntimeFrameworkVersion matches the package version -->

src/Tests/Microsoft.NET.Build.Tests/ImplicitAspNetVersions.cs

@@ -0,0 +1,208 @@
+using System;
+using System.Collections.Generic;
+using System.IO;
+using System.Linq;
+using System.Text;
+using FluentAssertions;
+using Microsoft.NET.TestFramework;
+using Microsoft.NET.TestFramework.Assertions;
+using Microsoft.NET.TestFramework.Commands;
+using Microsoft.NET.TestFramework.ProjectConstruction;
+using NuGet.Common;
+using NuGet.Frameworks;
+using NuGet.ProjectModel;
+using NuGet.Versioning;
+using Xunit;
+using Xunit.Abstractions;
+
+namespace Microsoft.NET.Build.Tests
+{
+    public class ImplicitAspNetVersions : SdkTest
+    {
+        public ImplicitAspNetVersions(ITestOutputHelper log) : base(log)
+        {
+        }
+
+        [Theory]
+        [InlineData("Microsoft.AspNetCore.App")]
+        [InlineData("Microsoft.AspNetCore.All")]
+        public void AspNetCoreVersionIsSetImplicitly(string aspnetPackageName)
+        {
+            var testProject = new TestProject()
+            {
+                Name = "AspNetImplicitVersion",
+                TargetFrameworks = "netcoreapp2.1",
+                IsSdkProject = true,
+                IsExe = true
+            };
+
+            //  Add versionless PackageReference
+            testProject.PackageReferences.Add(new TestPackageReference(aspnetPackageName, null));
+
+            var testAsset = _testAssetsManager.CreateTestProject(testProject, identifier: aspnetPackageName)
+                .Restore(Log, testProject.Name);
+
+            var buildCommand = new BuildCommand(Log, Path.Combine(testAsset.TestRoot, testProject.Name));
+
+            buildCommand
+                .Execute()
+                .Should()
+                .Pass();
+
+            var aspnetVersion = GetLibraryVersion(testProject, buildCommand, aspnetPackageName);
+
+            //  Version of AspNetCore packages is 2.1.1 because 2.1.0 packages had exact version constraints, which was broken
+            aspnetVersion.ToString().Should().Be("2.1.1");
+        }
+
+        [Theory]
+        [InlineData("Microsoft.AspNetCore.App")]
+        [InlineData("Microsoft.AspNetCore.All")]
+        public void AspNetCoreVersionRollsForward(string aspnetPackageName)
+        {
+            var testProject = new TestProject()
+            {
+                Name = "AspNetImplicitVersion",
+                TargetFrameworks = "netcoreapp2.1",
+                IsSdkProject = true,
+                IsExe = true,
+
+            };
+
+            testProject.RuntimeIdentifier = EnvironmentInfo.GetCompatibleRid(testProject.TargetFrameworks);
+
+            //  Add versionless PackageReference
+            testProject.PackageReferences.Add(new TestPackageReference(aspnetPackageName, null));
+
+            var testAsset = _testAssetsManager.CreateTestProject(testProject, identifier: aspnetPackageName)
+                .Restore(Log, testProject.Name);
+
+            var buildCommand = new BuildCommand(Log, Path.Combine(testAsset.TestRoot, testProject.Name));
+
+            buildCommand
+                .Execute()
+                .Should()
+                .Pass();
+
+            var aspnetVersion = GetLibraryVersion(testProject, buildCommand, aspnetPackageName);
+
+            //  Self-contained app (because RID is specified) should roll forward to later patch
+            aspnetVersion.CompareTo(new SemanticVersion(2, 1, 1)).Should().BeGreaterThan(0);
+        }
+
+        [Theory]
+        [InlineData("Microsoft.AspNetCore.App")]
+        [InlineData("Microsoft.AspNetCore.All")]
+        public void ExplicitVersionsOfAspNetCoreWarn(string aspnetPackageName)
+        {
+            var testProject = new TestProject()
+            {
+                Name = "AspNetExplicitVersion",
+                TargetFrameworks = "netcoreapp2.1",
+                IsSdkProject = true,
+                IsExe = true
+            };
+
+            string explicitVersion = "2.1.0";
+
+            testProject.PackageReferences.Add(new TestPackageReference(aspnetPackageName, explicitVersion));
+
+            var testAsset = _testAssetsManager.CreateTestProject(testProject, identifier: aspnetPackageName)
+                .Restore(Log, testProject.Name);
+
+            var buildCommand = new BuildCommand(Log, Path.Combine(testAsset.TestRoot, testProject.Name));
+
+            buildCommand
+                .Execute()
+                .Should()
+                .Pass()
+                .And
+                .HaveStdOutContaining("NETSDK1071");
+
+            var aspnetVersion = GetLibraryVersion(testProject, buildCommand, aspnetPackageName);
+
+            aspnetVersion.ToString().Should().Be(explicitVersion);
+        }
+
+        [Theory]
+        [InlineData("netcoreapp2.0", "Microsoft.AspNetCore.All", "2.0.9")]
+        [InlineData("netcoreapp1.1", "Microsoft.AspNetCore", "1.1.7")]
+        public void ExplicitVersionsDontWarnForOlderVersions(string targetFramework, string packageName, string packageVersion)
+        {
+            var testProject = new TestProject()
+            {
+                Name = "AspNetPreviousVersion",
+                TargetFrameworks = targetFramework,
+                IsSdkProject = true,
+                IsExe = true
+            };
+
+            testProject.PackageReferences.Add(new TestPackageReference(packageName, packageVersion));
+
+            var testAsset = _testAssetsManager.CreateTestProject(testProject, identifier: targetFramework)
+                .Restore(Log, testProject.Name);
+
+            var buildCommand = new BuildCommand(Log, Path.Combine(testAsset.TestRoot, testProject.Name));
+
+            buildCommand
+                .Execute()
+                .Should()
+                .Pass()
+                .And
+                .NotHaveStdOutContaining("warning");
+
+            var aspnetVersion = GetLibraryVersion(testProject, buildCommand, packageName);
+
+            aspnetVersion.ToString().Should().Be(packageVersion);
+        }
+
+        [Fact]
+        public void MultipleWarningsAreGeneratedForMultipleExplicitReferences()
+        {
+            var testProject = new TestProject()
+            {
+                Name = "MultipleExplicitReferences",
+                TargetFrameworks = "netcoreapp2.1",
+                IsSdkProject = true,
+                IsExe = true
+            };
+
+            testProject.PackageReferences.Add(new TestPackageReference("Microsoft.NETCore.App", "2.1.0"));
+            testProject.PackageReferences.Add(new TestPackageReference("Microsoft.AspNetCore.App", "2.1.0"));
+
+            var testAsset = _testAssetsManager.CreateTestProject(testProject);
+
+            var restoreCommand = new RestoreCommand(Log, Path.Combine(testAsset.TestRoot, testProject.Name));
+            restoreCommand
+                .Execute()
+                .Should()
+                .Pass()
+                .And
+                .NotHaveStdOutContaining("NETSDK1071");
+
+
+            var buildCommand = new BuildCommand(Log, Path.Combine(testAsset.TestRoot, testProject.Name));
+
+            buildCommand
+                .Execute()
+                .Should()
+                .Pass()
+                .And
+                .HaveStdOutContaining("NETSDK1071")
+                .And
+                .HaveStdOutContaining("NETSDK1023");
+        }
+
+        static NuGetVersion GetLibraryVersion(TestProject testProject, BuildCommand buildCommand, string libraryName)
+        {
+            LockFile lockFile = LockFileUtilities.GetLockFile(
+                Path.Combine(buildCommand.GetBaseIntermediateDirectory().FullName, "project.assets.json"),
+                NullLogger.Instance);
+
+            var target = lockFile.GetTarget(NuGetFramework.Parse(testProject.TargetFrameworks), testProject.RuntimeIdentifier);
+            var lockFileLibrary = target.Libraries.Single(l => l.Name == libraryName);
+
+            return lockFileLibrary.Version;
+        }
+    }
+}

src/Tests/Microsoft.NET.TestFramework/ProjectConstruction/TestProject.cs

@@ -145,9 +145,13 @@ internal void Create(TestAsset targetTestAsset, string testProjectsSourceFolder)
 
             foreach (TestPackageReference packageReference in PackageReferences)
             {
-                packageReferenceItemGroup.Add(new XElement(ns + "PackageReference",
-                    new XAttribute("Include", $"{packageReference.ID}"),
-                    new XAttribute("Version", $"{packageReference.Version}")));
+                var packageReferenceElement = new XElement(ns + "PackageReference",
+                    new XAttribute("Include", packageReference.ID));
+                if (packageReference.Version != null)
+                {
+                    packageReferenceElement.Add(new XAttribute("Version", packageReference.Version));
+                }
+                packageReferenceItemGroup.Add(packageReferenceElement);
             }
 
             foreach (TestPackageReference dotnetCliToolReference in DotNetCliToolReferences)