diff --git a/changes/unreleased/undefined-operator-parse-record.performance.md b/changes/unreleased/undefined-operator-parse-record.performance.md new file mode 100644 index 000000000..ef93698d0 --- /dev/null +++ b/changes/unreleased/undefined-operator-parse-record.performance.md @@ -0,0 +1 @@ +- The `~` undefined-operator warning now reads the operator sites the parser records instead of walking every node of every document, removing about 8% from load and validation time. diff --git a/internal/check/passes/undefined_operator.go b/internal/check/passes/undefined_operator.go index 9f04d4aa4..d43a5c992 100644 --- a/internal/check/passes/undefined_operator.go +++ b/internal/check/passes/undefined_operator.go @@ -19,25 +19,22 @@ type UndefinedOperatorPass struct{} // later-tier failure never hides the warning. func (UndefinedOperatorPass) Level() PassLevel { return LevelSyntax } -// Run walks the parsed tree and warns at each `~` operator expression. It stays -// a warning in every conformance mode: the specification asks for a warning, -// not a rejection. +// Run warns at each `~` operator expression the parser recorded on the root. +// It stays a warning in every conformance mode: the specification asks for a +// warning, not a rejection. func (UndefinedOperatorPass) Run(ctx *Context, name string, root *ast.RootNamespace) []diag.Diagnostic { if root == nil { return nil } - var diags []diag.Diagnostic - ast.Inspect(root, func(n ast.Node) bool { - if e, ok := n.(*ast.OperatorExpr); ok && e.Operator == ast.OpBitNot { - diags = append(diags, diag.Diagnostic{ - Severity: diag.SeverityWarning, - Span: e.Span(), - Message: msgUndefinedOperator, - Code: codeUndefinedOperator, - Source: "syntax", - }) - } - return true - }) + diags := make([]diag.Diagnostic, 0, len(root.UndefinedOperators)) + for _, e := range root.UndefinedOperators { + diags = append(diags, diag.Diagnostic{ + Severity: diag.SeverityWarning, + Span: e.Span(), + Message: msgUndefinedOperator, + Code: codeUndefinedOperator, + Source: "syntax", + }) + } return diags } diff --git a/internal/syntax/ast/astcodec/codec.go b/internal/syntax/ast/astcodec/codec.go index 0b6a31c76..820c06dc8 100644 --- a/internal/syntax/ast/astcodec/codec.go +++ b/internal/syntax/ast/astcodec/codec.go @@ -166,6 +166,13 @@ func (e *Encoder) ends(ns []*ast.ConnectorEnd) { } } +func (e *Encoder) operatorExprs(ns []*ast.OperatorExpr) { + e.w.Len(len(ns)) + for _, n := range ns { + e.node(n) + } +} + func (e *Encoder) segments(segs []ast.NameSegment) { e.w.Len(len(segs)) for _, s := range segs { @@ -213,6 +220,7 @@ type Decoder struct { nameSlices pack.Arena[*ast.QualifiedName] regSlices pack.Arena[*ast.StateRegion] endSlices pack.Arena[*ast.ConnectorEnd] + opSlices pack.Arena[*ast.OperatorExpr] segArena pack.Arena[ast.NameSegment] argArena pack.Arena[ast.NamedArg] paramArena pack.Arena[ast.BodyParam] @@ -390,6 +398,14 @@ func (d *Decoder) ends() []*ast.ConnectorEnd { return out } +func (d *Decoder) operatorExprs() []*ast.OperatorExpr { + out := d.opSlices.Take(d.r.Len()) + for i := range out { + out[i] = typed[*ast.OperatorExpr](d) + } + return out +} + func (d *Decoder) segments() []ast.NameSegment { return d.segmentsN(d.r.Len()) } diff --git a/internal/syntax/ast/astcodec/nodes.go b/internal/syntax/ast/astcodec/nodes.go index 60303f43a..166d09e98 100644 --- a/internal/syntax/ast/astcodec/nodes.go +++ b/internal/syntax/ast/astcodec/nodes.go @@ -1001,6 +1001,7 @@ func (e *Encoder) encodeFields(node ast.Node) { case *ast.RootNamespace: e.base(&n.NodeBase) e.nodes(n.Members) + e.operatorExprs(n.UndefinedOperators) case *ast.SelectExpr: e.base(&n.NodeBase) e.node(n.Operand) @@ -1504,6 +1505,7 @@ func (d *Decoder) decodeFields(node ast.Node) { case *ast.RootNamespace: d.base(&n.NodeBase) n.Members = d.nodes() + n.UndefinedOperators = d.operatorExprs() case *ast.SelectExpr: d.base(&n.NodeBase) n.Operand = d.node() diff --git a/internal/syntax/ast/namespace.go b/internal/syntax/ast/namespace.go index 6c3fd8133..ed5cb6e16 100644 --- a/internal/syntax/ast/namespace.go +++ b/internal/syntax/ast/namespace.go @@ -249,6 +249,9 @@ type Membership struct { type RootNamespace struct { NodeBase Members []Node // *Membership | *Import | *Alias | *ErrorNode + // UndefinedOperators are the `~` operator expressions of the document in + // source order: KerML leaves DataFunctions::'~' undefined, so a tool warns at each. + UndefinedOperators []*OperatorExpr } // PrefixMetadata records a `# QualifiedName` metadata annotation reference. diff --git a/internal/syntax/parser/expr.go b/internal/syntax/parser/expr.go index cc789a6d3..63eabeb62 100644 --- a/internal/syntax/parser/expr.go +++ b/internal/syntax/parser/expr.go @@ -185,9 +185,18 @@ func (p *Parser) parseUnary() ast.Node { return p.parsePrimary() } p.advance() // prefix operator + // Reserve the slot before the operand so nested `~~x` records in source order. + slot := -1 + if op == ast.OpBitNot { + slot = len(p.undefinedOps) + p.undefinedOps = append(p.undefinedOps, nil) + } operand := p.parseUnary() e := &ast.OperatorExpr{Operator: op, Operands: []ast.Node{operand}} e.NodeSpan = p.spanFrom(start) + if slot >= 0 { + p.undefinedOps[slot] = e + } return e } diff --git a/internal/syntax/parser/parser.go b/internal/syntax/parser/parser.go index de0f6f5fe..1ea05599a 100644 --- a/internal/syntax/parser/parser.go +++ b/internal/syntax/parser/parser.go @@ -36,6 +36,10 @@ type Parser struct { // as a declaration name. Warnings []Diagnostic + // undefinedOps are the `~` operator expressions the parse built, in source + // order; ParseFile hands them to the root. + undefinedOps []*ast.OperatorExpr + // calcBodyDepth counts the calculation bodies being parsed, so a `return` // reached in a statement position inside one is read as the result // parameter it declares rather than as an unknown action keyword. @@ -117,9 +121,10 @@ func (p *Parser) bodyContext() bodyContext { // parseCheckpoint captures parser state for backtracking. type parseCheckpoint struct { - pos int - diagnosticLen int - warningLen int + pos int + diagnosticLen int + warningLen int + undefinedOpLen int // triv is a copy of the pending trivia at the checkpoint; trivLogLen is // how much of trivLog was already lexed then. triv []ast.Trivia @@ -489,6 +494,7 @@ func (p *Parser) ParseFile() *ast.RootNamespace { } } root.NodeSpan = p.spanFrom(start) + root.UndefinedOperators = p.undefinedOps return root } @@ -497,13 +503,14 @@ func (p *Parser) ParseFile() *ast.RootNamespace { func (p *Parser) checkpoint() parseCheckpoint { p.checkpoints++ return parseCheckpoint{ - pos: p.pos, - diagnosticLen: len(p.Diagnostics), - warningLen: len(p.Warnings), - triv: slices.Clone(p.triv), - trivLogLen: len(p.trivLog), - pendingSpan: p.pendingComment, - hadPending: p.hasPendingComment, + pos: p.pos, + diagnosticLen: len(p.Diagnostics), + warningLen: len(p.Warnings), + undefinedOpLen: len(p.undefinedOps), + triv: slices.Clone(p.triv), + trivLogLen: len(p.trivLog), + pendingSpan: p.pendingComment, + hadPending: p.hasPendingComment, } } @@ -515,6 +522,7 @@ func (p *Parser) restore(cp parseCheckpoint) { p.pos = cp.pos p.Diagnostics = p.Diagnostics[:cp.diagnosticLen] p.Warnings = p.Warnings[:cp.warningLen] + p.undefinedOps = p.undefinedOps[:cp.undefinedOpLen] p.pendingComment = cp.pendingSpan p.hasPendingComment = cp.hadPending p.triv = append(cp.triv, p.trivLog[cp.trivLogLen:]...) diff --git a/internal/syntax/parser/undefined_operators_test.go b/internal/syntax/parser/undefined_operators_test.go new file mode 100644 index 000000000..3773d2ab2 --- /dev/null +++ b/internal/syntax/parser/undefined_operators_test.go @@ -0,0 +1,70 @@ +package parser + +import ( + "strings" + "testing" + + "github.com/Open-MBEE/OpenSysML/internal/syntax/ast" + "github.com/Open-MBEE/OpenSysML/internal/syntax/source" +) + +// The parser records every `~` operator expression on the root in source +// order, wherever the expression sits. +func TestUndefinedOperatorsRecordsEveryTilde(t *testing.T) { + src := "package p {\n" + + "attribute a = ~1;\n" + + "calc def C { return ~x; }\n" + + "attribute b = f(~y, 2);\n" + + "}" + root := New(source.New("t.sysml", []byte(src))).ParseFile() + ops := root.UndefinedOperators + if len(ops) != 3 { + t.Fatalf("UndefinedOperators len = %d, want 3", len(ops)) + } + for i := 1; i < len(ops); i++ { + if ops[i].Span().Offset <= ops[i-1].Span().Offset { + t.Fatalf("UndefinedOperators not in source order at %d", i) + } + } + for _, e := range ops { + if e.Operator != ast.OpBitNot { + t.Fatalf("recorded operator = %v, want OpBitNot", e.Operator) + } + } +} + +// `~~x` records both the inner and the outer `~` expression. +func TestUndefinedOperatorsRecordsNestedTildes(t *testing.T) { + src := "package p { attribute a = ~~x; }" + root := New(source.New("t.sysml", []byte(src))).ParseFile() + ops := root.UndefinedOperators + if len(ops) != 2 { + t.Fatalf("UndefinedOperators len = %d, want 2 for ~~x", len(ops)) + } + outer := strings.Index(src, "~~") + if got := []int{ops[0].Span().Offset, ops[1].Span().Offset}; got[0] != outer || got[1] != outer+1 { + t.Fatalf("offsets = %v, want outer %d then inner %d", got, outer, outer+1) + } +} + +// A checkpoint restore drops the `~` sites the abandoned attempt recorded. +func TestUndefinedOperatorsFollowsRestore(t *testing.T) { + p := newParser("~x + ~y") + if p.parseUnary(); len(p.undefinedOps) != 1 { + t.Fatalf("after ~x: len = %d, want 1", len(p.undefinedOps)) + } + cp := p.checkpoint() + p.advance() // '+' + if p.parseUnary(); len(p.undefinedOps) != 2 { + t.Fatalf("after ~y: len = %d, want 2", len(p.undefinedOps)) + } + p.restore(cp) + p.release() + if len(p.undefinedOps) != 1 || p.undefinedOps[0].Span().Offset != 0 { + t.Fatalf("after restore: %d ops, want only the ~ at offset 0", len(p.undefinedOps)) + } + p.advance() // '+' + if p.parseUnary(); len(p.undefinedOps) != 2 || p.undefinedOps[1].Span().Offset != 5 { + t.Fatalf("after reparse: %d ops, want the ~ at offset 5 second", len(p.undefinedOps)) + } +} diff --git a/internal/workspace/libs/stdlib.snapshot b/internal/workspace/libs/stdlib.snapshot index 3b61e0bc3..78a089a4b 100644 Binary files a/internal/workspace/libs/stdlib.snapshot and b/internal/workspace/libs/stdlib.snapshot differ diff --git a/tests/parser/testdata/parse/undefined-operator.golden b/tests/parser/testdata/parse/undefined-operator.golden new file mode 100644 index 000000000..137ae91ec --- /dev/null +++ b/tests/parser/testdata/parse/undefined-operator.golden @@ -0,0 +1,15 @@ +(RootNamespace + (Membership visibility="default" + (Package name="TildeDemo" library=false standard=false + (Membership visibility="default" + (Usage kind="attribute" name="magnitude" ref=false direction="none" composite=false derived=false ordered=false nonunique=false + (OperatorExpr operator="~" + (FeatureReference name="x")))) + (Membership visibility="default" + (Definition kind="calc" abstract=false variation=false name="Rough" + (OperatorExpr operator="+" + (OperatorExpr operator="~" + (FeatureReference name="y")) + (OperatorExpr operator="~" + (OperatorExpr operator="~" + (FeatureReference name="z"))))))))) \ No newline at end of file diff --git a/tests/parser/testdata/parse/undefined-operator.sysml b/tests/parser/testdata/parse/undefined-operator.sysml new file mode 100644 index 000000000..525666210 --- /dev/null +++ b/tests/parser/testdata/parse/undefined-operator.sysml @@ -0,0 +1,4 @@ +package TildeDemo { + attribute magnitude = ~x; + calc def Rough { ~y + ~~z } +} diff --git a/tests/parser/undefined_operators_test.go b/tests/parser/undefined_operators_test.go new file mode 100644 index 000000000..dc93fce76 --- /dev/null +++ b/tests/parser/undefined_operators_test.go @@ -0,0 +1,58 @@ +package parser_test + +import ( + "os" + "path/filepath" + "testing" + + "github.com/Open-MBEE/OpenSysML/internal/syntax/ast" + "github.com/Open-MBEE/OpenSysML/internal/syntax/parser" + "github.com/Open-MBEE/OpenSysML/internal/syntax/source" +) + +// TestUndefinedOperatorsCoversFixtures checks the parser's `~` record against +// an independent walk of every parse fixture. +func TestUndefinedOperatorsCoversFixtures(t *testing.T) { + fixtures := filepath.Join("testdata", "parse") + entries, err := os.ReadDir(fixtures) + if err != nil { + t.Fatalf("Failed to read fixtures dir %s: %v", fixtures, err) + } + + counted := 0 + for _, entry := range entries { + ext := filepath.Ext(entry.Name()) + if entry.IsDir() || (ext != ".sysml" && ext != ".kerml") { + continue + } + name := entry.Name() + t.Run(name, func(t *testing.T) { + content, err := os.ReadFile(filepath.Join(fixtures, name)) + if err != nil { + t.Fatalf("Failed to read fixture %s: %v", name, err) + } + sf := source.New(name, content) + root := parser.New(sf).ParseFile() + + want := 0 + ast.Inspect(root, func(n ast.Node) bool { + if e, ok := n.(*ast.OperatorExpr); ok && e.Operator == ast.OpBitNot { + want++ + } + return true + }) + if len(root.UndefinedOperators) != want { + t.Errorf("UndefinedOperators len = %d, tree walk counts %d", len(root.UndefinedOperators), want) + } + for _, e := range root.UndefinedOperators { + if e == nil || e.Operator != ast.OpBitNot { + t.Errorf("recorded operator is not a `~` expression: %v", e) + } + } + counted += want + }) + } + if counted == 0 { + t.Error("no fixture exercises a `~` operator; the comparison is vacuous") + } +}