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 string multiplication with a value between 0.0 and 1.0 #2142

Merged

Conversation

itchyny
Copy link
Contributor

@itchyny itchyny commented Jun 12, 2020

This commit fixes a regression of 6306ac8 (#2118). jq -n '0.5 * "abc"' should print "abc" not null. Based on the discussion, we change the behavior of string * number to emit "" when 0 ≦ number < 1.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 84.168% when pulling 9334a4a on itchyny:fix-string-multiplication-regression into a17dd32 on stedolan:master.

@itchyny
Copy link
Contributor Author

itchyny commented Jul 8, 2023

This is makes it behave as same as 1.6, but I personally prefer string * number emits "" when 0 ≦ number < 1.

@pkoppstein
Copy link
Contributor

@itchyny - There are certainly good arguments for evaluating String * 0 as "", so much so that I would be happy to open a “bug report” to that effect if you would be willing to make the change. As things are, I often define a helper function to work around the problem. Of course, it’s easy enough to use // but it’s so easy to forget that jq somehow got this one wrong….

@nicowilliams
Copy link
Contributor

nicowilliams commented Jul 8, 2023 via email

@pkoppstein
Copy link
Contributor

pkoppstein commented Jul 9, 2023

@nicowilliams wrote:

I'd like to remove string multiplication

Well, that particular horse is out of the barn, grazing in some
distant field in another country with which we no longer have
an extradition treaty.

Anyway, STRING * NUMBER is well-understood and quite widely adopted.(*)

The thing that is really questionable is NUMBER | length,
which evidently comes from mathematical notation (|...|) but still,
even mathematicians don't pronounce that as "length".

Since that horse too is out of the barn, I would propose that (at least pending jq 2.0):

a) NUMBER | length be officially deprecated.
b) def abs: be added

q.v. #2672


(*) Courtesy of ChatGPT:

Python:
"ab" * 2

Ruby:
"ab" * 2

JavaScript:
"ab".repeat(2);

Perl:
my $result = "ab" x 2;

PHP:
$result = str_repeat("ab", 2);

Elixir:
result = String.duplicate("ab", 2)

@nicowilliams
Copy link
Contributor

Well, that particular horse is out of the barn, grazing in some
distant field in another country with which we no longer have
an extradition treaty.

This is one reason I'd like to have builtin modules, and then we could have jq::v1_6, jq::v1_7, etc., and we could change builtins' behaviors in backwards incompatible ways.

src/builtin.c Outdated
@@ -359,7 +359,8 @@ static jv f_multiply(jq_state *jq, jv input, jv a, jv b) {
num = a;
}
jv res = jv_null();
int n = jv_number_value(num);
double d = jv_number_value(num);
int n = 0.0 < d && d < 1.0 ? 1 : d;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Questions while we're here:

  • if ("string" * 0) == null, why should ("string" * 0.1) == "string"?
  • shouldn't ("string") * 0 == ""? null seems very wrong
  • currently multiplying by a negative number, nan, -nan, infinite, or -infinite yields null -- why not an error, or empty? (well, empty would be harder to arrange here, so let's not go there)

@pkoppstein
Copy link
Contributor

Questions while we're here:
@nicowilliams wrote:
shouldn't ("string") * 0 == ""? null seems very wrong

Yes!!! I think we're all (?) agreed about that. Let's do it!

currently multiplying by a negative number, nan, -nan, infinite, or -infinite yields null -- why not an error, or empty? (well, empty would be harder to arrange here, so let's not go there)

Based on the results (i.e. jq), I think @stedolan sought to avoid raising errors, preferably by giving some meaning to otherwise meaningless expressions (this would help explain the liberal doses of polymorphism), or preferring null to empty (illustrated by index/1).

In the present case, it probably doesn't make much practical difference, except for backwards compatibility. Ergo, I'd stick with null for now.

But let's not make the perfect be the enemy of the good.

@nicowilliams
Copy link
Contributor

Based on the results (i.e. jq), I think @stedolan sought to avoid raising errors, preferably by giving some meaning to otherwise meaningless expressions (this would help explain the liberal doses of polymorphism), or preferring null to empty (illustrated by index/1).

Well, that was before we had a way to catch errors, but sure.

@itchyny itchyny force-pushed the fix-string-multiplication-regression branch from 646e859 to abbc482 Compare July 9, 2023 03:18
@itchyny itchyny force-pushed the fix-string-multiplication-regression branch from abbc482 to 075161d Compare July 9, 2023 03:19
@itchyny
Copy link
Contributor Author

itchyny commented Jul 9, 2023

Thanks, I fixed to emit an empty string when repeating a string by 0 (and less than 1). The logic here can be optimized by copying the written string to grow twice and twice, but could lead to another bug so let's do later.

@nicowilliams nicowilliams merged commit cac3ea3 into jqlang:master Jul 9, 2023
@nicowilliams
Copy link
Contributor

Thanks!

@pkoppstein
Copy link
Contributor

@nicowilliams wrote:

then we could have jq::v1_6, jq::v1_7, etc., and we could change builtins' behaviors in backwards incompatible ways.

One feature (or per @itchyny, either a misfeature or a non-feature) of jq is that jq allows :: prefixes even in the absence of modules. E.g.:

def v1_6::abs: fabs;
def v1_7::abs: if . < 0 then - . else . end;

This seems to be somewhat relevant here, as I'll illustrate in the case of walk, which has a (feeble) implementation in jq 1.6, and for which the proposed improvement for jq 1.7 (see (#2611) entails a backwards-compatibility issue.

Let's suppose that we want to make it easy for someone using jq 1.7 to invoke (or have their existing code use) the 1.6 version. Then we could retain the 1.6 version by adding the jq1_6:: prefix, and point out that users can:

  1. use jq1_6::walk; and/or

2 (a). add def walk: jq1_6::walk(f); early enough to any code that assumes the old implementation; or
2 (b). add the two lines:

def walk: jq1_6::walk(f);
def jq1_7::walk(f): walk(f);

This last allows users to have their cake (existing code does not need to be modified) and eat it (use the new walk at the same time, too).

One important question, though, remains: Is this ability to use :: prefixes in this way a bug, a feature, or something else?

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

Successfully merging this pull request may close these issues.

4 participants