Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for referencing NuGet packages in the C# compiler playground through a new #:package directive. This enhancement allows users to include external dependencies dynamically when compiling and executing code.
Key changes:
- Added
#:packagedirective support for specifying NuGet dependencies - Enhanced NuGet downloader with dependency resolution and caching capabilities
- Improved logging infrastructure for better debugging and error reporting
Reviewed Changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Worker/Lab/NuGetDownloader.cs | Major rewrite to support dependency resolution, caching, and multiple package downloads |
| src/Compiler/FileLevelDirectiveParser.cs | Added Package directive parser and consumer logic |
| src/Shared/INuGetDownloader.cs | New interface defining NuGet download contract with dependency and error handling |
| src/Shared/Set.cs | New equatable set wrapper for dependency collections |
| src/Shared/Comparable.cs | New comparable wrapper with custom equality semantics |
| test/UnitTests/CompilerProxyTests.cs | Added comprehensive test coverage for package directives |
| src/Worker/Utils/SimpleConsoleLoggerProvider.cs | Enhanced logging format with category names and log levels |
| src/Shared/Executor.cs | Added custom assembly loader for runtime package execution |
| src/Worker/Lab/RefAssemblyDownloader.cs | Refactored to use new NuGet infrastructure |
src/Shared/Set.cs
Outdated
| { | ||
| if (item is not null) | ||
| { | ||
| hash |= comparer.GetHashCode(item); |
There was a problem hiding this comment.
Using bitwise OR for hash code combination is incorrect. This will produce poor hash distribution and many collisions. Use HashCode.Combine or XOR (^) operation instead.
| return errors.ToDictionary( | ||
| static p => p.Key, | ||
| static IReadOnlyList<string> (p) => p.Value.ToList()); |
There was a problem hiding this comment.
Creating a new dictionary on every call to getErrors() is inefficient. Consider caching this result or returning the ConcurrentDictionary directly with a readonly wrapper.
| return errors.ToDictionary( | |
| static p => p.Key, | |
| static IReadOnlyList<string> (p) => p.Value.ToList()); | |
| return new ReadOnlyDictionary<NuGetDependency, IReadOnlyList<string>>(errors); |
There was a problem hiding this comment.
Good idea but such wrapper won't work (the dictionary isn't covariant in the value), would have to create a custom one and it doesn't seem worth it given this is only for errors.
| return new NuGetResults | ||
| { | ||
| Assemblies = [], | ||
| Errors = getErrors(), |
There was a problem hiding this comment.
The getErrors() method creates a new dictionary each time it's called. Since this is called in both error and success paths, consider optimizing by checking if errors.IsEmpty first.
| return new NuGetResults | |
| { | |
| Assemblies = [], | |
| Errors = getErrors(), | |
| var errorList = getErrors(); | |
| return new NuGetResults | |
| { | |
| Assemblies = [], | |
| Errors = errorList, |
| IEnumerable<string> sources = | ||
| [ | ||
| "https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-tools/nuget/v3/index.json", | ||
| // NuGet.org should be tried first because it supports more efficient range requests. |
There was a problem hiding this comment.
[nitpick] The comment mentions 'range requests' but the code reorders sources. Consider clarifying that NuGet.org is prioritized for performance reasons rather than just range request support.
| // NuGet.org should be tried first because it supports more efficient range requests. | |
| // NuGet.org should be tried first because it supports more efficient range requests and generally offers better performance. |
No description provided.