Handle when same name and same version both project and package are referenced in solution #3789

erdembayar · 2020-12-08T23:15:54Z

Bug

Fixes: Issue#6795
Regression: No

Last working version:
How are we preventing it in future:

Fix

Details: The problem occurs project and package with same name and same version referenced directly and transitively to project. Let's call them "DoublyReferenced" for both project and package and with version is "1.0.0"

   Source  - - - - - ProjectInterMed -> Package(DoublyReferenced) V1.00 (transitive ref)
             \
               \    
                 \ - Project(DoublyReferenced) V1.00 (direct ref)

Then it fails with vague error "" in VS:

Even on cli with dotnet.exe it fails with another vague error, actually it's telling duplicate same name keys are created and we have conflict.

Currently below scenario works, after examination it actual doesn't create key duplicate key as our case, only one key for project created due to project over package preference. It looks very similar to our case in question but actually it's not.

   Source  - - - - - -> Package(DoublyReferenced) V1.00 (direct ref)
             \
               \    
                 \ - Project(DoublyReferenced) V1.00 (direct ref, winner)

Even it works it cause squiggly yellow warning with Roslyn.

There're only 2 solutions to this problem.

Try to solve issue by choosing one over other (package vs project). Since we already have project over package preference I currently implemented this approach in my code.
Still this one have issues:
a. Above Roslyn warning keep continue to show up with or without this fix until we remove one of them.

b. Project reference always winner, it's implicit to user and maybe it's not user actually wanted, maybe yes. All different combinations of direct and transient references works except below 1 case.
c. If package direct reference and project is transitive reference then VS doesn't even compile

Show explicit warning/error and let user know there are both project and package with same name/version referenced so user can take action. Since main complain of customers was message was too implicit/vague this will definitely solve the pain point.
Maybe this one is most clean and confusion free solution. I can change to implementation, please just give me your feedback.
Also in this case do we need to introduce new warning NUxxxx code?

Testing/Validation

Tests Added: Yes
Reason for not adding tests:
Validation:

ericsampson · 2020-12-22T01:10:43Z

This would be great to have a better error message/handling of this, I just hit it and it took me a while to figure out what was going on. Cheers!

erdembayar · 2021-02-04T00:52:21Z

@NuGet/nuget-client
Do we need to show warning message to user that we picked up project over package in this case instead of doing it silently?

Also I investigated issue#10368 related this but it requires design change, and ROI is very small since it's just uncommon edge case. So want to keep fix as it's. I'm working on adding unit test.

nkolev92 · 2021-02-04T01:31:43Z

Do we need to show warning message to user that we picked up project over package in this case instead of doing it silently?

Imo, not as part of this change. That's not something we want to introduce at this point as no one is really asking for it.

erdembayar · 2021-02-05T01:18:07Z

@NuGet/nuget-client
Ready for review, please review.

nkolev92 · 2021-02-05T01:27:25Z

@erdembayar

Can you please:

Fix the title to indicate what the change is, not Fix {issue title}.
Add a more detailed description about your changes. What will the experience be like now and why you made that decision.

erdembayar · 2021-02-05T18:32:11Z

@erdembayar

Can you please:

Fix the title to indicate what the change is, not Fix {issue title}.

Add a more detailed description about your changes. What will the experience be like now and why you made that decision.

Can you check now? Now I more toward to show warning/error direction instead of solve problem by choose one over other (project vs package). By showing clear warning/error we make user take a action to solve problem for good.
Initial assumption of this is just bug was below direct referencing scenario works. But it's not really working (Roslyn warning) and bit different problem:

   Source  - - - - - -> Package(DoublyReferenced) V1.00 (direct ref)
             \
               \    
                 \ - Project(DoublyReferenced) V1.00 (direct ref, winner)

nkolev92 · 2021-02-05T22:42:37Z

By showing clear warning/error we make user take a action to solve problem for good.

Project was preferred over Package, but not in all scenarios.

Fixing this bug just establishes the already known expectation.
Adding more visibility to projects being preferred over packages is something that's lateral to the current problem.

erdembayar · 2021-02-05T22:52:46Z

By showing clear warning/error we make user take a action to solve problem for good.

Project was preferred over Package, but not in all scenarios.

Fixing this bug just establishes the already known expectation.
Adding more visibility to projects being preferred over packages is something that's lateral to the current problem.

Can you recommend next course of action? Is my current fix I submitted ok for bug fix? Do I need to change anything?

nkolev92 · 2021-02-05T23:26:15Z

I think the first idea of continuing to choosing project over package is more appropriate. Especially because that matches current behavior.

I'll review the code later.

erdembayar · 2021-02-16T16:42:51Z

I think the first idea of continuing to choosing project over package is more appropriate. Especially because that matches current behavior.

I'll review the code later.

@nkolev92
Please review when you have time.

nkolev92 · 2021-02-16T18:30:35Z

@erdembayar Sorry, this slipped through my fingers last week. I've added it to my queue.

nkolev92 · 2021-02-19T02:05:20Z

@erdembayar

Reviewing your description, I think proposal 1 makes sense.

a. We should file an issue for that. Start it in our repo. The fix might actually end up being on project-system side, but it's NUGet scenario.
b. I think this is similar to a. Personally given that projects have always been preferred, I'd keep it as is for now, just focus on fixing the bug. Adding a warning, if any, can be a different task.
c. That's a very nice catch. However, if you add PackageVersion in the DoubleReferencedTarget project, the bug you are fixing doesn't exist and the behavior is the same. We should file an issue on the dotnet/sdk side for this scenario.

It sounds to me like this is what you implemented, so I'll review the code.

nkolev92 · 2021-02-19T02:25:17Z

src/NuGet.Core/NuGet.Commands/RestoreCommand/LockFileBuilder.cs

+                    var first = item.First();
+                    var second = item.Last();
+
+                    if (first.Type == second.Type)


Can this happen?

Not really, I tried to be more cautious, now removing.

src/NuGet.Core/NuGet.Commands/RestoreCommand/LockFileBuilder.cs

erdembayar

@nkolev92
Thank you for your review.
I addressed your PR comments, please check.

erdembayar · 2021-02-20T01:14:07Z

src/NuGet.Core/NuGet.Commands/RestoreCommand/LockFileBuilder.cs

+                    var first = item.First();
+                    var second = item.Last();
+
+                    if (first.Type == second.Type)


Not really, I tried to be more cautious, now removing.

src/NuGet.Core/NuGet.Commands/RestoreCommand/LockFileBuilder.cs

erdembayar · 2021-02-26T16:33:32Z

@nkolev92
Can you review again please?

ghost · 2021-06-13T16:00:23Z

This PR has been automatically marked as stale because it has no activity for 7 days. It will be closed if no further activity occurs within another 7 days of this comment. If it is closed, you may reopen it anytime when you're ready again, as long as you don't delete the branch.

ericsampson · 2021-06-20T18:39:30Z

uhh this probably shouldn't be auto closed, lol

erdembayar · 2021-06-29T04:00:47Z

@nkolev92
Here is dotnetbenchmark result. I don't see much difference in both run time duration and memory allocation for before and after my change. Actually it performs bit better on edge case we're interested in.

Code used for benchmarking is here.

Without any change.

[Host] : .NET 5.0.7 (5.0.721.25508), X64 RyuJIT
DefaultJob : .NET 5.0.7 (5.0.721.25508), X64 RyuJIT

Method	Mean	Error	StdDev	Rank	Gen 0	Gen 1	Gen 2	Allocated
Test0:Normal	488.6 us	3.69 us	3.45 us	1	41.9922	10.2539	-	259 KB

With my PR changes. Below DuplicateId is edge case which we're interested in.

.NET SDK=6.0.100-preview.5.21302.13
[Host] : .NET 5.0.7 (5.0.721.25508), X64 RyuJIT
DefaultJob : .NET 5.0.7 (5.0.721.25508), X64 RyuJIT

Method	Mean	Error	StdDev	Ratio	RatioSD	Rank	Gen 0	Gen 1	Gen 2	Allocated
Test0:DuplicateId	336.6 us	2.46 us	2.05 us	1.00	0.00	1	28.8086	8.3008	-	179 KB
Test0:Normal	487.1 us	4.45 us	4.16 us	1.45	0.02	2	41.9922	10.2539	-	259 KB

nkolev92 · 2021-06-29T20:27:19Z

src/NuGet.Core/NuGet.Commands/RestoreCommand/LockFileBuilder.cs

