Fix ParseVBErrorOrWarning (#47833)

* Fix ParseVBErrorOrWarning

* Update Vbc.cs

* internal -> private

* Added test to verify error reporting from the VB compiler.

Verifies the text used for Microsoft.Build.Tasks.CodeAnalysis.UnitTests.CheckErrorAndWarningParsing.

* Added test to verify that column numbers insert correctly.

ErrorLoggerEngine was added to capture the parse results as produced by Microsoft.Build.Shared.EventArgsFormatting.

* Needs a space between the end parenthesis and the colon.

* Update ErrorLoggerEngine.cs

* Update Vbc.cs

* Update VbcTests.cs

* Make internal and fix spanish test failure

* Address feedback

Co-authored-by: Adam Biser <adambiser@gmail.com>
Co-authored-by: Julien Couvreur <julien.couvreur@gmail.com>

src/Compilers/Core/MSBuildTask/Csc.cs

@@ -176,7 +176,7 @@ public string? Nullable
         // Same separators as those used by Process.OutputDataReceived to maintain consistency between csc and VBCSCompiler
         private static readonly string[] s_separators = { "\r\n", "\r", "\n" };
 
-        private protected override void LogCompilerOutput(string output, MessageImportance messageImportance)
+        internal override void LogCompilerOutput(string output, MessageImportance messageImportance)
         {
             var lines = output.Split(s_separators, StringSplitOptions.RemoveEmptyEntries);
             foreach (string line in lines)

src/Compilers/Core/MSBuildTask/ManagedCompiler.cs

@@ -691,7 +691,10 @@ private int HandleResponse(Guid requestId, BuildResponse? response, string pathT
         /// in the language specific manner. This often involves parsing the raw output and formatting it as 
         /// individual messages for MSBuild.
         /// </summary>
-        private protected abstract void LogCompilerOutput(string output, MessageImportance messageImportance);
+        /// <remarks>
+        /// Internal for testing only.
+        /// </remarks>
+        internal abstract void LogCompilerOutput(string output, MessageImportance messageImportance);
 
         /// <summary>
         /// Used to log a message that should go into both the compiler server log as well as the MSBuild logs

src/Compilers/Core/MSBuildTask/Vbc.cs

@@ -238,7 +238,7 @@ public string? PdbFile
 
         private static readonly string[] s_separator = { Environment.NewLine };
 
-        private protected override void LogCompilerOutput(string output, MessageImportance messageImportance)
+        internal override void LogCompilerOutput(string output, MessageImportance messageImportance)
         {
             var lines = output.Split(s_separator, StringSplitOptions.None);
             foreach (string line in lines)
@@ -648,12 +648,9 @@ protected override void LogEventsFromTextOutput(string singleLine, MessageImport
         /// Given a string, parses it to find out whether it's an error or warning and, if so,
         /// make sure it's validated properly.  
         /// </summary>
-        /// <comments>
-        /// INTERNAL FOR UNITTESTING ONLY
-        /// </comments>
         /// <param name="singleLine">The line to parse</param>
         /// <param name="messageImportance">The MessageImportance to use when reporting the error.</param>
-        internal void ParseVBErrorOrWarning(string singleLine, MessageImportance messageImportance)
+        private void ParseVBErrorOrWarning(string singleLine, MessageImportance messageImportance)
         {
             // if this string is empty then we haven't seen the first line of an error yet
             if (_vbErrorLines.Count > 0)
@@ -689,7 +686,7 @@ internal void ParseVBErrorOrWarning(string singleLine, MessageImportance message
                     string originalVBErrorString = originalVBError.Message;
 
                     int column = singleLine.IndexOf('~') + 1;
-                    int endParenthesisLocation = originalVBErrorString.IndexOf(')');
+                    int endParenthesisLocation = originalVBErrorString.IndexOf(") :", StringComparison.Ordinal);
 
                     // If for some reason the line does not contain any ~ then something went wrong
                     // so abort and return the original string.

src/Compilers/Core/MSBuildTaskTests/TestUtilities/ErrorLoggerEngine.cs

@@ -0,0 +1,69 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+
+using System;
+using System.Collections;
+using System.Reflection;
+using System.Text;
+using Microsoft.Build.Framework;
+
+namespace Microsoft.CodeAnalysis.BuildTasks.UnitTests
+{
+    /// <summary>
+    /// An engine to output event messages as MSBuild does to test Vbc.ParseVBErrorOrWarning.
+    /// </summary>
+    internal sealed class ErrorLoggingEngine : IBuildEngine
+    {
+        private StringBuilder _log = new StringBuilder();
+        public MessageImportance MinimumMessageImportance = MessageImportance.Low;
+        private readonly MethodInfo _formatErrorMethod;
+        private readonly MethodInfo _formatWarningMethod;
+
+        public ErrorLoggingEngine()
+        {
+            // Use the formatting from Microsoft.Build.Shared.EventArgsFormatting.
+            var assembly = Assembly.LoadFrom("Microsoft.Build.dll");
+            var formattingClass = assembly.GetType("Microsoft.Build.Shared.EventArgsFormatting") ?? throw new Exception("Could not find EventArgsFormatting type");
+            _formatErrorMethod = formattingClass.GetMethod("FormatEventMessage", BindingFlags.Static | BindingFlags.NonPublic, null, CallingConventions.Any,
+                new Type[] { typeof(BuildErrorEventArgs) }, null) ?? throw new Exception("Could not find FormatEventMessage(BuildErrorEventArgs).");
+            _formatWarningMethod = formattingClass.GetMethod("FormatEventMessage", BindingFlags.Static | BindingFlags.NonPublic, null, CallingConventions.Any,
+                new Type[] { typeof(BuildWarningEventArgs) }, null) ?? throw new Exception("Could not find FormatEventMessage(BuildWarningEventArgs).");
+        }
+
+        internal string Log
+        {
+            set { _log = new StringBuilder(value); }
+            get { return _log.ToString(); }
+        }
+
+        public void LogErrorEvent(BuildErrorEventArgs eventArgs)
+        {
+            _log.Append(_formatErrorMethod.Invoke(null, new object[] { eventArgs }));
+            _log.AppendLine();
+        }
+
+        public void LogWarningEvent(BuildWarningEventArgs eventArgs)
+        {
+            _log.Append(_formatWarningMethod.Invoke(null, new object[] { eventArgs }));
+            _log.AppendLine();
+        }
+
+        public void LogCustomEvent(CustomBuildEventArgs eventArgs)
+        {
+        }
+
+        public void LogMessageEvent(BuildMessageEventArgs eventArgs)
+        {
+        }
+
+        public string ProjectFileOfTaskNode => "";
+        public int ColumnNumberOfTaskNode => 0;
+        public int LineNumberOfTaskNode => 0;
+        public bool ContinueOnError => true;
+
+        public bool BuildProjectFile(string projectFileName, string[] targetNames, IDictionary globalProperties, IDictionary targetOutputs)
+            => throw new NotImplementedException();
+
+    }
+}

src/Compilers/Core/MSBuildTaskTests/VbcTests.cs

@@ -464,6 +464,37 @@ public void SkipAnalyzersFlag()
             Assert.Equal("/optionstrict:custom /out:test.exe test.vb", vbc.GenerateResponseFileContents());
         }
 
+        [Fact]
+        [WorkItem(47790, "https://github.com/dotnet/roslyn/issues/47790")]
+        public void CheckErrorAndWarningParsing()
+        {
+            var vbc = new Vbc();
+            var engine = new ErrorLoggingEngine();
+            vbc.BuildEngine = engine;
+            // Use reflection to set protected property UsedCommandLineTool to true so Vbc.LogEventsFromTextOutput will pass the messages along to ParseVBErrorOrWarning.
+            typeof(Vbc).GetProperty("UsedCommandLineTool", System.Reflection.BindingFlags.Instance | System.Reflection.BindingFlags.NonPublic)?.SetValue(vbc, true);
+            // Errors and warnings were generated by compiling the code used in the test at
+            // Microsoft.CodeAnalysis.VisualBasic.CommandLine.UnitTests.CommandLineTests.LogErrorsWithColumnNumbers()
+            vbc.LogCompilerOutput(@"
+C:\Test Path (123)\hellovb.vb(6) : warning BC40008: 'Public Property x As Integer' is obsolete.
+
+        x = 3.5
+        ~      
+C:\Test Path (123)\hellovb.vb(6) : error BC30512: Option Strict On disallows implicit conversions from 'Double' to 'Integer'.
+
+        x = 3.5
+            ~~~
+C:\Test Path (123)\hellovb.vb(7) : error BC30451: 'asdf' is not declared. It may be inaccessible due to its protection level.
+
+        asdf
+        ~~~~
+", Build.Framework.MessageImportance.High);
+            // Check that the column number is being added at the correct place.
+            Assert.Contains(@"C:\Test Path (123)\hellovb.vb(6,9): warning BC40008: 'Public Property x As Integer' is obsolete.", engine.Log);
+            Assert.Contains(@"C:\Test Path (123)\hellovb.vb(6,13): error BC30512: Option Strict On disallows implicit conversions from 'Double' to 'Integer'.", engine.Log);
+            Assert.Contains(@"C:\Test Path (123)\hellovb.vb(7,9): error BC30451: 'asdf' is not declared. It may be inaccessible due to its protection level.", engine.Log);
+        }
+
         [Fact]
         [WorkItem(52467, "https://github.com/dotnet/roslyn/issues/52467")]
         public void UnexpectedExceptionLogsMessage()

src/Compilers/VisualBasic/Test/CommandLine/CommandLineTests.vb

@@ -337,6 +337,38 @@ End Module
             Assert.Equal("", output.ToString().Trim())
         End Sub
 
+        <WorkItem(47790, "https://github.com/dotnet/roslyn/issues/47790")>
+        <ConditionalFact(GetType(IsEnglishLocal))>
+        Public Sub LogErrorsWithColumnNumbers()
+            ' Arguments with quoted rootnamespace and main type are unquoted when
+            ' the arguments are read in by the command line compiler.
+            Dim dir = Temp.CreateDirectory()
+            Dim file = dir.CreateDirectory("Test Path (123)").CreateFile("hellovb.vb").WriteAllText(
+"Option Strict On
+Module Module1
+    <System.Obsolete(Nothing, False)>
+    Property x As Integer = 3
+    Sub Main()
+        x = 3.5
+        asdf
+    End Sub
+End Module
+").Path
+
+            Dim output As New StringWriter()
+            Dim compiler As New MockVisualBasicCompiler(Nothing, _baseDirectory, {"/nologo", "/target:exe", "/main:""Module1""", file})
+
+            Dim exitCode = compiler.Run(output, Nothing)
+            Assert.Equal(1, exitCode)
+            ' The errors and warnings generated by compiling this code are used in the test at
+            ' Microsoft.Build.Tasks.CodeAnalysis.UnitTests.CheckErrorAndWarningParsing()
+            Assert.Contains("\Test Path (123)\hellovb.vb(6) : warning BC40008: 'Public Property x As Integer' is obsolete.", output.ToString())
+            Assert.Contains("\Test Path (123)\hellovb.vb(6) : error BC30512: Option Strict On disallows implicit conversions from 'Double' to 'Integer'.", output.ToString())
+            Assert.Contains("\Test Path (123)\hellovb.vb(7) : error BC30451: 'asdf' is not declared. It may be inaccessible due to its protection level.", output.ToString())
+
+            CleanupAllGeneratedFiles(file)
+        End Sub
+
         <Fact>
         Public Sub CreateCompilationWithKeyFile()
             Dim source = "