Merge pull request #18446 from JunTaoLuo/error-list

Ensure errors link to correct line numbers through the VS Error List

src/csharp/Grpc.Tools.Tests/ProtoCompileBasicTest.cs

@@ -16,6 +16,8 @@
 
 #endregion
 
+using System.Collections.Generic;
+using System.Linq;
 using System.Reflection;  // UWYU: Object.GetType() extension.
 using Microsoft.Build.Framework;
 using Moq;
@@ -30,6 +32,7 @@ namespace Grpc.Tools.Tests
         {
             public string LastPathToTool { get; private set; }
             public string[] LastResponseFile { get; private set; }
+            public List<string> StdErrMessages { get; } = new List<string>();
 
             protected override int ExecuteTool(string pathToTool,
                                                string response,
@@ -45,8 +48,13 @@ namespace Grpc.Tools.Tests
                 LastPathToTool = pathToTool;
                 LastResponseFile = response.Remove(response.Length - 1).Split('\n');
 
+                foreach (string message in StdErrMessages)
+                {
+                    LogEventsFromTextOutput(message, MessageImportance.High);
+                }
+
                 // Do not run the tool, but pretend it ran successfully.
-                return 0;
+                return StdErrMessages.Any() ? -1 : 0;
             }
         };
 

src/csharp/Grpc.Tools.Tests/ProtoCompileCommandLineGeneratorTest.cs

@@ -175,5 +175,86 @@ namespace Grpc.Tools.Tests
             Assert.That(_task.LastResponseFile,
                         Does.Contain("--csharp_out=" + expect));
         }
+
+        [TestCase(
+            "../Protos/greet.proto(19) : warning in column=5 : warning : When enum name is stripped and label is PascalCased (Zero) this value label conflicts with Zero.",
+            "../Protos/greet.proto",
+            19,
+            5,
+            "warning : When enum name is stripped and label is PascalCased (Zero) this value label conflicts with Zero.")]
+        [TestCase(
+            "../Protos/greet.proto: warning: Import google/protobuf/empty.proto but not used.",
+            "../Protos/greet.proto",
+            0,
+            0,
+            "Import google/protobuf/empty.proto but not used.")]
+        [TestCase("../Protos/greet.proto(14) : error in column=10: \"name\" is already defined in \"Greet.HelloRequest\".", null, 0, 0, null)]
+        [TestCase("../Protos/greet.proto: Import \"google / protobuf / empty.proto\" was listed twice.", null, 0, 0, null)]
+        public void WarningsParsed(string stderr, string file, int line, int col, string message)
+        {
+            _task.StdErrMessages.Add(stderr);
+
+            _mockEngine
+                .Setup(me => me.LogWarningEvent(It.IsAny<BuildWarningEventArgs>()))
+                .Callback((BuildWarningEventArgs e) => {
+                    if (file != null)
+                    {
+                        Assert.AreEqual(file, e.File);
+                        Assert.AreEqual(line, e.LineNumber);
+                        Assert.AreEqual(col, e.ColumnNumber);
+                        Assert.AreEqual(message, e.Message);
+                    }
+                    else
+                    {
+                        Assert.Fail($"Error logged by build engine:\n{e.Message}");
+                    }
+                });
+
+            bool result = _task.Execute();
+            Assert.IsFalse(result);
+        }
+
+        [TestCase(
+            "../Protos/greet.proto(14) : error in column=10: \"name\" is already defined in \"Greet.HelloRequest\".",
+            "../Protos/greet.proto",
+            14,
+            10,
+            "\"name\" is already defined in \"Greet.HelloRequest\".")]
+        [TestCase(
+            "../Protos/greet.proto: Import \"google / protobuf / empty.proto\" was listed twice.",
+            "../Protos/greet.proto",
+            0,
+            0,
+            "Import \"google / protobuf / empty.proto\" was listed twice.")]
+        [TestCase("../Protos/greet.proto(19) : warning in column=5 : warning : When enum name is stripped and label is PascalCased (Zero) this value label conflicts with Zero.", null, 0, 0, null)]
+        [TestCase("../Protos/greet.proto: warning: Import google/protobuf/empty.proto but not used.", null, 0, 0, null)]
+        public void ErrorsParsed(string stderr, string file, int line, int col, string message)
+        {
+            _task.StdErrMessages.Add(stderr);
+
+            _mockEngine
+                .Setup(me => me.LogErrorEvent(It.IsAny<BuildErrorEventArgs>()))
+                .Callback((BuildErrorEventArgs e) => {
+                    if (file != null)
+                    {
+                        Assert.AreEqual(file, e.File);
+                        Assert.AreEqual(line, e.LineNumber);
+                        Assert.AreEqual(col, e.ColumnNumber);
+                        Assert.AreEqual(message, e.Message);
+                    }
+                    else
+                    {
+                        // Ignore expected error
+                        // "protoc/protoc.exe" existed with code -1.
+                        if (!e.Message.EndsWith("exited with code -1."))
+                        {
+                            Assert.Fail($"Error logged by build engine:\n{e.Message}");
+                        }
+                    }
+                });
+
+            bool result = _task.Execute();
+            Assert.IsFalse(result);
+        }
     };
 }

src/csharp/Grpc.Tools/ProtoCompile.cs

@@ -16,7 +16,10 @@
 
 #endregion
 
+using System;
+using System.Collections.Generic;
 using System.Text;
+using System.Text.RegularExpressions;
 using Microsoft.Build.Framework;
 using Microsoft.Build.Utilities;
 
@@ -123,6 +126,110 @@ namespace Grpc.Tools
                                                         "javanano", "js", "objc",
                                                         "php", "python", "ruby" };
 
+        static readonly TimeSpan s_regexTimeout = TimeSpan.FromMilliseconds(100);
+
+        static readonly List<ErrorListFilter> s_errorListFilters = new List<ErrorListFilter>()
+        {
+            // Example warning with location
+            //../Protos/greet.proto(19) : warning in column=5 : warning : When enum name is stripped and label is PascalCased (Zero),
+            // this value label conflicts with Zero. This will make the proto fail to compile for some languages, such as C#.
+            new ErrorListFilter
+            {
+                Pattern = new Regex(
+                    pattern: "(?'FILENAME'.+)\\((?'LINE'\\d+)\\) ?: ?warning in column=(?'COLUMN'\\d+) ?: ?(?'TEXT'.*)",
+                    options: RegexOptions.Compiled | RegexOptions.IgnoreCase,
+                    matchTimeout: s_regexTimeout),
+                LogAction = (log, match) =>
+                {
+                    int.TryParse(match.Groups["LINE"].Value, out var line);
+                    int.TryParse(match.Groups["COLUMN"].Value, out var column);
+
+                    log.LogWarning(
+                        subcategory: null,
+                        warningCode: null,
+                        helpKeyword: null,
+                        file: match.Groups["FILENAME"].Value,
+                        lineNumber: line,
+                        columnNumber: column,
+                        endLineNumber: 0,
+                        endColumnNumber: 0,
+                        message: match.Groups["TEXT"].Value);
+                }
+            },
+
+            // Example error with location
+            //../Protos/greet.proto(14) : error in column=10: "name" is already defined in "Greet.HelloRequest".
+            new ErrorListFilter
+            {
+                Pattern = new Regex(
+                    pattern: "(?'FILENAME'.+)\\((?'LINE'\\d+)\\) ?: ?error in column=(?'COLUMN'\\d+) ?: ?(?'TEXT'.*)",
+                    options: RegexOptions.Compiled | RegexOptions.IgnoreCase,
+                    matchTimeout: s_regexTimeout),
+                LogAction = (log, match) =>
+                {
+                    int.TryParse(match.Groups["LINE"].Value, out var line);
+                    int.TryParse(match.Groups["COLUMN"].Value, out var column);
+
+                    log.LogError(
+                        subcategory: null,
+                        errorCode: null,
+                        helpKeyword: null,
+                        file: match.Groups["FILENAME"].Value,
+                        lineNumber: line,
+                        columnNumber: column,
+                        endLineNumber: 0,
+                        endColumnNumber: 0,
+                        message: match.Groups["TEXT"].Value);
+                }
+            },
+
+            // Example warning without location
+            //../Protos/greet.proto: warning: Import google/protobuf/empty.proto but not used.
+            new ErrorListFilter 
+            {
+                Pattern = new Regex(
+                    pattern: "(?'FILENAME'.+): ?warning: ?(?'TEXT'.*)",
+                    options: RegexOptions.Compiled | RegexOptions.IgnoreCase,
+                    matchTimeout: s_regexTimeout),
+                LogAction = (log, match) =>
+                {
+                    log.LogWarning(
+                        subcategory: null,
+                        warningCode: null,
+                        helpKeyword: null,
+                        file: match.Groups["FILENAME"].Value,
+                        lineNumber: 0,
+                        columnNumber: 0,
+                        endLineNumber: 0,
+                        endColumnNumber: 0,
+                        message: match.Groups["TEXT"].Value);
+                }
+            },
+
+            // Example error without location
+            //../Protos/greet.proto: Import "google/protobuf/empty.proto" was listed twice.
+            new ErrorListFilter
+            {
+                Pattern = new Regex(
+                    pattern: "(?'FILENAME'.+): ?(?'TEXT'.*)",
+                    options: RegexOptions.Compiled | RegexOptions.IgnoreCase,
+                    matchTimeout: s_regexTimeout),
+                LogAction = (log, match) =>
+                {
+                    log.LogError(
+                        subcategory: null,
+                        errorCode: null,
+                        helpKeyword: null,
+                        file: match.Groups["FILENAME"].Value,
+                        lineNumber: 0,
+                        columnNumber: 0,
+                        endLineNumber: 0,
+                        endColumnNumber: 0,
+                        message: match.Groups["TEXT"].Value);
+                }
+            }
+        };
+
         /// <summary>
         /// Code generator.
         /// </summary>
