Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

internal/eval: strange let declaration behaviour #1116

Closed
myitcv opened this issue Jul 19, 2021 · 10 comments
Closed

internal/eval: strange let declaration behaviour #1116

myitcv opened this issue Jul 19, 2021 · 10 comments
Labels
NeedsFix roadmap/requiredfield Related to required fields proposal.

Comments

@myitcv
Copy link
Member

myitcv commented Jul 19, 2021

What version of CUE are you using (cue version)?

$ cue version
cue version +a4a38ed8 linux/amd64

Does this issue reproduce with the latest release?

Yes

What did you do?

Consider the following:

-- broken.cue --
package p

import "strings"

#f: {
	#in: string
	let List = strings.Split(#in, "/")
	List[len(List)-1]
}

test: #f & {_, #in: "hello/world/you"}
-- working.cue --
package p

import "strings"

#f: {
	#in: string
	let List = strings.Split(#in, "/")
	List
}

test: #f & {_, #in: "hello/world/you"}

cue export working.cue behaves as expected:

$ cue export !$
cue export working.cue
{
    "test": [
        "hello",
        "world",
        "you"
    ]
}

However, cue eval working.cue looks incorrect; notice the let declaration has shifted scope. This could, however, be a presentational issue:

$ cue eval working.cue
import "strings"

let List = strings.Split(#in, "/")
#f: {
    List
    #in: string
}
test: {
    #in: "hello/world/you"
    ["hello", "world", "you"]
}

Now consider cue export broken.cue:

$ cue export broken.cue
test: undefined field: #in:
    ./broken.cue:7:27

The trigger for this appears to be the fact that List is referenced from the index expression (noting however that List + List also fails with the same error). Contrast working.cue where the plain use of List works just fine.

cue eval broken.cue also looks "broken" in the same way as working.cue:

$ cue eval broken.cue
import "strings"

let List = strings.Split(#in, "/")
#f: {
    List[len(List)-1]
    #in: string
}
test: #f & {
    _
    #in: "hello/world/you"
}

What did you expect to see?

  • cue eval showing the let expression in the correct scope
  • cue export to not error in either case.

What did you see instead?

As above.

@myitcv myitcv added this to the v0.5.x milestone Jul 19, 2021
@myitcv
Copy link
Member Author

myitcv commented Jul 19, 2021

I forgot to note in the initial description that this came to light as a result of the discussion in #1096 - thanks @PierreR

@PierreR
Copy link

PierreR commented Jul 19, 2021

@myitcv Is this related to the fact that let needs to be enclosed in an embedded ?

// KO
test:
    let Name = "hello"
    Name
// OK
test: {
    let Name = "hello"
    Name
}

Or is this working as intended ?

@myitcv
Copy link
Member Author

myitcv commented Jul 20, 2021

That is working as intended AFAICT. From the spec:

Within a struct, a let clause binds an identifier to the given expression.

Within the scope of the identifier, the identifier refers to the locally declared expression. The expression is evaluated in the scope it was declared.

@mpvl
Copy link
Member

mpvl commented Nov 24, 2021

I think

test:
    let Name = "hello"
    Name

Is a separate issue. It should probably not result in a valid parse.

@mpvl mpvl added the roadmap/requiredfield Related to required fields proposal. label Nov 24, 2021
@mpvl
Copy link
Member

mpvl commented Nov 24, 2021

Minimal reproducer of the above:

#a: ["a", "b"]
let List = #a
List[1]

@mpvl
Copy link
Member

mpvl commented Nov 24, 2021

Analysis: let gets evaluated before the all arcs have been added to the struct. Normally this is fine and evaluation is postponed. But for lets the evaluations are cached and thus it never recovers.

A solution would be to not store incomplete errors. But this may lead to exponential runtime.

A better solution would be treat let expressions as field, but just handle their scope differently. The same treatment intended for optional fields, which is necessary for structure sharing.

@mpvl
Copy link
Member

mpvl commented Nov 24, 2021

Will submit a fix for the original problem. Note that the unrelated issue of a let in a non-scoped field value position is still open.

@mpvl
Copy link
Member

mpvl commented Nov 24, 2021

See also:

Name: 1
test:
    let Name = "hello"
    Name

cueckoo pushed a commit that referenced this issue Nov 24, 2021
These may be resolved later.

Issue #1116

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

Change-Id: I46b8dda41193b41f5b6b0fd341164a2568697bfc
@mpvl mpvl modified the milestones: v0.5.x, v0.4.1: bug fixes Nov 24, 2021
cueckoo pushed a commit that referenced this issue Nov 24, 2021
These may be resolved later.

