Unable to distinguish between bool and string in sprintf

[lcarva]

Short description

When using sprintf, markers like %T and %#v produce the same exact output for the string "true" and the boolean true. Both are reported as of type string.

Steps To Reproduce

https://play.openpolicyagent.org/p/5HXqBdDKoX

package play

boolean := sprintf("%T %#v", [true, true])
string := sprintf("%T %#v", ["true", "true"])
integer := sprintf("%T %#v", [1, 1])

Output:

{
    "boolean": "string \"true\"",
    "integer": "int 1",
    "string": "string \"true\""
}

Expected behavior

Like go, I would expect a marker like %T to evaluate to bool if a boolean value is provided.

Additional context

I'm writing a rule that expects a certain value to be the string "true" (I know... confusing). I want to be able to construct an error message that makes it clear to the user the value is not of type string.

[lcarva]

We could add a case here for ast.Boolean, e.g.:

case ast.Boolean:
  args[i] = bool(v)

That would make sprintf("hi %s %T", [false, false]) produce "hi %!s(bool=false) bool", which is technically correct, but it is definitely a breaking change. You would have to use %t (lower-case) instead of %s to format boolean values, which does produce the expected result, hi false bool.

Alternatively, we could do:

case ast.Boolean:
  args[i] = v

which would produce: hi false ast.Boolean

That is a bit messy since it leaks rego's internals.

[anderseknert]

That is a bit messy since it leaks rego's internals.

Indeed, and tbh I'm not sure leaking Go's internals is that much better. Passing sprintf calls straight back to the Go implementation was probably a mistake seen in hindsight, as it makes it more or less impossible to completely port this built-in function to other implementations (Wasm, IR implementations, or entire re-implementations) unless they too are written in Go.

A less leaky solution would be to use the type_name built-in function for the purpose of getting the name of a Rego value's type.

[srenatus]

Passing sprintf calls straight back to the Go implementation was probably a mistake seen in hindsight:

💯 See also #4771. It's probably impossible to fix this in a backwards-compatible manner, though. We'd need to start deny-listing certain formats first, I guess...

[lcarva]

Didn't know about type_name. That is sufficient for my use case. Thanks!

Would you be open to a PR that adds a blurb to the docs of sprintf to warn users about this pitfal and suggest using type_name instead of %T ? Not sure if there's a better place for it.

[anderseknert]

Sure! I think the table is too cramped for details on that, but if we could add a blurb below the docs on the string functions, similar to how there's one below the functions for time, that could then later also be expanded with more gotchas, that'd be very useful!

[srenatus]

Sounds like a good linter rule, too...

[lcarva]

@srenatus , just to clarify, do you mean in regal?