Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Return early if map type mismatch in buf lint protovalidate #2602

Conversation

oliversun9
Copy link
Contributor

This PR fixes an issue where buf lint crashes when map rules are defined on a non-map field:

  int32 not_a_map = 14 [
    (buf.validate.field).map.values.int32.lt = 10,
    (buf.validate.field).map.values.int32.gt = 1
  ];

The fix is to return early if type mismatches.

@rodaine
Copy link
Member

rodaine commented Nov 17, 2023

Are we returning a lint warning/error for this otherwise? (And do we need to handle repeated rules on singular fields as well?)

@@ -614,6 +614,7 @@ func TestRunProtovalidateRules(t *testing.T) {
bufanalysistesting.NewFileAnnotation(t, "map.proto", 44, 5, 44, 47, "PROTOVALIDATE"),
bufanalysistesting.NewFileAnnotation(t, "map.proto", 46, 5, 46, 47, "PROTOVALIDATE"),
bufanalysistesting.NewFileAnnotation(t, "map.proto", 50, 5, 50, 57, "PROTOVALIDATE"),
bufanalysistesting.NewFileAnnotation(t, "map.proto", 53, 5, 53, 50, "PROTOVALIDATE"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this produce two lint errors l, one for each of the two mismatched annotations?

Copy link
Contributor Author

@oliversun9 oliversun9 Nov 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The two lines belong to the same rule, (buf.validate.field).map.

When we pass the path (buf.validate.field).map to OptionExtensionLocation, the location we get back is just the first line. Ideally we get a location back that covers both lines.

I think it's due to the way sourceCodeLocations are generated by the compiler -- there isn't a location that spans both lines exactly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are also not able to tell whether the option is specified in two lines or in the aggregate form.

On a high level, the linter cannot tell

  int32 not_a_map = 14 [
    (buf.validate.field).map.values.int32.lt = 10,
    (buf.validate.field).map.values.int32.gt = 1
  ];

from

  int32 not_a_map = 14 [(buf.validate.field).map.values.int32 = {lt: 10, gt: 1}];

@oliversun9
Copy link
Contributor Author

oliversun9 commented Nov 17, 2023

Are we returning a lint warning/error for this otherwise?

Now we return a lint warning saying you have map rules for a non-map field. We don't further check whether a the map rule's key and value sub rules are valid. For example, it's impossible to check whether the map rule's key rule typechecks.

Copy link
Member

@bufdev bufdev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved assuming @rodaine comment addressed

@oliversun9 oliversun9 merged commit b1fc7dd into main Nov 20, 2023
7 checks passed
@oliversun9 oliversun9 deleted the osun/fix-lint-protovalidate-not-returning-early-for-map-mismatch branch November 20, 2023 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants