fix: correctly split quoted args (#909)

lib/jq.bzl

@@ -144,7 +144,7 @@ def jq(name, srcs, filter = None, filter_file = None, args = [], out = None, dat
 
  filter_file: File containing filter expression (alternative to `filter`)
  args: Additional args to pass to jq
- expand_args: Run bazel's location-expansion on the args.
+ expand_args: Run bazel's location and make variable expansion on the args.
  out: Name of the output json file; defaults to the rule name plus ".json"
  **kwargs: Other common named parameters such as `tags` or `visibility`
  """

lib/private/BUILD.bazel

@@ -160,6 +160,8 @@ bzl_library(
  srcs = ["jq.bzl"],
  visibility = ["//lib:__subpackages__"],
  deps = [
+ ":expand_variables",
+ ":strings",
  "//lib:stamping",
  ],
 )
@@ -182,6 +184,10 @@ bzl_library(
  name = "params_file",
  srcs = ["params_file.bzl"],
  visibility = ["//lib:__subpackages__"],
+ deps = [
+ ":expand_variables",
+ ":strings",
+ ],
 )
 
 bzl_library(
@@ -204,6 +210,7 @@ bzl_library(
  visibility = ["//lib:__subpackages__"],
  deps = [
  ":expand_variables",
+ ":strings",
  "//lib:stamping",
  "@bazel_skylib//lib:dicts",
  ],

lib/private/bats.bzl

@@ -41,7 +41,7 @@ def _bats_test_impl(ctx):
  for (key, value) in ctx.attr.env.items():
  envs.append(_ENV_SET.format(
  key = key,
- value = " ".join([expand_variables(ctx, exp, attribute_name = "env") for exp in ctx.expand_location(value, targets = ctx.attr.data).split(" ")]),
+ value = expand_variables(ctx, ctx.expand_location(value, targets = ctx.attr.data), attribute_name = "env"),
  ))
 
  # See https://www.msys2.org/wiki/Porting/:

lib/private/expand_template.bzl

@@ -2,15 +2,12 @@
 
 load("@bazel_skylib//lib:dicts.bzl", "dicts")
 load("//lib:stamping.bzl", "STAMP_ATTRS", "maybe_stamp")
-load(":expand_variables.bzl", _expand_variables = "expand_variables")
+load(":expand_variables.bzl", "expand_variables")
 
 def _expand_substitutions(ctx, output, substitutions):
  result = {}
  for k, v in substitutions.items():
- result[k] = " ".join([
- _expand_variables(ctx, e, outs = [output], attribute_name = "substitutions")
- for e in ctx.expand_location(v, targets = ctx.attr.data).split(" ")
- ])
+ result[k] = expand_variables(ctx, ctx.expand_location(v, targets = ctx.attr.data), outs = [output], attribute_name = "substitutions")
  return result
 
 def _expand_template_impl(ctx):

lib/private/jq.bzl

@@ -1,6 +1,8 @@
 """Implementation for jq rule"""
 
 load("//lib:stamping.bzl", "STAMP_ATTRS", "maybe_stamp")
+load(":expand_variables.bzl", "expand_variables")
+load(":strings.bzl", "split_args")
 
 _jq_attrs = dict({
  "srcs": attr.label_list(
@@ -22,20 +24,14 @@ _jq_attrs = dict({
  ),
 }, **STAMP_ATTRS)
 
-def _expand_locations(ctx, s):
- # `.split(" ")` is a work-around https://github.com/bazelbuild/bazel/issues/10309
- # TODO: If the string has intentional spaces or if one or more of the expanded file
- # locations has a space in the name, we will incorrectly split it into multiple arguments
- return ctx.expand_location(s, targets = ctx.attr.data).split(" ")
-
 def _jq_impl(ctx):
  jq_bin = ctx.toolchains["@aspect_bazel_lib//lib:jq_toolchain_type"].jqinfo.bin
 
  out = ctx.outputs.out or ctx.actions.declare_file(ctx.attr.name + ".json")
  if ctx.attr.expand_args:
  args = []
  for a in ctx.attr.args:
- args += _expand_locations(ctx, a)
+ args += split_args(expand_variables(ctx, ctx.expand_location(a, targets = ctx.attr.data), outs = [out]))
  else:
  args = ctx.attr.args
 
@@ -52,7 +48,7 @@ def _jq_impl(ctx):
  args = args + ["--null-input"]
 
  if ctx.attr.filter_file:
- args = args + ["--from-file '%s'" % ctx.file.filter_file.path]
+ args = args + ["--from-file", ctx.file.filter_file.path]
  inputs.append(ctx.file.filter_file)
 
  stamp = maybe_stamp(ctx)
@@ -76,9 +72,16 @@ def _jq_impl(ctx):
 
  args = args + ["--slurpfile", "STAMP", stamp_json.path]
 
+ # quote args that contain spaces
+ quoted_args = []
+ for a in args:
+ if " " in a:
+ a = "'{}'".format(a)
+ quoted_args.append(a)
+
  cmd = "{jq} {args} {filter} {sources} > {out}".format(
  jq = jq_bin.path,
- args = " ".join(args),
+ args = " ".join(quoted_args),
  filter = "'%s'" % ctx.attr.filter if ctx.attr.filter else "",
  sources = " ".join(["'%s'" % file.path for file in ctx.files.srcs]),
  out = out.path,

lib/private/params_file.bzl

@@ -1,5 +1,8 @@
 "params_file rule"
 
+load(":expand_variables.bzl", "expand_variables")
+load(":strings.bzl", "split_args")
+
 _ATTRS = {
  "args": attr.string_list(),
  "data": attr.label_list(allow_files = True),
@@ -11,12 +14,6 @@ _ATTRS = {
  "_windows_constraint": attr.label(default = "@platforms//os:windows"),
 }
 
-def _expand_locations(ctx, s):
- # `.split(" ")` is a work-around https://github.com/bazelbuild/bazel/issues/10309
- # TODO: If the string has intentional spaces or if one or more of the expanded file
- # locations has a space in the name, we will incorrectly split it into multiple arguments
- return ctx.expand_location(s, targets = ctx.attr.data).split(" ")
-
 def _params_file_impl(ctx):
  is_windows = ctx.target_platform_has_constraint(ctx.attr._windows_constraint[platform_common.ConstraintValueInfo])
 
@@ -29,12 +26,9 @@ def _params_file_impl(ctx):
 
  expanded_args = []
 
- # First expand predefined source/output path variables
+ # Expand predefined source/output path && predefined variables & custom variables
  for a in ctx.attr.args:
- expanded_args += _expand_locations(ctx, a)
-
- # Next expand predefined variables & custom variables
- expanded_args = [ctx.expand_make_variables("args", e, {}) for e in expanded_args]
+ expanded_args += split_args(expand_variables(ctx, ctx.expand_location(a, targets = ctx.attr.data), outs = [ctx.outputs.out]))
 
  # ctx.actions.write creates a FileWriteAction which uses UTF-8 encoding.
  ctx.actions.write(

lib/private/run_binary.bzl

@@ -17,6 +17,7 @@
 load("@bazel_skylib//lib:dicts.bzl", "dicts")
 load("//lib:stamping.bzl", "STAMP_ATTRS", "maybe_stamp")
 load(":expand_variables.bzl", "expand_variables")
+load(":strings.bzl", "split_args")
 
 def _run_binary_impl(ctx):
  args = ctx.actions.args()
@@ -46,15 +47,11 @@ Possible fixes:
  rule_kind = str(ctx.attr.tool.label),
  ))
 
- # `expand_locations(...).split(" ")` is a work-around https://github.com/bazelbuild/bazel/issues/10309
- # _expand_locations returns an array of args to support $(execpaths) expansions.
- # TODO: If the string has intentional spaces or if one or more of the expanded file
- # locations has a space in the name, we will incorrectly split it into multiple arguments
  for a in ctx.attr.args:
- args.add_all([expand_variables(ctx, e, outs = outputs) for e in ctx.expand_location(a, targets = ctx.attr.srcs).split(" ")])
+ args.add_all(split_args(expand_variables(ctx, ctx.expand_location(a, targets = ctx.attr.srcs), outs = outputs)))
  envs = {}
  for k, v in ctx.attr.env.items():
- envs[k] = " ".join([expand_variables(ctx, e, outs = outputs, attribute_name = "env") for e in ctx.expand_location(v, targets = ctx.attr.srcs).split(" ")])
+ envs[k] = expand_variables(ctx, ctx.expand_location(v, targets = ctx.attr.srcs), outs = outputs, attribute_name = "env")
 
  stamp = maybe_stamp(ctx)
  if stamp:

lib/private/strings.bzl

@@ -583,3 +583,73 @@ def hex(number):
  hex_string = "0"
 
  return "{}0x{}".format("-" if is_signed else "", hex_string)
+
+def split_args(s):
+ """Split a string into a list space separated arguments
+
+ Unlike the naive `.split(" ")`, this function takes quoted strings
+ and escapes into account.
+
+ Args:
+ s: input string
+
+ Returns:
+ list of strings with each an argument found in the input string
+ """
+ args = []
+ arg = ""
+ single_quote = False
+ double_quote = False
+ escape = False
+ for c in s.elems():
+ if c == "\\":
+ escape = True
+ continue
+ if escape:
+ # this is an escaped character
+ if c == " ":
+ # a dangling escape is not an escape, put the backslack back
+ arg = arg + "\\"
+ else:
+ escape = False
+ else:
+ # not an escaped character, look for quotes & spaces
+ if c == "'":
+ # single quote char
+ if double_quote:
+ # we're in a double quote so single quotes are just chars
+ pass
+ elif single_quote:
+ # end of single quote
+ single_quote = False
+ continue
+ else:
+ # start of single quote
+ single_quote = True
+ continue
+ elif c == "\"":
+ # double quote char
+ if single_quote:
+ # we're in a single quote so double quotes are just chars
+ pass
+ elif double_quote:
+ # end of double quote
+ double_quote = False
+ continue
+ else:
+ # start of double quote
+ double_quote = True
+ continue
+ if c == " ":
+ if not single_quote and not double_quote:
+ # splitting space
+ if arg != "":
+ args.append(arg)
+ arg = ""
+ continue
+ arg = arg + c
+
+ # final arg?
+ if arg != "":
+ args.append(arg)
+ return args

lib/strings.bzl

@@ -1,7 +1,8 @@
 "Utilities for strings"
 
-load("//lib/private:strings.bzl", _chr = "chr", _hex = "hex", _ord = "ord")
+load("//lib/private:strings.bzl", _chr = "chr", _hex = "hex", _ord = "ord", _split_args = "split_args")
 
 chr = _chr
 ord = _ord
 hex = _hex
+split_args = _split_args

lib/tests/strings_tests.bzl

@@ -2,7 +2,7 @@
 
 load("@bazel_skylib//lib:partial.bzl", "partial")
 load("@bazel_skylib//lib:unittest.bzl", "asserts", "unittest")
-load("//lib/private:strings.bzl", "chr", "hex", "ord")
+load("//lib/private:strings.bzl", "chr", "hex", "ord", "split_args")
 
 def _ord_test_impl(ctx):
  env = unittest.begin(ctx)
@@ -56,10 +56,38 @@ def _hex_test_impl(ctx):
 
 hex_test = unittest.make(_hex_test_impl)
 
+def _split_args_test_impl(ctx):
+ env = unittest.begin(ctx)
+
+ asserts.equals(env, ["a", "b", "c", "d"], split_args("a b c d"))
+
+ # sinle quotes
+ asserts.equals(env, ["a", "b c", "d"], split_args("a 'b c' d"))
+
+ # double quotes
+ asserts.equals(env, ["a", "b c", "d"], split_args("a \"b c\" d"))
+
+ # escaped single quotes
+ asserts.equals(env, ["a", "'b", "c'", "d"], split_args("a \\'b c\\' d"))
+
+ # escaped double quotes
+ asserts.equals(env, ["a", "\"b", "c\"", "d"], split_args("a \\\"b c\\\" d"))
+
+ # sinle quotes containing escaped quotes
+ asserts.equals(env, ["a", "b'\" c", "d"], split_args("a 'b\\'\\\" c' d"))
+
+ # double quotes containing escaped quotes
+ asserts.equals(env, ["a", "b'\" c", "d"], split_args("a \"b\\'\\\" c\" d"))
+
+ return unittest.end(env)
+
+split_args_test = unittest.make(_split_args_test_impl)
+
 def strings_test_suite():
  unittest.suite(
  "strings_tests",
  partial.make(ord_test, timeout = "short"),
  partial.make(chr_test, timeout = "short"),
  partial.make(hex_test, timeout = "short"),
+ partial.make(split_args_test, timeout = "short"),
  )