@@ -406,6 +513,22 @@ namespace Grpc.Tools
             base.LogToolCommand(printer.ToString());
         }
 
+        protected override void LogEventsFromTextOutput(string singleLine, MessageImportance messageImportance)
+        {
+            foreach (ErrorListFilter filter in s_errorListFilters)
+            {
+                Match match = filter.Pattern.Match(singleLine);
+                
+                if (match.Success)
+                {
+                    filter.LogAction(Log, match);
+                    return;
+                }
+            }
+
+            base.LogEventsFromTextOutput(singleLine, messageImportance);
+        }
+
         // Main task entry point.
         public override bool Execute()
         {
@@ -438,5 +561,11 @@ namespace Grpc.Tools
 
             return true;
         }
+
+        class ErrorListFilter
+        {
+            public Regex Pattern { get; set; }
+            public Action<TaskLoggingHelper, Match> LogAction { get; set; }
+        }
     };
 }

src/csharp/Grpc.Tools/build/_protobuf/Google.Protobuf.Tools.targets

@@ -271,7 +271,6 @@
       GrpcPluginExe="%(_Protobuf_OutOfDateProto.GrpcPluginExe)"
       GrpcOutputDir="%(_Protobuf_OutOfDateProto.GrpcOutputDir)"
       GrpcOutputOptions="%(_Protobuf_OutOfDateProto._GrpcOutputOptions)"
-      LogStandardErrorAsError="true"
     >
       <Output TaskParameter="GeneratedFiles" ItemName="_Protobuf_GeneratedFiles"/>
     </ProtoCompile>