diff --git a/.claude/CLAUDE.md b/.claude/CLAUDE.md index 101b92dd..8f08d6b7 100644 --- a/.claude/CLAUDE.md +++ b/.claude/CLAUDE.md @@ -37,7 +37,7 @@ to builders without direct coupling. | `scan_context.go` | `ScanCtx` / `NewScanCtx` — loads Go packages via `golang.org/x/tools/go/packages` | | `index.go` | `TypeIndex` — node classification (meta/route/operation/model/parameters/response) | | `declaration.go` | `EntityDecl` — wraps a type/value declaration with its enclosing file/package | -| `enum_value.go` | `enumBasicLitValue` — converts a `const Foo Kind = "bar"` RHS into its runtime value (enum discovery) | +| `enum_value.go` | `enumLiteralValue` — converts a `const Foo Kind = "bar"` RHS into its runtime value (enum discovery), unwrapping signed numeric literals (`-1` is a unary minus applied to `1`) and reporting unsupported/unparseable forms so the caller skips them rather than emitting a nil member; `enumBasicLitValue` handles the unsigned case | | `provenance.go` | `Provenance` — ties a spec JSON pointer to the source position of the Go construct that produced it; emitted via `Options.OnProvenance` (cross-ref linker, source side) | | `classify/` | Classification predicates usable from both scanner and builders (e.g. `IsAllowedExtension`) | diff --git a/docs/doc-site/maintainers/annotations/swagger-enum.md b/docs/doc-site/maintainers/annotations/swagger-enum.md index 5a6b5861..20c2bd22 100644 --- a/docs/doc-site/maintainers/annotations/swagger-enum.md +++ b/docs/doc-site/maintainers/annotations/swagger-enum.md @@ -25,6 +25,13 @@ collects the type's `const` declarations. point at it via `$ref` — the general `swagger:model ⇒ definition + $ref` rule applied to enums. +Only `const` declarations with a literal right-hand side are collected: +string, integer and float literals, including signed ones (`-1`, `+1`). +Integers are read in whatever form Go accepts — decimal, `0x` hexadecimal, +`0b` binary, `0o` and legacy `0` octal, with optional `_` digit separators. +Constants with no explicit literal — most notably `iota`-derived ones — +are not collected, and neither is a value too large to represent. + If `swagger:enum` names a type for which no matching `const` values are found, the enum semantics are dropped and the type falls through to ordinary type resolution (typically a plain `$ref`, no `enum` array). diff --git a/fixtures/bugs/3412/api.go b/fixtures/bugs/3412/api.go new file mode 100644 index 00000000..cc9fd62b --- /dev/null +++ b/fixtures/bugs/3412/api.go @@ -0,0 +1,33 @@ +// SPDX-FileCopyrightText: Copyright 2015-2025 go-swagger maintainers +// SPDX-License-Identifier: Apache-2.0 + +// Package bug3412 reproduces go-swagger issue #3412 ("`swagger:enum` seems to +// ignore negative integer constants"): Go never scans a negative numeric +// literal — `-1` is a unary minus applied to `1` — so a signed const reached +// the enum collector as an *ast.UnaryExpr and was silently dropped, leaving +// [0] where [-1, 0, 1] was declared. +package bug3412 + +// PanDirection is the direction of a pan. +// +// swagger:enum PanDirection +type PanDirection int8 + +const ( + // PanLeft pans to the left. + PanLeft PanDirection = -1 + + // NoPan does not pan. + NoPan PanDirection = 0 + + // PanRight pans to the right. + PanRight PanDirection = +1 +) + +// ControlParams are PTZ control parameters. +// +// swagger:model ControlParams +type ControlParams struct { + // specifies the direction of the pan. + Pan PanDirection `json:"pan,omitempty"` +} diff --git a/fixtures/integration/golden/bugs_3412_schema.json b/fixtures/integration/golden/bugs_3412_schema.json new file mode 100644 index 00000000..46efad18 --- /dev/null +++ b/fixtures/integration/golden/bugs_3412_schema.json @@ -0,0 +1,25 @@ +{ + "swagger": "2.0", + "paths": {}, + "definitions": { + "ControlParams": { + "type": "object", + "title": "ControlParams are PTZ control parameters.", + "properties": { + "pan": { + "description": "specifies the direction of the pan.\n-1 PanLeft pans to the left.\n0 NoPan does not pan.\n1 PanRight pans to the right.", + "type": "integer", + "format": "int64", + "enum": [ + -1, + 0, + 1 + ], + "x-go-enum-desc": "-1 PanLeft pans to the left.\n0 NoPan does not pan.\n1 PanRight pans to the right.", + "x-go-name": "Pan" + } + }, + "x-go-package": "github.com/go-openapi/codescan/fixtures/bugs/3412" + } + } +} \ No newline at end of file diff --git a/internal/integration/coverage_bug_3412_test.go b/internal/integration/coverage_bug_3412_test.go new file mode 100644 index 00000000..1a3d5921 --- /dev/null +++ b/internal/integration/coverage_bug_3412_test.go @@ -0,0 +1,43 @@ +// SPDX-FileCopyrightText: Copyright 2015-2025 go-swagger maintainers +// SPDX-License-Identifier: Apache-2.0 + +package integration_test + +import ( + "testing" + + "github.com/go-openapi/codescan" + "github.com/go-openapi/codescan/internal/scantest" + "github.com/go-openapi/testify/v2/assert" + "github.com/go-openapi/testify/v2/require" +) + +// TestCoverage_Bug3412 locks the fix for go-swagger issue #3412 ("swagger:enum seems to ignore +// negative integer constants"): Go's scanner never produces a negative numeric literal — `-1` is a +// unary minus applied to the literal `1` — so `const PanLeft PanDirection = -1` reached the enum +// collector as an *ast.UnaryExpr and was dropped, yielding [0] instead of [-1, 0, 1]. +// +// The explicit `+1` spelling exercises the other sign operator and must render without a stray plus. +func TestCoverage_Bug3412(t *testing.T) { + doc, err := codescan.Run(&codescan.Options{ + Packages: []string{"./bugs/3412/..."}, + WorkDir: scantest.FixturesDir(), + ScanModels: true, + }) + require.NoError(t, err) + require.NotNil(t, doc) + + props := doc.Definitions["ControlParams"].Properties + pan, ok := props["pan"] + require.True(t, ok) + + assert.Equal(t, "integer", pan.Type[0]) + assert.Equal(t, []any{int64(-1), int64(0), int64(1)}, pan.Enum, + "a negative const must not be dropped from the enum") + assert.Equal(t, + "-1 PanLeft pans to the left.\n0 NoPan does not pan.\n1 PanRight pans to the right.", + pan.Extensions["x-go-enum-desc"], + "the const→value mapping must carry the sign too") + + scantest.CompareOrDumpJSON(t, doc, "bugs_3412_schema.json") +} diff --git a/internal/scanner/enum_value.go b/internal/scanner/enum_value.go index c8373a21..9c6ab8c6 100644 --- a/internal/scanner/enum_value.go +++ b/internal/scanner/enum_value.go @@ -5,28 +5,87 @@ package scanner import ( "go/ast" + "go/token" "strconv" "strings" ) +// enumLiteralValue converts the RHS expression of a `const Foo Kind = ` declaration into its +// runtime value, reporting whether the expression is a literal this scanner understands. +// +// Go's scanner never produces a negative numeric literal: `-1` is a unary minus APPLIED to the +// literal `1`, so it reaches the AST as *ast.UnaryExpr wrapping *ast.BasicLit (same for the +// explicit `+1` spelling). Signed numeric literals are therefore unwrapped here — otherwise a +// `const PanLeft PanDirection = -1` would be dropped from the enum (go-swagger#3412). +// +// Anything else (identifiers — including iota-derived constants — calls, arithmetic) is reported as +// unsupported, and the caller skips that const. So is a numeric literal whose text does not parse, +// so that a const is never emitted as a nil enum member. +func enumLiteralValue(expr ast.Expr) (any, bool) { + switch e := expr.(type) { + case *ast.BasicLit: + return enumBasicLitValue(e) + + case *ast.UnaryExpr: + if e.Op != token.SUB && e.Op != token.ADD { + return nil, false + } + + basicLit, ok := e.X.(*ast.BasicLit) + if !ok { + return nil, false + } + + // A sign is only meaningful on a number: `-'a'` is legal Go but not a meaningful enum entry. + if basicLit.Kind != token.INT && basicLit.Kind != token.FLOAT { + return nil, false + } + + sign := "" + if e.Op == token.SUB { + sign = "-" + } + + return enumSignedLitValue(basicLit, sign) + + default: + return nil, false + } +} + // enumBasicLitValue converts the RHS of a `const Foo Kind = "bar"` declaration into its runtime // value — int64 / float64 / unquoted string — for emission as an enum entry on the Swagger // schema the scanner is building. // -// Returns nil when the literal kind is INT or FLOAT but the textual value fails to parse (rare — -// Go's own parser would have caught it upstream, but the safety net is cheap). -func enumBasicLitValue(basicLit *ast.BasicLit) any { +// Reports false when the literal kind is INT or FLOAT but the textual value fails to parse, so that +// the caller skips the const rather than emitting a nil enum member. +func enumBasicLitValue(basicLit *ast.BasicLit) (any, bool) { + return enumSignedLitValue(basicLit, "") +} + +// enumSignedLitValue is enumBasicLitValue with the sign carried by an enclosing unary operator +// folded back into the literal text. +// +// The sign is prepended rather than applied after parsing so that the whole int64 range round-trips: +// `-9223372036854775808` parses, whereas parsing `9223372036854775808` alone would overflow. +// +// Integers are parsed with base 0 so that the literal is read exactly as Go's own scanner wrote it: +// that accepts the `0x` / `0b` / `0o` prefixes, the legacy `0` octal form, and `_` digit +// separators. Base 10 would reject every one of those and, before the sign was handled here, quietly +// turned `017` into 17 where Go means 15. +func enumSignedLitValue(basicLit *ast.BasicLit, sign string) (any, bool) { switch basicLit.Kind.String() { case "INT": - if result, err := strconv.ParseInt(basicLit.Value, 10, 64); err == nil { - return result + if result, err := strconv.ParseInt(sign+basicLit.Value, 0, 64); err == nil { + return result, true } case "FLOAT": - if result, err := strconv.ParseFloat(basicLit.Value, 64); err == nil { - return result + if result, err := strconv.ParseFloat(sign+basicLit.Value, 64); err == nil { + return result, true } default: - return strings.Trim(basicLit.Value, "\"") + return strings.Trim(basicLit.Value, "\""), true } - return nil + + return nil, false } diff --git a/internal/scanner/scan_context.go b/internal/scanner/scan_context.go index 0abeb542..16317435 100644 --- a/internal/scanner/scan_context.go +++ b/internal/scanner/scan_context.go @@ -932,6 +932,10 @@ func (s *ScanCtx) FindEnumValues(pkg *packages.Package, enumName string) (list [ // — each sharing the spec's doc comment. // The Go compiler guarantees len(Names) == len(Values) when Values is non-empty, so out-of-parity // specs are ignored defensively. +// +// Values are collected from literals only: a signed numeric literal is unwrapped, since Go models +// `-1` as a unary operator applied to `1` rather than as a negative literal. Any other RHS form +// (identifier — including iota-derived constants — call, arithmetic) is skipped. func (s *ScanCtx) findEnumValue(spec ast.Spec, enumName string) (values []any, descriptions []string, positions []token.Pos) { vs, ok := spec.(*ast.ValueSpec) if !ok { @@ -954,13 +958,11 @@ func (s *ScanCtx) findEnumValue(spec ast.Spec, enumName string) (values []any, d docSuffix := buildEnumDocSuffix(vs.Doc, vs.Names) for i, nameIdent := range vs.Names { - bl, ok := vs.Values[i].(*ast.BasicLit) + literalValue, ok := enumLiteralValue(vs.Values[i]) if !ok { continue } - literalValue := enumBasicLitValue(bl) - var desc strings.Builder fmt.Fprintf(&desc, "%v %s", literalValue, nameIdent.Name) desc.WriteString(docSuffix) diff --git a/internal/scanner/scan_context_test.go b/internal/scanner/scan_context_test.go index 9d1115ea..7da8f698 100644 --- a/internal/scanner/scan_context_test.go +++ b/internal/scanner/scan_context_test.go @@ -7,6 +7,7 @@ import ( "go/ast" "go/token" "go/types" + "math" "os" "path/filepath" "testing" @@ -700,6 +701,159 @@ func TestScanCtx_findEnumValue_EdgeCases(t *testing.T) { assert.EqualT(t, int64(7), intVal) assert.EqualT(t, "7 X", descs[0]) }) + + // go-swagger#3412: Go models `-1` as a unary minus applied to the literal `1`, so a signed + // const reaches this collector as *ast.UnaryExpr and used to be dropped from the enum. + t.Run("ValueSpec with negative integer literal keeps the sign", func(t *testing.T) { + spec := &ast.ValueSpec{ + Names: []*ast.Ident{ast.NewIdent("PanLeft")}, + Type: ast.NewIdent("Foo"), + Values: []ast.Expr{&ast.UnaryExpr{ + Op: token.SUB, + X: &ast.BasicLit{Kind: token.INT, Value: "1"}, + }}, + } + values, descs, _ := sctx.findEnumValue(spec, "Foo") + require.Len(t, values, 1) + intVal, ok := values[0].(int64) + require.True(t, ok, "a signed INT literal must stay an int64") + assert.EqualT(t, int64(-1), intVal) + assert.EqualT(t, "-1 PanLeft", descs[0]) + }) + + t.Run("ValueSpec with explicitly positive integer literal drops the plus", func(t *testing.T) { + spec := &ast.ValueSpec{ + Names: []*ast.Ident{ast.NewIdent("PanRight")}, + Type: ast.NewIdent("Foo"), + Values: []ast.Expr{&ast.UnaryExpr{ + Op: token.ADD, + X: &ast.BasicLit{Kind: token.INT, Value: "1"}, + }}, + } + values, descs, _ := sctx.findEnumValue(spec, "Foo") + require.Len(t, values, 1) + intVal, ok := values[0].(int64) + require.True(t, ok) + assert.EqualT(t, int64(1), intVal) + assert.EqualT(t, "1 PanRight", descs[0]) + }) + + t.Run("ValueSpec with negative float literal keeps the sign", func(t *testing.T) { + spec := &ast.ValueSpec{ + Names: []*ast.Ident{ast.NewIdent("Below")}, + Type: ast.NewIdent("Foo"), + Values: []ast.Expr{&ast.UnaryExpr{ + Op: token.SUB, + X: &ast.BasicLit{Kind: token.FLOAT, Value: "1.5"}, + }}, + } + values, _, _ := sctx.findEnumValue(spec, "Foo") + require.Len(t, values, 1) + floatVal, ok := values[0].(float64) + require.True(t, ok) + assert.EqualT(t, -1.5, floatVal) + }) + + t.Run("ValueSpec with most negative int64 round-trips", func(t *testing.T) { + // The sign must be folded into the literal text before parsing: 9223372036854775808 on its + // own overflows an int64. + spec := &ast.ValueSpec{ + Names: []*ast.Ident{ast.NewIdent("Min")}, + Type: ast.NewIdent("Foo"), + Values: []ast.Expr{&ast.UnaryExpr{ + Op: token.SUB, + X: &ast.BasicLit{Kind: token.INT, Value: "9223372036854775808"}, + }}, + } + values, _, _ := sctx.findEnumValue(spec, "Foo") + require.Len(t, values, 1) + intVal, ok := values[0].(int64) + require.True(t, ok) + assert.EqualT(t, int64(math.MinInt64), intVal) + }) + + t.Run("ValueSpec with non-sign unary operator skips that position", func(t *testing.T) { + spec := &ast.ValueSpec{ + Names: []*ast.Ident{ast.NewIdent("X")}, + Type: ast.NewIdent("Foo"), + Values: []ast.Expr{&ast.UnaryExpr{ + Op: token.XOR, + X: &ast.BasicLit{Kind: token.INT, Value: "1"}, + }}, + } + values, descs, _ := sctx.findEnumValue(spec, "Foo") + assert.Empty(t, values) + assert.Empty(t, descs) + }) + + t.Run("ValueSpec with signed non-numeric literal skips that position", func(t *testing.T) { + // `-'a'` is legal Go but not a meaningful enum entry. + spec := &ast.ValueSpec{ + Names: []*ast.Ident{ast.NewIdent("X")}, + Type: ast.NewIdent("Foo"), + Values: []ast.Expr{&ast.UnaryExpr{ + Op: token.SUB, + X: &ast.BasicLit{Kind: token.CHAR, Value: `'a'`}, + }}, + } + values, descs, _ := sctx.findEnumValue(spec, "Foo") + assert.Empty(t, values) + assert.Empty(t, descs) + }) + + // An integer literal is read exactly as Go's scanner wrote it, so every form Go accepts for a + // const carries its real value into the enum — signed or not. + t.Run("ValueSpec accepts every integer literal form Go writes", func(t *testing.T) { + for _, tc := range []struct { + name string + literal string + negative bool + expected int64 + }{ + {name: "hexadecimal", literal: "0x10", expected: 16}, + {name: "negative hexadecimal", literal: "0x10", negative: true, expected: -16}, + {name: "binary", literal: "0b1010", expected: 10}, + {name: "negative binary", literal: "0b1010", negative: true, expected: -10}, + {name: "octal", literal: "0o17", expected: 15}, + {name: "negative octal", literal: "0o17", negative: true, expected: -15}, + // Go reads a leading zero as octal, so `017` is 15 — not 17. + {name: "legacy octal", literal: "017", expected: 15}, + {name: "negative legacy octal", literal: "017", negative: true, expected: -15}, + {name: "digit separators", literal: "1_000", expected: 1000}, + {name: "negative digit separators", literal: "1_000", negative: true, expected: -1000}, + } { + t.Run(tc.name, func(t *testing.T) { + var value ast.Expr = &ast.BasicLit{Kind: token.INT, Value: tc.literal} + if tc.negative { + value = &ast.UnaryExpr{Op: token.SUB, X: value} + } + spec := &ast.ValueSpec{ + Names: []*ast.Ident{ast.NewIdent("X")}, + Type: ast.NewIdent("Foo"), + Values: []ast.Expr{value}, + } + values, _, _ := sctx.findEnumValue(spec, "Foo") + require.Len(t, values, 1) + intVal, ok := values[0].(int64) + require.True(t, ok, "an INT literal must stay an int64") + assert.EqualT(t, tc.expected, intVal) + }) + } + }) + + // A const whose value cannot be represented is skipped outright: emitting it as a nil enum + // member would put a null in the spec, and go-openapi/spec reflects on the first member. + t.Run("ValueSpec with unrepresentable integer literal skips that position", func(t *testing.T) { + spec := &ast.ValueSpec{ + Names: []*ast.Ident{ast.NewIdent("TooBig")}, + Type: ast.NewIdent("Foo"), + // One past math.MaxInt64. + Values: []ast.Expr{&ast.BasicLit{Kind: token.INT, Value: "9223372036854775808"}}, + } + values, descs, _ := sctx.findEnumValue(spec, "Foo") + assert.Empty(t, values) + assert.Empty(t, descs) + }) } func TestSliceToSet(t *testing.T) {