search

microsoft / Power-Fx

Power Fx low-code programming language
MIT License
3.21k stars 327 forks source link

Boolean(true) shouldn't be invalid #489

Closed susosuso closed 1 year ago

susosuso commented 2 years ago

Type conversion typically allow a value to convert to its current type. Looks like Power FX allows that for Value and Text, but not Boolean:

Value(10) returns 10 Text("hello") returns "hello"

But Boolean(true) is invalid.

This feels like a bug to me. Thoughts?

MikeStall commented 2 years ago

Curious - what sort of scenario does this come up?

susosuso commented 2 years ago

it's just more of an inconsistency to me. Say if evaluator takes in two parameters. Param A is untyped object, and param B is strongly typed. For ease of mind, user might just explicit convert all the values in the expression, even though only param A is required to do so. But Boolean(A.some_bool) will succeed but Boolean(B.some_bool) will fail.

MikeStall commented 2 years ago

To fix:

  1. Expand Boolean function to accept bool type. See https://github.com/microsoft/Power-Fx/blob/main/src/libraries/Microsoft.PowerFx.Core/Texl/Builtins/Boolean.cs
  2. Consider Tabular overloads.
  3. Ideally purely a fix in the IR - for Boolean(x) when x is a bool, just emit x.

JS will need a separate fix since 1+2 will impact its resolution, and it's also not on IR yet (so won't pick up fix from 3) .

MikeStall commented 2 years ago

We discussed and we'd like to fix this.

MikeStall commented 1 year ago

Load balancing....

MikeStall commented 1 year ago

Marking Breaking Internal since we will need a corresponding PAClient update to handle this. (if it was an IR change, we wouldn't...)

Ideally, we could do this as an IR change, but we don't have the contracts in place. See #808 -that's just parameters, whereas this is the entire method. Table overload - see #874