From 1d0e506830c1cbf9f78d95f2a8fd256399e8fd0c Mon Sep 17 00:00:00 2001 From: Eljees <3.14hell@gmail.com> Date: Sat, 15 Aug 2026 10:16:27 +0000 Subject: [PATCH] Honour disable comments in FIELD_NUMBERS_ORDER_ASCENDING The rule walks message.MessageBody and enum.EnumBody itself instead of implementing VisitField and VisitEnumField, so extendedDisableRuleVisitor never gets to look at the comments attached to a single field and "// protolint:disable:this FIELD_NUMBERS_ORDER_ASCENDING" is silently ignored. The block form is ignored inside a message for the same reason, so the rule cannot currently be turned off for part of a message at all. Interpret the directives in the rule itself. A disabled field still takes part in the ordering chain and only the report about it is suppressed, so a field added after the disabled ones keeps being checked against them. Fixes #558 --- .../rules/fieldNumbersOrderAscendingRule.go | 87 ++++++---- ...dNumbersOrderAscendingRule_disable_test.go | 148 ++++++++++++++++++ 2 files changed, 207 insertions(+), 28 deletions(-) create mode 100644 internal/addon/rules/fieldNumbersOrderAscendingRule_disable_test.go diff --git a/internal/addon/rules/fieldNumbersOrderAscendingRule.go b/internal/addon/rules/fieldNumbersOrderAscendingRule.go index 76100ee..b417cbc 100644 --- a/internal/addon/rules/fieldNumbersOrderAscendingRule.go +++ b/internal/addon/rules/fieldNumbersOrderAscendingRule.go @@ -5,6 +5,7 @@ import ( "github.com/yoheimuta/go-protoparser/v4/parser" "github.com/yoheimuta/go-protoparser/v4/parser/meta" + "github.com/yoheimuta/protolint/linter/disablerule" "github.com/yoheimuta/protolint/linter/report" "github.com/yoheimuta/protolint/linter/rule" "github.com/yoheimuta/protolint/linter/visitor" @@ -41,12 +42,14 @@ func (r FieldNumbersOrderAscendingRule) IsOfficial() bool { func (r FieldNumbersOrderAscendingRule) Apply(proto *parser.Proto) ([]report.Failure, error) { v := &fieldNumbersOrderAscendingVisitor{ BaseAddVisitor: visitor.NewBaseAddVisitor(r.ID(), string(r.Severity())), + ruleID: r.ID(), } return visitor.RunVisitor(v, proto, r.ID()) } type fieldNumbersOrderAscendingVisitor struct { *visitor.BaseAddVisitor + ruleID string } // VisitMessage checks the message @@ -57,35 +60,48 @@ func (v *fieldNumbersOrderAscendingVisitor) VisitMessage(message *parser.Message hasError bool ) + // This rule walks the message body itself instead of relying on VisitField, so the + // disable comments attached to a single field never reach the visitor wrapper and have + // to be interpreted here. A disabled field keeps taking part in the ordering chain; + // only the report about it is suppressed. + // See https://github.com/yoheimuta/protolint/issues/558 + interpreter := disablerule.NewInterpreter(v.ruleID) + for _, element := range message.MessageBody { field, ok := element.(*parser.Field) if !ok { continue } + disabled := interpreter.Interpret(field.Comments, field.InlineComment) + number, err := strconv.Atoi(field.FieldNumber) if err != nil { - v.AddFailuref( - field.Meta.Pos, - "field number '%s' is not a number", - field.FieldNumber, - ) - - hasError = true + if !disabled { + v.AddFailuref( + field.Meta.Pos, + "field number '%s' is not a number", + field.FieldNumber, + ) + + hasError = true + } continue } if number <= 0 { - v.AddFailuref( - field.Meta.Pos, - "field number should be positive integer", - ) - - hasError = true + if !disabled { + v.AddFailuref( + field.Meta.Pos, + "field number should be positive integer", + ) + + hasError = true + } continue } - number, isError := v.isAscending(field.Meta.Pos, field.FieldName, number, lastName, lastNumber, false) + number, isError := v.isAscending(field.Meta.Pos, field.FieldName, number, lastName, lastNumber, false, disabled) if isError { hasError = true } @@ -118,35 +134,43 @@ func (v *fieldNumbersOrderAscendingVisitor) VisitEnum(enum *parser.Enum) bool { } } + interpreter := disablerule.NewInterpreter(v.ruleID) + for _, element := range enum.EnumBody { field, ok := element.(*parser.EnumField) if !ok { continue } + disabled := interpreter.Interpret(field.Comments, field.InlineComment) + number, err := strconv.Atoi(field.Number) if err != nil { - v.AddFailuref( - field.Meta.Pos, - "field number '%s' is not a number", - field.Number, - ) - - hasError = true + if !disabled { + v.AddFailuref( + field.Meta.Pos, + "field number '%s' is not a number", + field.Number, + ) + + hasError = true + } continue } if number < 0 { - v.AddFailuref( - field.Meta.Pos, - "field number should be positive integer", - ) - - hasError = true + if !disabled { + v.AddFailuref( + field.Meta.Pos, + "field number should be positive integer", + ) + + hasError = true + } continue } - number, isError := v.isAscending(field.Meta.Pos, field.Ident, number, lastIdent, lastNumber, allowAlias) + number, isError := v.isAscending(field.Meta.Pos, field.Ident, number, lastIdent, lastNumber, allowAlias, disabled) if isError { hasError = true } @@ -160,11 +184,15 @@ func (v *fieldNumbersOrderAscendingVisitor) VisitEnum(enum *parser.Enum) bool { func (v *fieldNumbersOrderAscendingVisitor) isAscending( pos meta.Position, fieldName string, number int, lastName string, lastNumber int, allowEqual bool, + disabled bool, ) (curNumber int, hasError bool) { if number == lastNumber { if allowEqual { return number, false } + if disabled { + return number, false + } v.AddFailuref( pos, "fields %s and %s have the same number %d", @@ -175,6 +203,9 @@ func (v *fieldNumbersOrderAscendingVisitor) isAscending( } if number < lastNumber { + if disabled { + return number, false + } v.AddFailuref( pos, "field %s should be after %s (ascending order expected)", diff --git a/internal/addon/rules/fieldNumbersOrderAscendingRule_disable_test.go b/internal/addon/rules/fieldNumbersOrderAscendingRule_disable_test.go new file mode 100644 index 0000000..5425167 --- /dev/null +++ b/internal/addon/rules/fieldNumbersOrderAscendingRule_disable_test.go @@ -0,0 +1,148 @@ +package rules_test + +import ( + "reflect" + "testing" + + "github.com/yoheimuta/go-protoparser/v4/parser" + "github.com/yoheimuta/go-protoparser/v4/parser/meta" + "github.com/yoheimuta/protolint/internal/addon/rules" + "github.com/yoheimuta/protolint/linter/report" + "github.com/yoheimuta/protolint/linter/rule" +) + +// TestFieldNumbersOrderAscendingRule_Apply_disableComments checks that the per-line +// disable comments are honoured by this rule. +// See https://github.com/yoheimuta/protolint/issues/558 +func TestFieldNumbersOrderAscendingRule_Apply_disableComments(t *testing.T) { + disableThis := func() *parser.Comment { + return &parser.Comment{Raw: "// protolint:disable:this FIELD_NUMBERS_ORDER_ASCENDING"} + } + + tests := []struct { + name string + inputProto *parser.Proto + wantFailures []report.Failure + }{ + { + name: "no failures for out-of-order fields disabled by an inline comment", + inputProto: &parser.Proto{ + ProtoBody: []parser.Visitee{ + &parser.Message{ + MessageBody: []parser.Visitee{ + &parser.Field{ + FieldName: "a", + FieldNumber: "2", + InlineComment: disableThis(), + }, + &parser.Field{ + FieldName: "b", + FieldNumber: "1", + InlineComment: disableThis(), + }, + }, + }, + }, + }, + }, + { + name: "no failures for out-of-order enum fields disabled by an inline comment", + inputProto: &parser.Proto{ + ProtoBody: []parser.Visitee{ + &parser.Enum{ + EnumBody: []parser.Visitee{ + &parser.EnumField{ + Ident: "A", + Number: "2", + InlineComment: disableThis(), + }, + &parser.EnumField{ + Ident: "B", + Number: "1", + InlineComment: disableThis(), + }, + }, + }, + }, + }, + }, + { + name: "a field following disabled ones is still checked against them", + inputProto: &parser.Proto{ + ProtoBody: []parser.Visitee{ + &parser.Message{ + MessageBody: []parser.Visitee{ + &parser.Field{ + FieldName: "a", + FieldNumber: "2", + InlineComment: disableThis(), + }, + &parser.Field{ + FieldName: "b", + FieldNumber: "1", + InlineComment: disableThis(), + }, + &parser.Field{ + FieldName: "c", + FieldNumber: "1", + }, + }, + }, + }, + }, + wantFailures: []report.Failure{ + report.Failuref( + meta.Position{}, + "FIELD_NUMBERS_ORDER_ASCENDING", + string(rule.SeverityError), + "fields %s and %s have the same number %d", + "b", "c", 1, + ), + }, + }, + { + name: "a field without the comment is still checked", + inputProto: &parser.Proto{ + ProtoBody: []parser.Visitee{ + &parser.Message{ + MessageBody: []parser.Visitee{ + &parser.Field{ + FieldName: "a", + FieldNumber: "2", + }, + &parser.Field{ + FieldName: "b", + FieldNumber: "1", + }, + }, + }, + }, + }, + wantFailures: []report.Failure{ + report.Failuref( + meta.Position{}, + "FIELD_NUMBERS_ORDER_ASCENDING", + string(rule.SeverityError), + "field %s should be after %s (ascending order expected)", + "a", "b", + ), + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + r := rules.NewFieldNumbersOrderAscendingRule(rule.SeverityError) + + got, err := r.Apply(test.inputProto) + if err != nil { + t.Errorf("got err %v, but want nil", err) + return + } + + if !reflect.DeepEqual(got, test.wantFailures) { + t.Errorf("got %v, but want %v", got, test.wantFailures) + } + }) + } +}