Fixes (#119)

* Remove diagnostics and instead throw on invalid logs

* Fix analyzer host with binary log reader

* remove more TODOs

* fixes

src/Basic.CompilerLog.UnitTests/BinaryLogReaderTests.cs

@@ -109,4 +109,15 @@ public void CreateStream2(BasicAnalyzerKind basicAnalyzerKind, bool leaveOpen)
         Assert.False(state.IsDisposed);
         state.Dispose();
     }
+
+    [Theory]
+    [InlineData(BasicAnalyzerKind.InMemory)]
+    [InlineData(BasicAnalyzerKind.OnDisk)]
+    public void VerifyBasicAnalyzerKind(BasicAnalyzerKind basicAnalyzerKind)
+    {
+        using var reader = BinaryLogReader.Create(Fixture.Console.Value.BinaryLogPath!, basicAnalyzerKind);
+        var compilerCall = reader.ReadAllCompilerCalls().First();
+        var compilationData = reader.ReadCompilationData(compilerCall);
+        Assert.Equal(basicAnalyzerKind, compilationData.BasicAnalyzerHost.Kind);
+    }
 }

src/Basic.CompilerLog.UnitTests/BinaryLogUtilTests.cs

@@ -22,15 +22,39 @@ public sealed class BinaryLogUtilTests
 {
     [Theory]
     [InlineData("dotnet exec csc.dll a.cs", "csc.dll", "a.cs")]
-    [InlineData("dotnet not what we expect a.cs", null, "")]
+    [InlineData("dotnet.exe exec csc.dll a.cs", "csc.dll", "a.cs")]
+    [InlineData("dotnet-can-be-any-host-name exec csc.dll a.cs", "csc.dll", "a.cs")]
     [InlineData("csc.exe a.cs b.cs", "csc.exe", "a.cs b.cs")]
-    public void ParseCompilerAndArguments(string inputArgs, string? expectedCompilerFilePath, string expectedArgs)
+    public void ParseCompilerAndArgumentsCsc(string inputArgs, string? expectedCompilerFilePath, string expectedArgs)
     {
         var (actualCompilerFilePath, actualArgs) = BinaryLogUtil.ParseTaskForCompilerAndArguments(ToArray(inputArgs), "csc.exe", "csc.dll");
         Assert.Equal(ToArray(expectedArgs), actualArgs);
         Assert.Equal(expectedCompilerFilePath, actualCompilerFilePath);
         static string[] ToArray(string arg) => arg.Split(new char[]{' '}, StringSplitOptions.RemoveEmptyEntries);
     }
+
+    [Theory]
+    [InlineData("dotnet.exe exec vbc.dll a.cs", "vbc.dll", "a.cs")]
+    [InlineData("dotnet-can-be-any-host-name exec vbc.dll a.vb", "vbc.dll", "a.vb")]
+    [InlineData("vbc.exe a.cs b.cs", "vbc.exe", "a.cs b.cs")]
+    public void ParseCompilerAndArgumentsVbc(string inputArgs, string? expectedCompilerFilePath, string expectedArgs)
+    {
+        var (actualCompilerFilePath, actualArgs) = BinaryLogUtil.ParseTaskForCompilerAndArguments(ToArray(inputArgs), "vbc.exe", "vbc.dll");
+        Assert.Equal(ToArray(expectedArgs), actualArgs);
+        Assert.Equal(expectedCompilerFilePath, actualCompilerFilePath);
+        static string[] ToArray(string arg) => arg.Split(new char[]{' '}, StringSplitOptions.RemoveEmptyEntries);
+    }
+
+
+    [Theory]
+    [InlineData("dotnet not what we expect a.cs")]
+    [InlineData("dotnet csc2 what we expect a.cs")]
+    [InlineData("dotnet exec vbc.dll what we expect a.cs")]
+    public void ParseCompilerAndArgumentsBad(string inputArgs)
+    {
+        Assert.Throws<InvalidOperationException>(() => BinaryLogUtil.ParseTaskForCompilerAndArguments(ToArray(inputArgs), "csc.exe", "csc.dll"));
+        static string[] ToArray(string arg) => arg.Split(new char[]{' '}, StringSplitOptions.RemoveEmptyEntries);
+    }
 }
 
 public sealed class MSBuildProjectDataTests
