C#: protoc should update its error and warning messages for the msvs format and include location information

[JunTaoLuo]

trafficstars

What version of protobuf and what language are you using? Version: The protoc included in Grpc.Tools 1.19.0 Language: C#

What operating system (Linux, Windows, ...) and version? All platforms

What runtime / compiler are you using (e.g., python version or gcc version) .NET Core 3.0 but this shouldn't matter

What did you do? Write a .proto file that contains the following errors:

1. duplicated names for message fields
2. unused imports
3. duplicated imports
4. enums with conflicting names I have been testing with the following:

syntax = "proto2";

import "google/protobuf/empty.proto";
import "google/protobuf/empty.proto";

package Greet;

// The greeting service definition.
service Greeter {
  // Sends a greeting
  rpc SayHello (HelloRequest) returns (HelloReply) {}
}

// The request message containing the user's name.
message HelloRequest {
  required string name = 1;
  required string name = 2;
  enum Enum {
    Zero = 0;
	ZERO = 1;
  }
}

// The response message containing the greetings.
message HelloReply {
  required string message = 1;
}

Observe the error and warning formats.

What did you expect to see Errors and warnings formatted in the current MSVS format. For example:

Startup.cs(12,17): error CS1001: Identifier expected

Note the format of file:(line:column) error/warning code: message

What did you see instead? Incorrect MSVS format. Some warnings and errors also do not include any location information. For example. Error with location:

../Protos/greet.proto(14) : error in column=10: "name" is already defined in "Greet.HelloRequest".
Error without location:
../Protos/greet.proto: Import "google/protobuf/empty.proto" was listed twice.
Warning without location:
../Protos/greet.proto: warning: Import google/protobuf/empty.proto but not used.
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#.

cc @haon4

[jtattermusch]

cc @jtattermusch

[kkm000]

/cc @kkm000 :)

[kkm000]

@JunTaoLuo,

1. How important it is for the codes to be distinc? Can I just add a single generic code for everything (PROTOC01), or is there a considerable benefit in using individual error codes per message?

2. Can you please suggest the code prefix, so we do not clash with another product? C# = CS, cl the C/C++ compiler = C, MsBuild = MSB, link = LNK, protoc = ??? I'd use a longer one, like PROTOC, as it looks like Microsoft seems to use short-ish codes, and there are only so many possible.

[JunTaoLuo]

Doesn't look like there is any authoritative docs on how error codes should be defined and it's left up to the author. That being said, there are a few rules of thumb that may be relevant here:

1. PROTOC is a perfect prefix, it's distinct and communicates clearly where the error is coming from. The length is fine, we have NETSDK prefix for errors too so the length is okay.
2. The error code should (but not must) be 4 digits long. Generally the thousands digit is used to categorize errors but this is mostly up to the author. You can look to MSB or CS errors for inspiration: https://docs.microsoft.com/en-us/previous-versions/visualstudio/visual-studio-2008/bb383807(v%3dvs.90) https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/compiler-messages/.

For example, we might want to reseve 0 - 999 for critical or future use and start errors at 1000. We might want to use 1000-4999 for errors and 5000+ for warnings. We might want to categorize argument errors for 1000-1999, syntax errors for 2000-2999, etc.

3. Yes the error codes should be distinct. This would allow better links to references on how to fix the issues for example.

[anandolee]

The error produced in descriptor.cc should also call AddError()/AddWarning() in CommandLineInterface: https://github.com/protocolbuffers/protobuf/blob/d0f91c863ae0fbb75b41460c8bbb786ade197a0f/src/google/protobuf/compiler/command_line_interface.cc#L289

[anandolee]

Update: A change has been submitted in internal which can add location error info for import errors (including the two examples above). Expect to see the fix in future release.

Still have some errors in descriptor.cc that do not have location info. I will try to look how many of them can add location info in.

[anandolee]

Update: location info are added for all errors in descriptor.cc. Will be included in future release.

[anandolee]

for MSVS format, feel free to create a PR and assign to us. Related code: https://github.com/protocolbuffers/protobuf/blob/2350e20cae1a9a1f54147c6e18e1a0da706cca96/src/google/protobuf/compiler/command_line_interface.cc#L346

[github-actions[bot]]

We triage inactive PRs and issues in order to make it easier to find active work. If this issue should remain active or becomes active again, please add a comment.

This issue is labeled inactive because the last activity was over 90 days ago.

[github-actions[bot]]

We triage inactive PRs and issues in order to make it easier to find active work. If this issue should remain active or becomes active again, please reopen it.

This issue was closed and archived because there has been no new activity in the 14 days since the inactive label was added.