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

Let error(null) throw null #2823

Merged
merged 1 commit into from
Aug 4, 2023
Merged

Let error(null) throw null #2823

merged 1 commit into from
Aug 4, 2023

Conversation

emanuele6
Copy link
Member

@emanuele6 emanuele6 commented Aug 4, 2023

This patch removes the weird behaviour of jv_invalid_with_msg(jv_null()) that returns jv_invalid() (i.e. empty), instead of a boxed jv_null().

The previous behaviour of null|error was obviously unintentional, and allowing jv_invalid_with_msg() to return values on which you can't call jv_invalid_get_msg() is only error prone.


I don't see a reason to keep and document this behaviour, it is obviously a bug, it is not useful, and it was not documented in jq 1.6.

Either way, jv_invalid_with_msg(jv_null()) should be fixed to not return jv_invalid() in my opinion.
The "null|error is equivalent to empty" behaviour can be implemented in f_error if we really want to keep it.

diff --git a/src/builtin.c b/src/builtin.c
index 0d0ac23..e84bef9 100644
--- a/src/builtin.c
+++ b/src/builtin.c
@@ -1139,6 +1139,8 @@ static jv f_nan(jq_state *jq, jv input) {
 }
 
 static jv f_error(jq_state *jq, jv input) {
+  if (jv_get_kind(input) == JV_KIND_NULL)
+    return jv_invalid();
   return jv_invalid_with_msg(input);
 }
 

This patch removes the weird behaviour of jv_invalid_with_msg(jv_null())
that returns jv_invalid() (i.e. empty), instead of a boxed jv_null().

The previous behaviour of  null|error  was obviously unintentional, and
allowing is jv_invalid_with_msg() to return values on which you can't
call jv_invalid_get_msg() is only error prone.
@nicowilliams nicowilliams merged commit f94a9d4 into jqlang:master Aug 4, 2023
29 checks passed
@nicowilliams
Copy link
Contributor

Thanks!

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

Successfully merging this pull request may close these issues.

None yet

2 participants