@@ -55,9 +79,7 @@ public void TryCreateCompilerCallBadArguments()
             CommandLineArguments = "dotnet not a compiler call",
         };
 
-        var diagnostics = new List<string>();
-        Assert.Null(data.TryCreateCompilerCall(null, diagnostics));
-        Assert.NotEmpty(diagnostics);
+        Assert.Throws<InvalidOperationException>(() => data.TryCreateCompilerCall(ownerState: null));
     }
 
     [Fact]
@@ -68,10 +90,6 @@ public void TryCreateCompilerNoArguments()
             CommandLineArguments = null,
         };
 
-        var diagnostics = new List<string>();
-        Assert.Null(data.TryCreateCompilerCall(null, diagnostics));
-
-        // This is a normal non-compile case so no diagnostics are emitted
-        Assert.Empty(diagnostics);
+        Assert.Null(data.TryCreateCompilerCall(null));
     }
 }

src/Basic.CompilerLog.UnitTests/CompilerLogBuilderTests.cs

@@ -22,7 +22,7 @@ public void AddMissingFile()
         using var builder = new CompilerLogBuilder(stream, new());
         using var binlogStream = new FileStream(Fixture.ConsoleWithDiagnosticsBinaryLogPath, FileMode.Open, FileAccess.Read, FileShare.Read);
 
-        var compilerCall = BinaryLogUtil.ReadAllCompilerCalls(binlogStream, new()).First(x => x.IsCSharp);
+        var compilerCall = BinaryLogUtil.ReadAllCompilerCalls(binlogStream).First(x => x.IsCSharp);
         compilerCall = compilerCall.ChangeArguments(["/sourcelink:does-not-exist.txt"]);
         Assert.Throws<Exception>(() => builder.Add(compilerCall, BinaryLogUtil.ReadCommandLineArgumentsUnsafe(compilerCall)));
     }
@@ -34,7 +34,7 @@ public void AddWithMissingCompilerFilePath()
         using var builder = new CompilerLogBuilder(stream, new());
         using var binlogStream = new FileStream(Fixture.ConsoleWithDiagnosticsBinaryLogPath, FileMode.Open, FileAccess.Read, FileShare.Read);
 
-        var compilerCall = BinaryLogUtil.ReadAllCompilerCalls(binlogStream, new()).First(x => x.IsCSharp);
+        var compilerCall = BinaryLogUtil.ReadAllCompilerCalls(binlogStream).First(x => x.IsCSharp);
         var args = compilerCall.GetArguments();
         compilerCall = new CompilerCall(
             compilerFilePath: null,

src/Basic.CompilerLog.UnitTests/CompilerLogReaderExTests.cs

@@ -41,17 +41,14 @@ public CompilerLogReaderExTests(ITestOutputHelper testOutputHelper, SolutionFixt
     /// </summary>
     private CompilerLogReader ConvertConsole(Func<CompilerCall, CompilerCall> func, BasicAnalyzerKind? basicAnalyzerKind = null)
     {
-        var diagnostics = new List<string>();
-
         using var binlogStream = new FileStream(Fixture.SolutionBinaryLogPath, FileMode.Open, FileAccess.Read, FileShare.Read);
         var compilerCall = BinaryLogUtil.ReadAllCompilerCalls(
             binlogStream,
-            diagnostics,
             static x => x.ProjectFileName == "console.csproj").Single();
 
-        Assert.Empty(diagnostics);
         compilerCall = func(compilerCall);
-
+
+        var diagnostics = new List<string>();
         var stream = new MemoryStream();
         var builder = new CompilerLogBuilder(stream, diagnostics);
         builder.Add(compilerCall, BinaryLogUtil.ReadCommandLineArgumentsUnsafe(compilerCall));

src/Basic.CompilerLog.Util/BinaryLogReader.cs

@@ -86,18 +86,15 @@ public List<CompilerCall> ReadAllCompilerCalls(Func<CompilerCall, bool>? predica
     {
         predicate ??= static _ => true;
 
-        // TODO: need to remove this and consider just throwing exceptions here instead. Look inside
-        // compiler log to see what it does for exception during read and get some symetry with it
-        var diagnosticList = new List<string>();
-        return BinaryLogUtil.ReadAllCompilerCalls(_stream, diagnosticList, predicate, ownerState: this);
+        return BinaryLogUtil.ReadAllCompilerCalls(_stream, predicate, ownerState: this);
     }
 
     public List<CompilationData> ReadAllCompilationData(Func<CompilerCall, bool>? predicate = null)
     {
         var list = new List<CompilationData>();
         foreach (var compilerCall in ReadAllCompilerCalls(predicate))
         {
-            list.Add(Convert(compilerCall));
+            list.Add(ReadCompilationData(compilerCall));
         }
         return list;
     }
@@ -121,7 +118,7 @@ public CommandLineArguments ReadCommandLineArguments(CompilerCall compilerCall)
         return BinaryLogUtil.ReadCommandLineArgumentsUnsafe(compilerCall);
     }
 
-    public CompilationData Convert(CompilerCall compilerCall)
+    public CompilationData ReadCompilationData(CompilerCall compilerCall)
     {
         CheckOwnership(compilerCall);
         var args = ReadCommandLineArguments(compilerCall);
@@ -169,7 +166,6 @@ VisualBasicCompilationData GetVisualBasic()
                 PathNormalizationUtil.Empty);
         }
 
-        // TODO: this is tough. Existing hosts are way to tied to CompilerLogReader. Have to break that apart.
         BasicAnalyzerHost CreateAnalyzerHost()
         {
             var list = new List<RawAnalyzerData>(args.AnalyzerReferences.Length);
@@ -178,8 +174,17 @@ BasicAnalyzerHost CreateAnalyzerHost()
                 var data = new RawAnalyzerData(RoslynUtil.GetMvid(analyzer.FilePath), analyzer.FilePath);
                 list.Add(data);
             }
-
-            return new BasicAnalyzerHostOnDisk(this, list);
+
+            return LogReaderState.GetOrCreate(
+                BasicAnalyzerKind,
+                list,
+                (kind, analyzers) => kind switch
+                {
+                    BasicAnalyzerKind.None => throw new ArgumentException("Cannot create a host for None"),
+                    BasicAnalyzerKind.OnDisk => new BasicAnalyzerHostOnDisk(this, analyzers),
+                    BasicAnalyzerKind.InMemory => new BasicAnalyzerHostInMemory(this, analyzers),
+                    _ => throw new ArgumentOutOfRangeException(nameof(kind)),
+                });
         }
 
         List<(SourceText SourceText, string Path)> GetAnalyzerConfigs() => 
@@ -190,7 +195,6 @@ List<MetadataReference> GetReferences()
             var list = new List<MetadataReference>(capacity: args.MetadataReferences.Length);
             foreach (var reference in args.MetadataReferences)
             {
-                // TODO: should cache this
                 var mdref = MetadataReference.CreateFromFile(reference.Reference, reference.Properties);
                 list.Add(mdref);
             }
@@ -281,4 +285,7 @@ void IBasicAnalyzerHostDataProvider.CopyAssemblyBytes(RawAnalyzerData data, Stre
         using var fileStream = new FileStream(data.FilePath, FileMode.Open, FileAccess.Read, FileShare.Read);
         fileStream.CopyTo(stream);
     }
+
+    byte[] IBasicAnalyzerHostDataProvider.GetAssemblyBytes(RawAnalyzerData data) =>
+        File.ReadAllBytes(data.FilePath);
 }

src/Basic.CompilerLog.Util/BinaryLogUtil.cs

@@ -38,13 +38,13 @@ public CompilationTaskData GetOrCreateTaskData(int targetId)
             return data;
         }
 
-        public List<CompilerCall> GetAllCompilerCalls(object? ownerState, List<string> diagnostics)
+        public List<CompilerCall> GetAllCompilerCalls(object? ownerState)
         {
             var list = new List<CompilerCall>();
 
             foreach (var data in _targetMap.Values)
             {
-                if (data.TryCreateCompilerCall(ownerState, diagnostics) is { } compilerCall)
+                if (data.TryCreateCompilerCall(ownerState) is { } compilerCall)
                 {
                     if (compilerCall.Kind == CompilerCallKind.Regular)
                     {
@@ -83,7 +83,7 @@ public CompilationTaskData(MSBuildProjectData projectData, int targetId)
 
         public override string ToString() => $"{ProjectData} {TargetId}";
 
-        internal CompilerCall? TryCreateCompilerCall(object? ownerState, List<string> diagnosticList)
+        internal CompilerCall? TryCreateCompilerCall(object? ownerState)
         {
             if (CommandLineArguments is null)
             {
@@ -96,11 +96,6 @@ public CompilationTaskData(MSBuildProjectData projectData, int targetId)
             var (compilerFilePath, args) = IsCSharp
                 ? ParseTaskForCompilerAndArguments(rawArgs, "csc.exe", "csc.dll")
                 : ParseTaskForCompilerAndArguments(rawArgs, "vbc.exe", "vbc.dll");
-            if (args.Length == 0)
-            {
-                diagnosticList.Add($"Project {ProjectFile} ({TargetFramework}): bad argument list");
-                return null;
-            }
 
             return new CompilerCall(
                 compilerFilePath,
@@ -127,7 +122,7 @@ public MSBuildEvaluationData(string projectFile)
         public override string ToString() => $"{Path.GetFileName(ProjectFile)}({TargetFramework})";
     }
 
-    public static List<CompilerCall> ReadAllCompilerCalls(Stream stream, List<string> diagnosticList, Func<CompilerCall, bool>? predicate = null, object? ownerState = null)
+    public static List<CompilerCall> ReadAllCompilerCalls(Stream stream, Func<CompilerCall, bool>? predicate = null, object? ownerState = null)
     {
         // https://github.com/KirillOsenkov/MSBuildStructuredLog/issues/752
         Microsoft.Build.Logging.StructuredLogger.Strings.Initialize();
@@ -166,7 +161,7 @@ data.EvaluationId is { } evaluationId &&
                             data.TargetFramework = evaluationData.TargetFramework;
                         }
 
-                        foreach (var compilerCall in data.GetAllCompilerCalls(ownerState, diagnosticList))
+                        foreach (var compilerCall in data.GetAllCompilerCalls(ownerState))
                         {
                             if (predicate(compilerCall))
                             {
@@ -322,7 +317,8 @@ internal static (string? CompilerFilePath, string[] Arguments) ParseTaskForCompi
 
         if (!found)
         {
-            return (null, Array.Empty<string>());
+            var cmdLine = string.Join(" ", args);
+            throw new InvalidOperationException($"Could not parse command line arguments: {cmdLine}");
         }
 
         var list = new List<string>();

src/Basic.CompilerLog.Util/CompilerLogReader.cs

@@ -653,4 +653,5 @@ public void Dispose()
     }
 
     void IBasicAnalyzerHostDataProvider.CopyAssemblyBytes(RawAnalyzerData data, Stream stream) => CopyAssemblyBytes(data.Mvid, stream);
+    byte[] IBasicAnalyzerHostDataProvider.GetAssemblyBytes(RawAnalyzerData data) => GetAssemblyBytes(data.Mvid);
 }

src/Basic.CompilerLog.Util/CompilerLogUtil.cs

@@ -91,7 +91,7 @@ internal static ConvertBinaryLogResult TryConvertBinaryLog(Stream binaryLogStrea
         var diagnostics = new List<string>();
         var included = new List<CompilerCall>();
 
-        var list = BinaryLogUtil.ReadAllCompilerCalls(binaryLogStream, diagnostics, predicate);
+        var list = BinaryLogUtil.ReadAllCompilerCalls(binaryLogStream, predicate);
         using var builder = new CompilerLogBuilder(compilerLogStream, diagnostics, metadataVersion);
         var success = true;
         foreach (var compilerCall in list)

src/Basic.CompilerLog.Util/IBasicAnalyzerHostDataProvider.cs

@@ -5,4 +5,5 @@ internal interface IBasicAnalyzerHostDataProvider
 {
     public LogReaderState LogReaderState { get; }
     public void CopyAssemblyBytes(RawAnalyzerData data, Stream stream);
+    public byte[] GetAssemblyBytes(RawAnalyzerData data);
 }

src/Basic.CompilerLog.Util/ICompilerCallReader.cs

@@ -7,4 +7,5 @@ public interface ICompilerCallReader : IDisposable
     public bool OwnsLogReaderState { get; }
     public List<CompilerCall> ReadAllCompilerCalls(Func<CompilerCall, bool>? predicate = null);
     public List<CompilationData> ReadAllCompilationData(Func<CompilerCall, bool>? predicate = null);
+    public CompilationData ReadCompilationData(CompilerCall compilerCall);
 }

src/Basic.CompilerLog.Util/Impl/BasicAnalyzerHostInMemory.cs

@@ -20,11 +20,11 @@ internal sealed class BasicAnalyzerHostInMemory : BasicAnalyzerHost
     internal InMemoryLoader Loader { get; }
     protected override ImmutableArray<AnalyzerReference> AnalyzerReferencesCore => Loader.AnalyzerReferences;
 
-    internal BasicAnalyzerHostInMemory(CompilerLogReader reader, List<RawAnalyzerData> analyzers)
+    internal BasicAnalyzerHostInMemory(IBasicAnalyzerHostDataProvider provider, List<RawAnalyzerData> analyzers)
         :base(BasicAnalyzerKind.InMemory)
     {
         var name = $"{nameof(BasicAnalyzerHostInMemory)} - {Guid.NewGuid().ToString("N")}";
-        Loader = new InMemoryLoader(name, reader, analyzers, AddDiagnostic);
+        Loader = new InMemoryLoader(name, provider, analyzers, AddDiagnostic);
     }
 
     protected override void DisposeCore()
@@ -41,15 +41,15 @@ internal sealed class InMemoryLoader : AssemblyLoadContext
     internal ImmutableArray<AnalyzerReference> AnalyzerReferences { get; }
     internal AssemblyLoadContext CompilerLoadContext { get; }
 
-    internal InMemoryLoader(string name, CompilerLogReader reader, List<RawAnalyzerData> analyzers, Action<Diagnostic> onDiagnostic)
+    internal InMemoryLoader(string name, IBasicAnalyzerHostDataProvider provider, List<RawAnalyzerData> analyzers, Action<Diagnostic> onDiagnostic)
         :base(name, isCollectible: true)
     {
-        CompilerLoadContext = reader.LogReaderState.CompilerLoadContext;
+        CompilerLoadContext = provider.LogReaderState.CompilerLoadContext;
         var builder = ImmutableArray.CreateBuilder<AnalyzerReference>(analyzers.Count);
         foreach (var analyzer in analyzers)
         {
             var simpleName = Path.GetFileNameWithoutExtension(analyzer.FileName);
-            _map[simpleName] = reader.GetAssemblyBytes(analyzer.Mvid);
+            _map[simpleName] = provider.GetAssemblyBytes(analyzer);
             builder.Add(new BasicAnalyzerReference(new AssemblyName(simpleName), this, onDiagnostic));
         }
 
@@ -92,7 +92,7 @@ internal sealed class InMemoryLoader
 {
     internal ImmutableArray<AnalyzerReference> AnalyzerReferences { get; }
 
-    internal InMemoryLoader(string name, CompilerLogReader reader, List<RawAnalyzerData> analyzers, Action<Diagnostic> onDiagnostic)
+    internal InMemoryLoader(string name, IBasicAnalyzerHostDataProvider provider, List<RawAnalyzerData> analyzers, Action<Diagnostic> onDiagnostic)
     {
         throw new PlatformNotSupportedException();
     }

src/Basic.CompilerLog.Util/LogReaderState.cs

@@ -7,7 +7,6 @@
 
 namespace Basic.CompilerLog.Util;
 
-// TODO: update this
 /// <summary>
 /// The <see cref="CompilationData"/> have underlying state associated with them: 
 ///     - File system entries to hold crypto key files