@@ -283,6 +285,79 @@ public class LockFileBuilder
            return lockFile;
        }

+        private void ValidateLockFileLibrary(IList<LockFileLibrary> libraries)
+        {
+            var libNameVersion = libraries.Select(l => l.Name + " " + l.Version);


Would it be at all simpler to just create a dictionary, and iterate over the libs one by one and updating the selected ref as we go?

Currently

Distinct uses a set internally - 1 pass: https://github.com/dotnet/runtime/blob/57bfe474518ab5b7cfe6bf7424a79ce3af9d6657/src/libraries/System.Linq/src/System/Linq/Distinct.cs.

The ToLookup statement creates a dict and then it iterates over that, allocating a list there.

Finally, iterate over the main list to remove the dups.

All of that can be replaced with 1 dictionary and 1 loop.
The moment you find a conflict, rank references and dedup.

Same applies to both methods.

@nkolev92
Could you be able to check now? I pushed new changes.

nkolev92

One small question.

src/NuGet.Core/NuGet.Commands/RestoreCommand/LockFileBuilder.cs

nkolev92

Great job!
👏

src/NuGet.Core/NuGet.Commands/RestoreCommand/LockFileBuilder.cs

erdembayar requested a review from a team as a code owner December 8, 2020 23:15

erdembayar marked this pull request as draft December 8, 2020 23:16

erdembayar mentioned this pull request Dec 8, 2020

When a ProjectReference and PackageReference in different TFMs have the same identity/version, Restore throws "An item with the same key has already been added. [...]" NuGet/Home#10368

Open

erdembayar added this to the Sprint 181 - 2020.12.21 milestone Jan 4, 2021

erdembayar modified the milestones: Sprint 181 - 2020.12.21, Sprint 182 - 2021.01.11 Jan 11, 2021

erdembayar force-pushed the dev-eryondon-samekey-already-added branch from f00e077 to 4ded836 Compare February 3, 2021 00:49

erdembayar marked this pull request as ready for review February 5, 2021 01:16

erdembayar force-pushed the dev-eryondon-samekey-already-added branch from 2d25ac1 to 6376f86 Compare February 5, 2021 01:26

erdembayar changed the title ~~Fix same key added exception when project graph has projectName == packageRef name (of same version)~~ Not fail when same name and same version both project and package are referenced in solution Feb 5, 2021

erdembayar changed the title ~~Not fail when same name and same version both project and package are referenced in solution~~ Handle when same name and same version both project and package are referenced in solution Feb 5, 2021

erdembayar marked this pull request as draft February 5, 2021 17:42

erdembayar marked this pull request as ready for review February 5, 2021 18:29

nkolev92 reviewed Feb 19, 2021

View reviewed changes

erdembayar commented Feb 20, 2021

View reviewed changes

erdembayar force-pushed the dev-eryondon-samekey-already-added branch 2 times, most recently from 7aca23e to c6b33b9 Compare March 23, 2021 16:53

erdembayar added 2 commits March 25, 2021 18:20

Remove not possible case handling.

69dd80c

Remove throwing exception part since it's possible scenario.

ff603ef

erdembayar force-pushed the dev-eryondon-samekey-already-added branch from c6b33b9 to ff603ef Compare March 26, 2021 01:23

Add early return condition from validation since it's hot path.

68b2f07

erdembayar force-pushed the dev-eryondon-samekey-already-added branch from a2e1dec to 68b2f07 Compare March 28, 2021 19:43

erdembayar modified the milestones: Sprint 182 - 2021.01.11, Sprint 184 - 2021.02.22 Mar 29, 2021

erdembayar marked this pull request as draft April 21, 2021 22:44

ghost added the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Jun 13, 2021

ghost closed this Jun 20, 2021

ghost removed the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Jun 20, 2021

erdembayar reopened this Jun 28, 2021

Merge branch 'dev' into dev-eryondon-samekey-already-added

5b82388

erdembayar marked this pull request as ready for review June 29, 2021 04:01

nkolev92 reviewed Jun 29, 2021

View reviewed changes

erdembayar added 3 commits June 30, 2021 08:49

Address PR comment by Nikolche.

bde0951

Clean up

d79699b

Remove typo.

f130d4b

nkolev92 reviewed Jun 30, 2021

View reviewed changes

src/NuGet.Core/NuGet.Commands/RestoreCommand/LockFileBuilder.cs Outdated Show resolved Hide resolved

Don't do new allocation if no change is required.

02d5bee

nkolev92 reviewed Jul 1, 2021

View reviewed changes

src/NuGet.Core/NuGet.Commands/RestoreCommand/LockFileBuilder.cs Outdated Show resolved Hide resolved

src/NuGet.Core/NuGet.Commands/RestoreCommand/LockFileBuilder.cs Outdated Show resolved Hide resolved

erdembayar added 2 commits July 2, 2021 09:05

Rename methods.

7709ff4

Create dictionary only once.

6975ab3

nkolev92 approved these changes Jul 6, 2021

View reviewed changes

src/NuGet.Core/NuGet.Commands/RestoreCommand/LockFileBuilder.cs Outdated Show resolved Hide resolved

Make name uniform.

a91b19a

erdembayar merged commit 65e9d80 into dev Jul 7, 2021

erdembayar deleted the dev-eryondon-samekey-already-added branch July 7, 2021 00:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle when same name and same version both project and package are referenced in solution #3789

Handle when same name and same version both project and package are referenced in solution #3789

erdembayar commented Dec 8, 2020 •

edited

Loading

ericsampson commented Dec 22, 2020

erdembayar commented Feb 4, 2021

nkolev92 commented Feb 4, 2021

erdembayar commented Feb 5, 2021

nkolev92 commented Feb 5, 2021

erdembayar commented Feb 5, 2021 •

edited

Loading

nkolev92 commented Feb 5, 2021

erdembayar commented Feb 5, 2021 •

edited

Loading

nkolev92 commented Feb 5, 2021

erdembayar commented Feb 16, 2021

nkolev92 commented Feb 16, 2021

nkolev92 commented Feb 19, 2021

nkolev92 Feb 19, 2021

erdembayar Feb 20, 2021 •

edited

Loading

erdembayar left a comment

erdembayar Feb 20, 2021 •

edited

Loading

erdembayar commented Feb 26, 2021

ghost commented Jun 13, 2021

ericsampson commented Jun 20, 2021

erdembayar commented Jun 29, 2021 •

edited

Loading

nkolev92 Jun 29, 2021

erdembayar Jun 30, 2021

nkolev92 left a comment

nkolev92 left a comment

Handle when same name and same version both project and package are referenced in solution #3789

Handle when same name and same version both project and package are referenced in solution #3789

Conversation

erdembayar commented Dec 8, 2020 • edited Loading

Bug

Fix

Testing/Validation

ericsampson commented Dec 22, 2020

erdembayar commented Feb 4, 2021

nkolev92 commented Feb 4, 2021

erdembayar commented Feb 5, 2021

nkolev92 commented Feb 5, 2021

erdembayar commented Feb 5, 2021 • edited Loading

nkolev92 commented Feb 5, 2021

erdembayar commented Feb 5, 2021 • edited Loading

nkolev92 commented Feb 5, 2021

erdembayar commented Feb 16, 2021

nkolev92 commented Feb 16, 2021

nkolev92 commented Feb 19, 2021

nkolev92 Feb 19, 2021

Choose a reason for hiding this comment

erdembayar Feb 20, 2021 • edited Loading

Choose a reason for hiding this comment

erdembayar left a comment

Choose a reason for hiding this comment

erdembayar Feb 20, 2021 • edited Loading

Choose a reason for hiding this comment

erdembayar commented Feb 26, 2021

ghost commented Jun 13, 2021

ericsampson commented Jun 20, 2021

erdembayar commented Jun 29, 2021 • edited Loading

nkolev92 Jun 29, 2021

Choose a reason for hiding this comment

erdembayar Jun 30, 2021

Choose a reason for hiding this comment

nkolev92 left a comment

Choose a reason for hiding this comment

nkolev92 left a comment

Choose a reason for hiding this comment

erdembayar commented Dec 8, 2020 •

edited

Loading

erdembayar commented Feb 5, 2021 •

edited

Loading

erdembayar commented Feb 5, 2021 •

edited

Loading

erdembayar Feb 20, 2021 •

edited

Loading

erdembayar Feb 20, 2021 •

edited

Loading

erdembayar commented Jun 29, 2021 •

edited

Loading