Add support for transitive NoWarn properties from project references #1642

mishra14 · 2017-08-11T21:41:51Z

Spec: https://github.com/NuGet/Home/wiki/%5BSpec%5D-Transitive-Warning-Properties
Fixes: NuGet/Home#5691 and NuGet/Home#5501

This change adds the capability to flow NoWarn properties transitively. The details are in the spec along with a high level implementation detail section.

emgarten · 2017-08-11T22:07:25Z

test/TestUtilities/Test.Utility/TestExtensions.cs

+{
+    public static class TestExtensions
+    {
+        public static int GetSubstringCount(this string str, string substr, bool ignoreCase)


This is going to show up everywhere in test projects, make this a regular static method instead of an extension method. For classes such as string it is just too painful.

emgarten · 2017-08-11T22:11:14Z

src/NuGet.Core/NuGet.Commands/RestoreCommand/Logging/PackageSpecificWarningProperties.cs

+        /// <param name="code">NuGetLogCode for which no warning should be thrown.</param>
+        /// <param name="libraryId">Library for which no warning should be thrown.</param>
+        /// <param name="frameworks">IEnumerable of Target graph for which no warning should be thrown.</param>
+        public void AddRange(NuGetLogCode code, string libraryId, IEnumerable<NuGetFramework> frameworks)


The name AddRange is confusing here since it is doing a lot more than other AddRange methods do. I would just call this Add or AddCode

emgarten · 2017-08-11T22:13:57Z

src/NuGet.Core/NuGet.Commands/RestoreCommand/Logging/PackageSpecificWarningProperties.cs

+            var hashCode = new HashCodeCombiner();
+
+            // Add a constant hash for all objects
+            hashCode.AddObject(1);


Why does this method not match Equals? They should both check the same things.

This seems acceptable as per the requirements on HashCode -

if x equals y then the hash code of x must equal the hash code of y. Equivalently: if the hash code of x does not equal the hash code of y then x and y must be unequal.

the hash code of x must remain stable while x is in a hash table.

Ref - https://stackoverflow.com/questions/19710028/what-to-return-when-overriding-object-gethashcode-in-classes-with-no-immutable

emgarten · 2017-08-11T22:14:58Z

src/NuGet.Core/NuGet.Commands/RestoreCommand/Logging/PackageSpecificWarningProperties.cs

+                return true;
+            }
+
+            return Properties.Keys.OrderedEquals(other.Properties.Keys, (code) => code) &&


Use EqualityUtilities it does this same thing with additional error handling, and it has already been optimized

emgarten · 2017-08-11T22:34:43Z

src/NuGet.Core/NuGet.Commands/RestoreCommand/Logging/PackageSpecificWarningProperties.cs

+            var targetFrameworkInformation = packageSpec
+                .TargetFrameworks
+                .Where(tfi => tfi.FrameworkName == framework)
+                .First();


You can replace Where with First here and it will handle both.

emgarten · 2017-08-11T22:46:21Z

src/NuGet.Core/NuGet.Commands/RestoreCommand/Logging/WarningPropertiesCollection.cs

@@ -61,7 +70,8 @@ public bool ApplyWarningProperties(IRestoreLogMessage message)
                    if (messageTargetFrameworks.Count == 0)
                    {
                        // Suppress the warning if the code + libraryId combination is suppressed for all project frameworks.
-                        if (ProjectFrameworks.Count > 0 &&
+                        if (ProjectFrameworks != null &&


Why would a project not have frameworks? This should be validated earlier and WarningPropertiesCollection should not be created if the project is invalid. Having to recheck this everywhere in the code makes it seem like there are bugs here.

emgarten · 2017-08-11T22:48:49Z

src/NuGet.Core/NuGet.Commands/RestoreCommand/Logging/TransitiveNoWarnUtils.cs

+
+namespace NuGet.Commands
+{
+    internal class TransitiveNoWarnUtils


internal static

emgarten · 2017-08-11T22:51:26Z

src/NuGet.Core/NuGet.Commands/RestoreCommand/Logging/TransitiveNoWarnUtils.cs

+    internal class TransitiveNoWarnUtils
+    {
+        // static should be fine across multiple restore calls as this solely depends on the csproj file of the project.
+        private static readonly ConcurrentDictionary<string, WarningPropertiesCollection> _warningPropertiesCache =


🔥 Statics should not be used for caching.

Users often leave VS open for weeks at a time, this is a memory leak that will continue growing until VS is closed.

I also don't see this being updated, which means that if a user changes their project and restores again it won't have the updated warning properties.

emgarten · 2017-08-12T00:13:50Z

src/NuGet.Core/NuGet.Commands/RestoreCommand/Logging/TransitiveNoWarnUtils.cs

+                }
+
+                return string.Equals(Id, other.Id, StringComparison.OrdinalIgnoreCase) &&
+                    IsProject == other.IsProject &&


order this by bools first

emgarten · 2017-08-12T00:24:23Z

src/NuGet.Core/NuGet.Commands/RestoreCommand/Logging/TransitiveNoWarnUtils.cs

+
+        private static PackageSpec GetNodePackageSpec(LocalMatch localMatch)
+        {
+            return localMatch.LocalLibrary.Items[KnownLibraryProperties.PackageSpec] as PackageSpec;


Use (PackageSpec) instead

…tadata

…y equality

mishra14 · 2017-08-23T01:58:37Z

@emgarten 🔔

emgarten · 2017-08-25T21:54:17Z

src/NuGet.Core/NuGet.Commands/RestoreCommand/Logging/TransitiveNoWarnUtils.cs

+
+namespace NuGet.Commands
+{
+    public class TransitiveNoWarnUtils


Could this be a static class? It seems like the only reason this isn't currently is because of the cache, but if there were a method that took all graphs at once it could just pass in a cache to the rest of the methods using it.

emgarten · 2017-08-25T21:55:14Z

src/NuGet.Core/NuGet.Commands/RestoreCommand/Logging/TransitiveNoWarnUtils.cs

+                if (!seen.Contains(node))
+                {
+                    // Add the node to seen set
+                    seen.Add(node);


Use seen.Add(node) above, it returns true if seen.Contains was false, this code is currently doing the look up twice

emgarten · 2017-08-25T21:55:43Z

src/NuGet.Core/NuGet.Commands/RestoreCommand/Logging/TransitiveNoWarnUtils.cs

+    public class TransitiveNoWarnUtils
+    {
+        // static should be fine across multiple calls as this solely depends on the csproj file of the project.
+        private readonly ConcurrentDictionary<string, ConcurrentDictionary<NuGetFramework, WarningPropertiesCollection>> _warningPropertiesPerFrameworkCache =


Since this isn't done in parallel I this should be a normal dictionary, or a sorteddictionary

emgarten · 2017-08-25T21:56:17Z

src/NuGet.Core/NuGet.Commands/RestoreCommand/Logging/TransitiveNoWarnUtils.cs

+            // All the packages in parent project's closure. 
+            // Once we have collected data for all of these, we can exit.
+            var parentPackageDependencies = new HashSet<string>(
+                targetGraph.Flattened.Where(d => d.Key.Type == LibraryType.Package).Select(d => d.Key.Name));


This should be created as part of the flattened loop below, it isn't used until later

This also looks incorrect since it is case sensitive by default, if this only checks the exact id because there can only be 1 instance of the id then it should use Ordinal instead of the culture specific default.

rohit21agrawal · 2017-08-25T23:34:58Z

src/NuGet.Core/NuGet.Commands/RestoreCommand/Logging/TransitiveNoWarnUtils.cs

+
+            // All the packages in parent project's closure. 
+            // Once we have collected data for all of these, we can exit.
+            var parentPackageDependencies = new HashSet<string>(


use StringComparer.OrdinalIgnoreCase?

mishra14 · 2017-08-28T21:39:45Z

Closing in the favor of #1672

mishra14 requested review from rohit21agrawal, PatoBeltran, nkolev92, emgarten, zhili1208 and jainaashish August 11, 2017 21:41

dnfclas added the cla-already-signed label Aug 11, 2017

mishra14 changed the title ~~Dev anmishr p2pnowarn~~ Add support for transitive NoWarn properties from project references Aug 11, 2017

emgarten reviewed Aug 12, 2017

View reviewed changes

mishra14 force-pushed the dev-anmishr-p2pnowarn branch 2 times, most recently from 69bece7 to bb16361 Compare August 21, 2017 22:41

Ankit Mishra added 12 commits August 21, 2017 18:29

temp status

e3e150f

working through merging warning properties

e25192e

Adding better quality across and changing tests

dbb4e78

checkpoint for correctly extracting transitive no warn from a basic case

3512c8a

Checkpoint with a functional case (A->B->C->Pkg) and lazy initialization

5bac353

Adding code to deal with path exclusion logic and fixing test breaks

7b06260

making TransitiveNoWarnUtils conditional on projectspec and restoreme…

32dd874

…tadata

Adding scenario tests

b41d9d3

addressing some PR feedback

d2d095a

Addressing warning properties related PR feedback

aff0915

Addressing feedback on TransitiveNoWarnUtil

fd2954d

Adding unit tests for TransitiveNoWarnUtils

2bb5484

mishra14 force-pushed the dev-anmishr-p2pnowarn branch from adfd4c6 to 2bb5484 Compare August 22, 2017 01:30

Ankit Mishra added 2 commits August 22, 2017 11:51

Adding more unit tests

bac6ebd

Adding unit tests related to Merging warning properties and dependenc…

206783d

…y equality

Ankit Mishra added 3 commits August 23, 2017 20:06

Improving perf by fixing seen set handling

2aca4d5

Adding func tests with dense solution

288abda

removing warnAsError from merge

b2610a1

Ankit Mishra added 2 commits August 25, 2017 10:55

fixing tests to match the perf changes

6f725f7

Reducung visible propertites in restore collector logger

1697f19

mishra14 force-pushed the dev-anmishr-p2pnowarn branch from 68a828d to 1697f19 Compare August 25, 2017 21:05

emgarten reviewed Aug 25, 2017

View reviewed changes

rohit21agrawal reviewed Aug 25, 2017

View reviewed changes

mishra14 mentioned this pull request Aug 26, 2017

Add support for transitive NoWarn properties from project references #1672

Merged

mishra14 closed this Aug 28, 2017

mishra14 deleted the dev-anmishr-p2pnowarn branch August 29, 2017 23:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for transitive NoWarn properties from project references #1642

Add support for transitive NoWarn properties from project references #1642

mishra14 commented Aug 11, 2017

emgarten Aug 11, 2017

emgarten Aug 11, 2017

emgarten Aug 11, 2017

mishra14 Aug 23, 2017 •

edited

Loading

emgarten Aug 11, 2017

emgarten Aug 11, 2017

emgarten Aug 11, 2017

emgarten Aug 11, 2017

emgarten Aug 11, 2017

emgarten Aug 12, 2017

emgarten Aug 12, 2017

mishra14 commented Aug 23, 2017

emgarten Aug 25, 2017

emgarten Aug 25, 2017

emgarten Aug 25, 2017

emgarten Aug 25, 2017

emgarten Aug 25, 2017

rohit21agrawal Aug 25, 2017

mishra14 commented Aug 28, 2017

Add support for transitive NoWarn properties from project references #1642

Add support for transitive NoWarn properties from project references #1642

Conversation

mishra14 commented Aug 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mishra14 Aug 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mishra14 commented Aug 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mishra14 commented Aug 28, 2017

mishra14 Aug 23, 2017 •

edited

Loading