protoc: source code info for field options is inconsistent

[jhump]

Most field options look like any other options that have a "compact" representation (such as enum values and extension ranges):

• The options field (number 8) is associated with a span that represents the entire options section, including enclosing [ and ].
• Options therein are associated with a span that represents the option declaration, name = value. This is consistent with the longer options form (with options keyword, ending in ;), since they need to be able to include leading and trailing comments around the full declaration.

But the default and json_name pseudo-options do not behave this way.

• The default option is associated with a span that represents only the value and does not include the default = part of the declaration.
• The json_name option is associated with two spans: one that represents the entire declaration (like normal options), and also a second that represents only the value (similar to default).

Take this test file for example:

syntax = "proto2";

package foo.bar;

import "google/protobuf/descriptor.proto";

extend google.protobuf.FieldOptions {
  optional int64 abc = 10000;
}

message M {
  optional string name = 1 [
    default = "blahblah",
    json_name = "_NAME_",
    deprecated = false,
    (foo.bar.abc) = -99
  ];
}

It produces source code info for options and pseudo-options with locations like so:

    // entire options block, from "[" to "]"
    location {
      path: [4, 0, 2, 0, 8]
      span: [11, 27, 16, 3]
    }
    // the field's default_value, span includes only the value
    location {
      path: [4, 0, 2, 0, 7]
      span: [12, 14, 24]
    }
    // the field's json_name, span includes entire declaration
    location {
      path: [4, 0, 2, 0, 10]
      span: [13, 4, 24]
    }
    // the field's json_name (again), span includes only the value
    location {
      path: [4, 0, 2, 0, 10]
      span: [13, 16, 24]
    }
    // the deprecated option, span includes entire declaration
    location {
      path: [4, 0, 2, 0, 8, 3]
      span: [14, 4, 22]
    }
    // the (foo.bar.abc) custom option, span includes entire declaration
    location {
      path: [4, 0, 2, 0, 8, 10000]
      span: [15, 4, 23]
    }

But, for consistency, I assert it should instead produce locations that look like so:

    location {
      path: [4, 0, 2, 0, 8]
      span: [11, 27, 16, 3]
    }
    location {
      path: [4, 0, 2, 0, 7]
      span: [12, 4, 24]
    }
    location {
      path: [4, 0, 2, 0, 10]
      span: [13, 4, 24]
    }
    location {
      path: [4, 0, 2, 0, 8, 3]
      span: [14, 4, 22]
    }
    location {
      path: [4, 0, 2, 0, 8, 10000]
      span: [15, 4, 23]
    }

[jhump]

@mcy, if you agree with the proposed "correct and consistent" source code info above, you can see a fix that implements this in #10470.

[jhump]

@mcy, @mkruskal-google, can this be reopened since the fix was reverted in #10658?

[mkruskal-google]

Yes thanks for catching this Joshua!

[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.