Skip to content

Commit

Permalink
core/toposort: Correct CloseInfo Decl
Browse files Browse the repository at this point in the history
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 <[email protected]>
Change-Id: I8a8e7c0e83386c20428bd2d8723d5b24f5181677
Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1207989
Reviewed-by: Marcel van Lohuizen <[email protected]>
Reviewed-by: Daniel Martí <[email protected]>
TryBot-Result: CUEcueckoo <[email protected]>
Unity-Result: CUE porcuepine <[email protected]>
  • Loading branch information
cuematthew committed Jan 30, 2025
1 parent 870bfc2 commit db9cc73
Show file tree
Hide file tree
Showing 8 changed files with 79 additions and 101 deletions.
8 changes: 4 additions & 4 deletions .github/workflows/evict_caches.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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" == "" ]]
Expand All @@ -38,11 +38,11 @@ jobs:
echo "value<<EOD" >> $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
Expand Down
28 changes: 14 additions & 14 deletions .github/workflows/trybot.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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: |-
Expand Down
4 changes: 2 additions & 2 deletions cmd/cue/cmd/testdata/script/cmd_issue2060.txtar
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
15 changes: 0 additions & 15 deletions internal/core/adt/closed.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand All @@ -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
}
Expand All @@ -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
}
Expand All @@ -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 {
Expand Down Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions internal/core/adt/composite.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
52 changes: 52 additions & 0 deletions internal/core/toposort/testdata/issue3710.txtar
Original file line number Diff line number Diff line change
@@ -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"
}]
}
}
50 changes: 4 additions & 46 deletions internal/core/toposort/testdata/t.txtar
Original file line number Diff line number Diff line change
Expand Up @@ -366,7 +366,7 @@ k2: {
diff old new
--- old
+++ new
@@ -56,35 +56,35 @@
@@ -56,19 +56,19 @@
w: 4
}
d3: {
Expand Down Expand Up @@ -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 --
Expand Down Expand Up @@ -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: {
Expand Down Expand Up @@ -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
}
}
Expand Down
19 changes: 1 addition & 18 deletions tools/flow/testdata/issue2490.txtar
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit db9cc73

Please sign in to comment.