From db9cc73250255c0719dcd674671003f7aeade379 Mon Sep 17 00:00:00 2001 From: Matthew Sackman Date: Wed, 29 Jan 2025 14:51:23 +0000 Subject: [PATCH] core/toposort: Correct CloseInfo Decl MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Toposort depends on StructInfos in Vertices to include the Decl which contains the StructInfo. It uses this to establish a parent-child relationship which enables it to walk topologically over the StructLits which contributed to a Vertex. In evalv3, when creating a StructInfo, the closeContext may contain the Decl we want. In evalv2, when creating a StructInfo, the closeInfo may contain the Decl we want, but this code was previously faulty: if the closeInfo had a nil Decl, then we would search parent closeInfos. This is faulty because it could feed into toposort the notion that a Decl is its own parent, which leads to infinite loop and stack explosion. Removing the search through any parent closeInfo solves this problem. The effect of this actually reduces the difference for toposort between evalv2 and evalv3, so it is beneficial in this regard too. Fixes #3710. Signed-off-by: Matthew Sackman Change-Id: I8a8e7c0e83386c20428bd2d8723d5b24f5181677 Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1207989 Reviewed-by: Marcel van Lohuizen Reviewed-by: Daniel Martí TryBot-Result: CUEcueckoo Unity-Result: CUE porcuepine --- .github/workflows/evict_caches.yaml | 8 +-- .github/workflows/trybot.yaml | 28 +++++----- .../cmd/testdata/script/cmd_issue2060.txtar | 4 +- internal/core/adt/closed.go | 15 ------ internal/core/adt/composite.go | 4 +- .../core/toposort/testdata/issue3710.txtar | 52 +++++++++++++++++++ internal/core/toposort/testdata/t.txtar | 50 ++---------------- tools/flow/testdata/issue2490.txtar | 19 +------ 8 files changed, 79 insertions(+), 101 deletions(-) create mode 100644 internal/core/toposort/testdata/issue3710.txtar diff --git a/.github/workflows/evict_caches.yaml b/.github/workflows/evict_caches.yaml index 55d712db60b..8afa89eb1ea 100644 --- a/.github/workflows/evict_caches.yaml +++ b/.github/workflows/evict_caches.yaml @@ -21,8 +21,8 @@ jobs: run: touch -t 202211302355 $(find * -type d) - name: Restore git file modification times uses: chetan/git-restore-mtime-action@075f9bc9d159805603419d50f794bd9f33252ebe - - id: DispatchTrailer - name: Try to extract Dispatch-Trailer + - name: Try to extract Dispatch-Trailer + id: DispatchTrailer run: |- x="$(git log -1 --pretty='%(trailers:key=Dispatch-Trailer,valueonly)')" if [[ "$x" == "" ]] @@ -38,11 +38,11 @@ jobs: echo "value<> $GITHUB_OUTPUT echo "$x" >> $GITHUB_OUTPUT echo "EOD" >> $GITHUB_OUTPUT - - if: |- + - name: Check we don't have Dispatch-Trailer on a protected branch + if: |- ((github.ref == 'refs/heads/master' || startsWith(github.ref, 'refs/heads/release-branch.')) && (! (contains(github.event.head_commit.message, ' Dispatch-Trailer: {"type":"')))) && (contains(github.event.head_commit.message, ' Dispatch-Trailer: {"type":"')) - name: Check we don't have Dispatch-Trailer on a protected branch run: |- echo "github.event.head_commit.message contains Dispatch-Trailer but we are on a protected branch" false diff --git a/.github/workflows/trybot.yaml b/.github/workflows/trybot.yaml index fd8c454f77f..f1d00a3be75 100644 --- a/.github/workflows/trybot.yaml +++ b/.github/workflows/trybot.yaml @@ -127,40 +127,40 @@ jobs: run: go test -short ./... - name: Test with -tags=cuewasm run: go test -tags cuewasm ./cmd/cue/cmd ./cue/interpreter/wasm - - name: gcloud auth for end-to-end tests - id: auth + - id: auth uses: google-github-actions/auth@v2 with: credentials_json: ${{ secrets.E2E_GCLOUD_KEY }} if: |- github.repository == 'cue-lang/cue' && (((github.ref == 'refs/heads/master' || startsWith(github.ref, 'refs/heads/release-branch.')) && (! (contains(github.event.head_commit.message, ' Dispatch-Trailer: {"type":"')))) || (github.ref == 'refs/heads/ci/test')) && (matrix.go-version == '1.23.x' && matrix.runner == 'ubuntu-24.04') - - name: gcloud setup for end-to-end tests + name: gcloud auth for end-to-end tests + - if: |- + github.repository == 'cue-lang/cue' && (((github.ref == 'refs/heads/master' || startsWith(github.ref, 'refs/heads/release-branch.')) && (! (contains(github.event.head_commit.message, ' + Dispatch-Trailer: {"type":"')))) || (github.ref == 'refs/heads/ci/test')) && (matrix.go-version == '1.23.x' && matrix.runner == 'ubuntu-24.04') + name: gcloud setup for end-to-end tests uses: google-github-actions/setup-gcloud@v2 - if: |- + - if: |- github.repository == 'cue-lang/cue' && (((github.ref == 'refs/heads/master' || startsWith(github.ref, 'refs/heads/release-branch.')) && (! (contains(github.event.head_commit.message, ' Dispatch-Trailer: {"type":"')))) || (github.ref == 'refs/heads/ci/test')) && (matrix.go-version == '1.23.x' && matrix.runner == 'ubuntu-24.04') - - name: End-to-end test - env: - CUE_TEST_TOKEN: ${{ secrets.E2E_PORCUEPINE_CUE_TOKEN }} + name: End-to-end test run: |- cd internal/_e2e go test -race - if: |- - github.repository == 'cue-lang/cue' && (((github.ref == 'refs/heads/master' || startsWith(github.ref, 'refs/heads/release-branch.')) && (! (contains(github.event.head_commit.message, ' - Dispatch-Trailer: {"type":"')))) || (github.ref == 'refs/heads/ci/test')) && (matrix.go-version == '1.23.x' && matrix.runner == 'ubuntu-24.04') - - name: Go checks + env: + CUE_TEST_TOKEN: ${{ secrets.E2E_PORCUEPINE_CUE_TOKEN }} + - if: (matrix.go-version == '1.23.x' && matrix.runner == 'ubuntu-24.04') + name: Go checks run: |- go vet ./... go mod tidy (cd internal/_e2e && go test -run=-) - if: (matrix.go-version == '1.23.x' && matrix.runner == 'ubuntu-24.04') - - name: staticcheck + - if: (matrix.go-version == '1.23.x' && matrix.runner == 'ubuntu-24.04') + name: staticcheck uses: dominikh/staticcheck-action@v1 with: version: 2024.1.1 install-go: false - if: (matrix.go-version == '1.23.x' && matrix.runner == 'ubuntu-24.04') - if: (matrix.go-version == '1.23.x' && matrix.runner == 'ubuntu-24.04') name: Check all git tags are available run: |- diff --git a/cmd/cue/cmd/testdata/script/cmd_issue2060.txtar b/cmd/cue/cmd/testdata/script/cmd_issue2060.txtar index a3c5713cb61..463d8578e72 100644 --- a/cmd/cue/cmd/testdata/script/cmd_issue2060.txtar +++ b/cmd/cue/cmd/testdata/script/cmd_issue2060.txtar @@ -136,11 +136,11 @@ StrokesName: ZenStrokes1 PlatformName: Linux CommandBindingsCount: 1 CommandBindings: - - CmdName: CursorMoveToLineForward + - BindingsMax: 0 + CmdName: CursorMoveToLineForward CmdHumanName: Cursor Down EditorCmdText: '"command": "move", "args": test' BindingsCount: 1 - BindingsMax: 0 Bindings: - DefText: Down BindText: Down diff --git a/internal/core/adt/closed.go b/internal/core/adt/closed.go index 007a768b1af..6db1eebf18b 100644 --- a/internal/core/adt/closed.go +++ b/internal/core/adt/closed.go @@ -180,7 +180,6 @@ func (c CloseInfo) SpawnEmbed(x Node) CloseInfo { mode: closeEmbed, root: EmbeddingSpan, span: c.span() | EmbeddingSpan, - decl: c.closeInfo.Decl(), } return c } @@ -194,7 +193,6 @@ func (c CloseInfo) SpawnGroup(x Expr) CloseInfo { parent: c.closeInfo, location: x, span: c.span(), - decl: c.closeInfo.Decl(), } return c } @@ -208,7 +206,6 @@ func (c CloseInfo) SpawnSpan(x Node, t SpanType) CloseInfo { location: x, root: t, span: c.span() | t, - decl: c.closeInfo.Decl(), } return c } @@ -231,7 +228,6 @@ func (c CloseInfo) SpawnRef(arc *Vertex, isDef bool, x Expr) CloseInfo { parent: c.closeInfo, location: x, span: span, - decl: c.closeInfo.Decl(), } } if isDef { @@ -306,17 +302,6 @@ type closeInfo struct { decl Decl } -// Returns the first non-nil Decl from c, or c's parents, if possible. -func (c *closeInfo) Decl() Decl { - for c != nil && c.decl == nil { - c = c.parent - } - if c == nil { - return nil - } - return c.decl -} - // closeStats holds the administrative fields for a closeInfo value. Each // closeInfo is associated with a single closeStats value per unification // operator. This association is done through an OpContext. This allows the diff --git a/internal/core/adt/composite.go b/internal/core/adt/composite.go index 29f57e07112..4e728285e82 100644 --- a/internal/core/adt/composite.go +++ b/internal/core/adt/composite.go @@ -1465,8 +1465,8 @@ func (v *Vertex) AddStruct(s *StructLit, env *Environment, ci CloseInfo) *Struct } if cc := ci.cc; cc != nil && cc.decl != nil { info.Decl = cc.decl - } else if decl := ci.closeInfo.Decl(); decl != nil { - info.Decl = decl + } else if ci := ci.closeInfo; ci != nil && ci.decl != nil { + info.Decl = ci.decl } for _, t := range v.Structs { if *t == info { // TODO: check for different identity. diff --git a/internal/core/toposort/testdata/issue3710.txtar b/internal/core/toposort/testdata/issue3710.txtar new file mode 100644 index 00000000000..58b1c260e27 --- /dev/null +++ b/internal/core/toposort/testdata/issue3710.txtar @@ -0,0 +1,52 @@ +-- input.cue -- +package p + +#Exporter: { + _objects: {...} + _imports: [...] + + objects: [ + for _, objs in _objects { + objs + } + for import in _imports for obj in import._export.objects { + obj + } + ] +} + +out: #Exporter & { + _imports: [_app1] +} +_app1: { + _export: #Exporter & { + _imports: [_app2] + } +} +_app2: { + _export: #Exporter & { + _objects: leaf: leafField: "leaf value" + } +} +-- out/TestTopologicalSort/lexicographical=false -- +{ + #Exporter: { + objects: [] + } + out: { + objects: [{ + leafField: "leaf value" + }] + } +} +-- out/TestTopologicalSort/lexicographical=true -- +{ + #Exporter: { + objects: [] + } + out: { + objects: [{ + leafField: "leaf value" + }] + } +} diff --git a/internal/core/toposort/testdata/t.txtar b/internal/core/toposort/testdata/t.txtar index 234b1383067..3070d930e3f 100644 --- a/internal/core/toposort/testdata/t.txtar +++ b/internal/core/toposort/testdata/t.txtar @@ -366,7 +366,7 @@ k2: { diff old new --- old +++ new -@@ -56,35 +56,35 @@ +@@ -56,19 +56,19 @@ w: 4 } d3: { @@ -396,48 +396,6 @@ diff old new t: 10 } d5: { -- za: 11 -- x: 3 -- w: 4 -- z: 1 -- y: 2 -+ x: 3 -+ w: 4 -+ z: 1 -+ y: 2 -+ za: 11 - t: 10 - } - d6: { -- za: 11 -- x: 3 -- w: 4 -- z: 1 -- y: 2 -+ x: 3 -+ w: 4 -+ z: 1 -+ y: 2 -+ za: 11 - t: 10 - } - e1: { -@@ -232,11 +232,11 @@ - z: 3 - } - k2: { -+ d: 0 -+ c: 1 -+ b: 2 -+ z: 3 - e: 3 -- d: 0 -- c: 1 -- b: 2 -- z: 3 - a: 0 - } - } -- diff/explanation/todo/p1 -- Not sure what is wrong. -- out/TestTopologicalSort/lexicographical=false -- @@ -515,19 +473,19 @@ Not sure what is wrong. t: 10 } d5: { - za: 11 x: 3 w: 4 z: 1 y: 2 + za: 11 t: 10 } d6: { - za: 11 x: 3 w: 4 z: 1 y: 2 + za: 11 t: 10 } e1: { @@ -675,11 +633,11 @@ Not sure what is wrong. z: 3 } k2: { - e: 3 d: 0 c: 1 b: 2 z: 3 + e: 3 a: 0 } } diff --git a/tools/flow/testdata/issue2490.txtar b/tools/flow/testdata/issue2490.txtar index 8d2c11309ec..3666ad8ffb6 100644 --- a/tools/flow/testdata/issue2490.txtar +++ b/tools/flow/testdata/issue2490.txtar @@ -23,28 +23,11 @@ graph TD graph TD t0("root.foo [Terminated]") --- out/run-v3/t1/value -- -{ - $id: "tool/cli.Print" - text: "{}" - stdout: "foo" -} --- diff/-out/run-v3/t1/value<==>+out/run/t1/value -- -diff old new ---- old -+++ new -@@ -1,5 +1,5 @@ - { - $id: "tool/cli.Print" -- stdout: "foo" - text: "{}" -+ stdout: "foo" - } -- out/run/t1/value -- { $id: "tool/cli.Print" - stdout: "foo" text: "{}" + stdout: "foo" } -- out/run/t1/stats -- Leaks: 0