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

fix: Fix error messages when a comprehension iterates over a non-array. #797

Conversation

rudo-thomas
Copy link
Contributor

Problems:

  • When iterating over an empty string in a list comprehension, the result is an empty string. This is a bug, it should be an error.
  • When iterating over a non-empty string in a list comprehension, the expected and unexpected types in the error message are swapped.
  • Error messages mention "std.flatMap" when object/list comprehensions would iterate over a value that is neither array nor string.
$ jsonnet --version
Jsonnet commandline interpreter (Go implementation) v0.21.0-rc2
$ jsonnet -e '[a for a in ""]'
""
$ jsonnet -e '[a for a in "b"]'
RUNTIME ERROR: Unexpected type array, expected string
	<cmdline>:1:1-17
	During evaluation
$ jsonnet -e '{[a]: 1 for a in 2}'
RUNTIME ERROR: std.flatMap second param must be array / string, got number
	<cmdline>:1:1-20
	<cmdline>:1:1-20
	During evaluation
$ jsonnet -e '[a for a in 1]'
RUNTIME ERROR: std.flatMap second param must be array / string, got number
	<cmdline>:1:1-15
	During evaluation

FWIW, the C++ implementation does not have any of these problems. It gives:

RUNTIME ERROR: In comprehension, can only iterate over array.

In the Go implementation comprehensions are desugared to a call to std.flatMap which does accept a string in the "arr" parameter.

The fix: Desugar comprehensions to a call to a new hidden builtin which only accepts arrays.

@rudo-thomas
Copy link
Contributor Author

@johnbartholomew
Maybe this could still make it into 0.21.0-rc3 (or final)? PTaL 🙏

@coveralls
Copy link

coveralls commented Mar 21, 2025

Coverage Status

coverage: 55.582% (+0.008%) from 55.574%
when pulling 1c79f57 on rudo-thomas:rudo-fix-comprehension-over-string-error-message
into 1add1e1 on google:master.

Problems:
 - When iterating over an empty string in a list comprehension, the
   result is an empty string. This is a bug, it should be an error.
 - When iterating over a non-empty string in a list comprehension, the
   expected and unexpected types in the error message are swapped.
 - Error messages mention "std.flatMap" when object/list comprehensions
   would iterate over a value that is neither array nor string.

```
$ jsonnet --version
Jsonnet commandline interpreter (Go implementation) v0.21.0-rc2
$ jsonnet -e '[a for a in ""]'
""
$ jsonnet -e '[a for a in "b"]'
RUNTIME ERROR: Unexpected type array, expected string
	<cmdline>:1:1-17
	During evaluation
$ jsonnet -e '{[a]: 1 for a in 2}'
RUNTIME ERROR: std.flatMap second param must be array / string, got number
	<cmdline>:1:1-20
	<cmdline>:1:1-20
	During evaluation
$ jsonnet -e '[a for a in 1]'
RUNTIME ERROR: std.flatMap second param must be array / string, got number
	<cmdline>:1:1-15
	During evaluation
```

FWIW, the C++ implementation does not have any of these problems. It
gives:
```
RUNTIME ERROR: In comprehension, can only iterate over array.
```

In the Go implementation comprehensions are desugared to a call to
std.flatMap which does accept a string in the "arr" parameter.

The fix: Desugar comprehensions to a call to a new hidden builtin which
only accepts arrays.
@johnbartholomew
Copy link
Collaborator

johnbartholomew commented Mar 21, 2025

Thanks for finding & fixing this. The implementation looks good to me.

@johnbartholomew johnbartholomew force-pushed the rudo-fix-comprehension-over-string-error-message branch from 82a7789 to 1c79f57 Compare March 21, 2025 16:41
@johnbartholomew johnbartholomew merged commit 1c79f57 into google:master Mar 21, 2025
9 checks passed
@johnbartholomew
Copy link
Collaborator

Rebased & merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants