Skip to content

Commit

Permalink
internal/core/export: fix let exporting
Browse files Browse the repository at this point in the history 
This is a hacky fix that relies on the original let AST expression
to be present in the ADT. In practice this will almost certainly
always be the case, though.

As part of this, the name uniquer has been fixed to remember
the random number. The exporter methods were defined on the
value, instead of the pointer type, so the random generator was
created on each instance.

The first few numbers are now fixed, to have more sane numbers
in the majority of cases.

This algorithm has the additional property that if an expression
refers to a let that is not exported, the expression will be inlined.
This does not give the correct result if this expression itself
contains references, but that is a general problem with the
current exporter.

NOTE: the filepath substitution is incorrect in value mode. The original
was incorrect as well, however, as the alias also was inserted in the
wrong scope. In practice this only matters for the already broken
`eval`. In export it would always generate an error anyway.

Unique name generator comment moved to where it applies.

Fixes #1116

Signed-off-by: Marcel van Lohuizen <[email protected]>

Change-Id: I31c05485a5af4d9b423c71bd4e51fc05b5d4a5d7
Signed-off-by: Marcel van Lohuizen <[email protected]>
  • Loading branch information
@mpvl
mpvl committed Nov 30, 2021
1 parent 910ff4d commit 6902110
Show file tree
Hide file tree
Showing 7 changed files with 550 additions and 91 deletions.
46 changes: 1 addition & 45 deletions internal/core/export/adt.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -179,51 +179,7 @@ func (e *exporter) adt(expr adt.Expr, conjuncts []adt.Conjunct) ast.Expr {
return ident

case *adt.LetReference:
// TODO: Handle references that went out of scope. In case of aliases
// this means they may need to be reproduced locally. Most of these
// issues can be avoided by either fully expanding a configuration
// (export) or not at all (def).
//
i := len(e.stack) - 1 - int(x.UpCount) - 1
if i < 0 {
i = 0
}
f := &(e.stack[i])
let := f.let[x.X]
if let == nil {
if f.let == nil {
f.let = map[adt.Expr]*ast.LetClause{}
}
label := e.uniqueLetIdent(x.Label, x.X)

name := label.IdentString(e.ctx)

// A let may be added multiple times to the same scope as a result
// of how merging works. If that happens here it must be one
// originating from the same expression, and it is safe to drop.
for _, elt := range f.scope.Elts {
if a, ok := elt.(*ast.LetClause); ok {
if a.Ident.Name == name {
let = a
break
}
}
}

if let == nil {
let = &ast.LetClause{
Ident: e.ident(label),
Expr: e.expr(x.X),
}
f.scope.Elts = append(f.scope.Elts, let)
}

f.let[x.X] = let
}
ident := ast.NewIdent(let.Ident.Name)
ident.Node = let
ident.Scope = f.scope
return ident
return e.resolveLet(x)

case *adt.SelectorExpr:
return &ast.SelectorExpr{
Expand Down
114 changes: 96 additions & 18 deletions internal/core/export/export.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ type exporter struct {
index adt.StringIndexer
rand *rand.Rand

// For resolving up references.
// For resolving references.
stack []frame

inDefinition int // for close() wrapping.
Expand All @@ -231,6 +231,7 @@ type exporter struct {
usedFeature map[adt.Feature]adt.Expr
labelAlias map[adt.Expr]adt.Feature
valueAlias map[*ast.Alias]*ast.Alias
letAlias map[*ast.LetClause]*ast.LetClause

usedHidden map[string]bool
}
Expand Down Expand Up @@ -307,15 +308,76 @@ func setFieldAlias(f *ast.Field, name string) {
}
}

// uniqueLetIdent returns a name for a let identifier that uniquely identifies
// the given expression. If the preferred name is already taken, a new globally
// unique name of the form base_X ... base_XXXXXXXXXXXXXX is generated.
//
// It prefers short extensions over large ones, while ensuring the likelihood of
// fast termination is high. There are at least two digits to make it visually
// clearer this concerns a generated number.
//
func (e exporter) uniqueLetIdent(f adt.Feature, x adt.Expr) adt.Feature {
func (e *exporter) markLets(n ast.Node) {
switch v := n.(type) {
case *ast.StructLit:
e.markLetDecls(v.Elts)
case *ast.File:
e.markLetDecls(v.Decls)
}
}

func (e *exporter) markLetDecls(decls []ast.Decl) {
for _, d := range decls {
if let, ok := d.(*ast.LetClause); ok {
e.markLetAlias(let)
}
}
}

// markLetAlias inserts an uninitialized let clause into the current scope.
// It gets initialized upon first usage.
func (e *exporter) markLetAlias(x *ast.LetClause) {
// The created let clause is initialized upon first usage, and removed
// later if never referenced.
let := &ast.LetClause{}

if e.letAlias == nil {
e.letAlias = make(map[*ast.LetClause]*ast.LetClause)
}
e.letAlias[x] = let

scope := e.top().scope
scope.Elts = append(scope.Elts, let)
}

// In value mode, lets are only used if there wasn't an error.
func filterUnusedLets(s *ast.StructLit) {
k := 0
for i, d := range s.Elts {
if let, ok := d.(*ast.LetClause); ok && let.Expr == nil {
continue
}
s.Elts[k] = s.Elts[i]
k++
}
s.Elts = s.Elts[:k]
}

// resolveLet actually parses the let expression.
// If there was no recorded let expression, it expands the expression in place.
func (e *exporter) resolveLet(x *adt.LetReference) ast.Expr {
letClause, _ := x.Src.Node.(*ast.LetClause)
let := e.letAlias[letClause]

switch {
case let == nil:
return e.expr(x.X)

case let.Expr == nil:
label := e.uniqueLetIdent(x.Label, x.X)

let.Ident = e.ident(label)
let.Expr = e.expr(x.X)
}

ident := ast.NewIdent(let.Ident.Name)
ident.Node = let
// TODO: set scope?
return ident
}

func (e *exporter) uniqueLetIdent(f adt.Feature, x adt.Expr) adt.Feature {
if e.usedFeature[f] == x {
return f
}
Expand All @@ -325,7 +387,7 @@ func (e exporter) uniqueLetIdent(f adt.Feature, x adt.Expr) adt.Feature {
return f
}

func (e exporter) uniqueAlias(name string) string {
func (e *exporter) uniqueAlias(name string) string {
f := adt.MakeIdentLabel(e.ctx, name, "")

if _, ok := e.usedFeature[f]; !ok {
Expand All @@ -337,29 +399,46 @@ func (e exporter) uniqueAlias(name string) string {
return name
}

func (e exporter) uniqueFeature(base string) (f adt.Feature, name string) {
// uniqueFeature returns a name for an identifier that uniquely identifies
// the given expression. If the preferred name is already taken, a new globally
// unique name of the form base_X ... base_XXXXXXXXXXXXXX is generated.
//
// It prefers short extensions over large ones, while ensuring the likelihood of
// fast termination is high. There are at least two digits to make it visually
// clearer this concerns a generated number.
//
func (e *exporter) uniqueFeature(base string) (f adt.Feature, name string) {
if e.rand == nil {
e.rand = rand.New(rand.NewSource(808))
}

// Try the first few numbers in sequence.
for i := 1; i < 5; i++ {
name := fmt.Sprintf("%s_%01X", base, i)
f := adt.MakeIdentLabel(e.ctx, name, "")
if _, ok := e.usedFeature[f]; !ok {
e.usedFeature[f] = nil
return f, name
}
}

const mask = 0xff_ffff_ffff_ffff // max bits; stay clear of int64 overflow
const shift = 4 // rate of growth
digits := 1
for n := int64(0x10); ; n = int64(mask&((n<<shift)-1)) + 1 {
num := e.rand.Intn(int(n))
name := fmt.Sprintf("%s_%01X", base, num)
num := e.rand.Intn(int(n)-1) + 1
name := fmt.Sprintf("%[1]s_%0[2]*[3]X", base, digits, num)
f := adt.MakeIdentLabel(e.ctx, name, "")
if _, ok := e.usedFeature[f]; !ok {
e.usedFeature[f] = nil
return f, name
}
digits++
}
}

type completeFunc func(scope *ast.StructLit, m adt.Node)

type frame struct {
scope *ast.StructLit
todo []completeFunc

docSources []adt.Conjunct

Expand All @@ -370,7 +449,6 @@ type frame struct {

// labeled fields
fields map[adt.Feature]entry
let map[adt.Expr]*ast.LetClause

// field to new field
mapped map[adt.Node]ast.Node
Expand Down
11 changes: 7 additions & 4 deletions internal/core/export/expr.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ func (x *exporter) mergeValues(label adt.Feature, src *adt.Vertex, a []conjunct,
_, saved := e.pushFrame(orig)
defer e.popFrame(saved)

// Handle value aliases
// Handle value aliases and lets
var valueAlias *ast.Alias
for _, c := range a {
if f, ok := c.c.Field().Source().(*ast.Field); ok {
Expand All @@ -110,14 +110,19 @@ func (x *exporter) mergeValues(label adt.Feature, src *adt.Vertex, a []conjunct,
e.valueAlias[a] = valueAlias
}
}
x.markLets(c.c.Expr().Source())
}

defer func() {
if valueAlias != nil {
valueAlias.Expr = expr
expr = valueAlias
}
}()

s := x.top().scope
hasAlias := len(s.Elts) > 0

for _, c := range a {
e.top().upCount = c.up
x := c.c.Expr()
Expand All @@ -133,8 +138,6 @@ func (x *exporter) mergeValues(label adt.Feature, src *adt.Vertex, a []conjunct,
}
}

s := x.top().scope

for _, a := range e.attrs {
s.Elts = append(s.Elts, a)
}
Expand Down Expand Up @@ -184,7 +187,7 @@ func (x *exporter) mergeValues(label adt.Feature, src *adt.Vertex, a []conjunct,
} else {
x = e.embed[0]
}
if len(e.attrs) == 0 {
if len(e.attrs) == 0 && !hasAlias {
return x
}
if st, ok := x.(*ast.StructLit); ok {
Expand Down
14 changes: 7 additions & 7 deletions internal/core/export/testdata/alias.txtar
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,12 +67,12 @@ fieldAlias: {
}
cross: {
baz: 3
X_85="d-2": {
X_2="d-2": {
E=[D="cue"]: {
C="foo\(baz)": {
name: "xx"
foo: C.name
bar: X_85
bar: X_2
baz: D
qux: E
}
Expand All @@ -84,8 +84,8 @@ valueAlias: {
merge: {
// Merge fields, rename alias to avoid conflict.
// TODO: merged values can still be simplified.
value: X_BA={
#value: X_BA.b & X_BA.b
value: X_3={
#value: X_3.b & X_3.b
b: 2
v: {
X: 3
Expand All @@ -95,8 +95,8 @@ valueAlias: {
selfRef: {
struct: {
a: {
b: X_57C8={
#foo: X_57C8.b
b: X_4={
#foo: X_4.b
b: 2
}
}
Expand All @@ -110,7 +110,7 @@ valueAlias: {
// an issue exclusive to value aliases, and falls within the
// range of what is acceptable for now.
// TODO: solve this issue.
a: X_35B7E=or(X_35B7E)
a: X_8=or(X_8)
}
pattern: {
// this triggers the verbatim "adt" path. Note that there
Expand Down
Loading

0 comments on commit 6902110

Please sign in to comment.