Issue #1116

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

Change-Id: I46b8dda41193b41f5b6b0fd341164a2568697bfc
Signed-off-by: Paul Jolly <[email protected]>
cueckoo pushed a commit that referenced this issue Nov 25, 2021
These may be resolved later.

Issue #1116

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

Change-Id: I46b8dda41193b41f5b6b0fd341164a2568697bfc
Signed-off-by: Marcel van Lohuizen <[email protected]>
cueckoo pushed a commit that referenced this issue Nov 25, 2021
These may be resolved later.

Issue #1116

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

Change-Id: I46b8dda41193b41f5b6b0fd341164a2568697bfc
Signed-off-by: Marcel van Lohuizen <[email protected]>
cueckoo pushed a commit that referenced this issue Nov 25, 2021
These may be resolved later.

Issue #1116

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

Change-Id: I46b8dda41193b41f5b6b0fd341164a2568697bfc
Signed-off-by: Marcel van Lohuizen <[email protected]>
Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/528052
Reviewed-by: Marcel van Lohuizen <[email protected]>
@myitcv
Copy link
Member Author

myitcv commented Nov 25, 2021

Further reproducer of the let moving out of scope (running with 9982526):

exec cue def x.cue
stdin stdout
exec cue def -

-- x.cue --
#TypeBool: {
	_args: required: bool
	let Args = _args
	if !Args.required {
		default: bool
	}
}
#TypePrimitive: {
	_args: {
		required: bool
	}
	let Args = _args
	{"bool": #TypeBool & {_args: required: Args.required}}
}

Which gives the output:

> exec cue def x.cue
[stdout]
#TypeBool: {
        _args: {
                required: bool
        }
        if !Args.required {
                default: bool
        }
}
let Args_1 = _args
#TypePrimitive: {
        _args: {
                required: bool
        }
        bool: #TypeBool & {
                _args: {
                        required: Args_1.required
                }
        }
}
> stdin stdout
> exec cue def -
[stderr]
#TypeBool: reference "Args" not found:
    -:5:6
let[]: reference "_args" not found:
    -:9:14
[exit status 1]

Notice the failure for the second cue def which fails as a result of one let being dropped from #TypeBool but also the let previously within #TypePrimitive moving up a scope. It's almost like the two similarly named let's are being incorrectly collapsed into one.

@mpvl
Copy link
Member

mpvl commented Nov 25, 2021

Regarding the latter, the issue seems to occur when the reference is nested, so

Foo: {
	_args: bool
	let Args = _args
	bool:  Args.required
}

gives the right input, but using {bool: Args.required}, bool: [Args.required], or bool: {args: required} does not. More nesting pushes it up higher, if possible.

cueckoo pushed a commit that referenced this issue Nov 26, 2021
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
cueckoo pushed a commit that referenced this issue Nov 26, 2021
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]>
@mpvl mpvl modified the milestones: v0.5.x, v0.4.1: bug fixes Nov 26, 2021
cueckoo pushed a commit that referenced this issue Nov 30, 2021
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]>
cueckoo pushed a commit that referenced this issue Nov 30, 2021
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]>
cueckoo pushed a commit that referenced this issue Dec 1, 2021
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]>
cueckoo pushed a commit that referenced this issue Dec 1, 2021
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]>
@cueckoo cueckoo closed this as completed in 9a040dc Dec 1, 2021
jlongtine pushed a commit to jlongtine/cue that referenced this issue Dec 8, 2021
These may be resolved later.

Issue cue-lang#1116

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

Change-Id: I46b8dda41193b41f5b6b0fd341164a2568697bfc
Signed-off-by: Marcel van Lohuizen <[email protected]>
Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/528052
Reviewed-by: Marcel van Lohuizen <[email protected]>
Signed-off-by: Joel Longtine <[email protected]>
jlongtine pushed a commit to jlongtine/cue that referenced this issue Dec 8, 2021
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 cue-lang#1116

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

Change-Id: I31c05485a5af4d9b423c71bd4e51fc05b5d4a5d7
Signed-off-by: Marcel van Lohuizen <[email protected]>
Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/528214
TryBot-Result: CUEcueckoo <[email protected]>
Reviewed-by: Marcel van Lohuizen <[email protected]>
Signed-off-by: Joel Longtine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix roadmap/requiredfield Related to required fields proposal.
Projects
None yet
Development

No branches or pull requests

3 participants