numberize - rewrite only positive leaves, add map value numberization

[lukaszsamson]

Addresses 1 and 2 from #15799
Extracted from #15800

AssistedBy: Claude Fable 5, GPT 5.6 Sol

[josevalim]

Now I am thinking... isn't our bdd_map inherently wrong because it may rewrite the leafs which means they have a different ordering? I am thinking everything needs to be written as in this formula... which would also make numberize too expensive...

[lukaszsamson]

I recall one of AI reviews flagged the order not being preserved as an issue. I wasn’t able to produce code where it would surface as an evident bug. I dismissed the proposed fix with BDD rebuilds everywhere as a bad tradeoff

[josevalim]

@lukaszsamson I am thinking a better solution here would be have a field in the context called numberize. So when we assemble the type here:

  defp custom_compare(name, arg, literal, expected, expr, stack, context, of_fun) do
    case booleaness(expected) do
      booleaness when booleaness in [:maybe_both, :none] ->
        compare(name, arg, literal, false, expr, stack, context, of_fun)

      {boolean, _maybe_or_always} ->
        {type, context} = of_fun.(literal, term(), expr, stack, context) # <=======

We set this value in the stack and we interpret all integers/floats in the literal as numbers. This makes it correct by construction and we can remove numberize.

We still have to address 3 from 15799 though... perhaps we have a version of disjoint that ignores numbers... but that may be complicated.

[lukaszsamson]

@lukaszsamson I am thinking a better solution here would be have a field in the context called numberize. So when we assemble the type here:

  defp custom_compare(name, arg, literal, expected, expr, stack, context, of_fun) do
    case booleaness(expected) do
      booleaness when booleaness in [:maybe_both, :none] ->
        compare(name, arg, literal, false, expr, stack, context, of_fun)

      {boolean, _maybe_or_always} ->
        {type, context} = of_fun.(literal, term(), expr, stack, context) # <=======

We set this value in the stack and we interpret all integers/floats in the literal as numbers. This makes it correct by construction and we can remove numberize.

We still have to address 3 from 15799 though... perhaps we have a version of disjoint that ignores numbers... but that may be complicated.

I tried that, see lukaszsamson/elixir@ls-numberize-map-domains...ls-numberize-flag-spike. It did not make numberize/1 go. It is still needed by Pattern.of_pattern_tree

[josevalim]

I tried that, see lukaszsamson/elixir@ls-numberize-map-domains...ls-numberize-flag-spike. It did not make numberize/1 go. It is still needed by Pattern.of_pattern_tree

We can apply the same technique in of_pattern_tree, no? If so, I'd say that PR is the way to go. The fact that we can track map keys and binaries correctly is a positive.

[josevalim]

We can apply the same technique in of_pattern_tree, no? If so, I'd say that PR is the way to go. The fact that we can track map keys and binaries correctly is a positive.

@lukaszsamson can you tackle the pending item above and send a pull request as well? Thanks!

[josevalim]

          name in [:==, :"/="] ->
           of_fun.(literal, term(), expr, %{stack | numberize: true}, context)

Instead of calling of_fun again, we should probaly reorganize the function to change the stack upfront (by checking the polarity and the name).

[lukaszsamson]

We can apply the same technique in of_pattern_tree, no? If so, I'd say that PR is the way to go. The fact that we can track map keys and binaries correctly is a positive.

@lukaszsamson can you tackle the pending item above and send a pull request as well? Thanks!

I tried that in https://github.com/lukaszsamson/elixir/tree/ls-numberize-flag-spike-v2 compare but it makes the two bugs this PR fixes come back. It does not cover the variable to variable path, that branch is only reached when neither operand is quoted literal

[lukaszsamson]

          name in [:==, :"/="] ->
           of_fun.(literal, term(), expr, %{stack | numberize: true}, context)

Instead of calling of_fun again, we should probaly reorganize the function to change the stack upfront (by checking the polarity and the name).

Yes. That would work. The only consequence is return_compare uses the type for mismatch warning so it would print number() instead of integer()

[josevalim]

The unit tests on numberize are enough here!

Suggested change

	test "does not discard equal values when narrowing against negated types" do
	# `other` carries a negation (`... and not {float()}`) from the clause
	# subtraction. Widening integers/floats inside that negation would wrongly
	# remove `{1}`, even though `{1} == {1.0}` at runtime.
	assert typecheck!(
	[v, q],
	(
	other =
	case v do
	{a} when is_float(a) -> {:float, a}
	other -> other
	end
	w = if q, do: {1}, else: {1.5}
	case w do
	x when x == other -> {:matched, x}
	_ -> :nomatch
	end
	)
	)
	|> to_quoted_string() ==
	"dynamic({:matched, {float() or integer()}}) or :nomatch"
	end
	test "does not discard equal values when narrowing against domain keys" do
	# `==` coerces map values, so narrowing `x` from `x == y` must widen the
	# `integer() => integer()` domain value to `integer() => number()`.
	assert typecheck!(
	(
	y = %{1 => 1}
	w = %{1 => 1.0}
	case w do
	x when x == y and map_size(x) == 1 -> {:eq, x}
	_ -> :ne
	end
	)
	)
	|> to_quoted_string() ==
	":ne or {:eq, %{integer() => float()} and not empty_map()}"
	end

[josevalim]

Suggested change

	case fun.(leaf) do
	^leaf -> bdd_node_new(leaf, left, union, right)
	new_leaf when right == :bdd_bot -> bdd_node_new(new_leaf, left, union, :bdd_bot)
	new_leaf -> bdd_union(bdd_intersection(new_leaf, left), bdd_union(union, right))
	end
	bdd_intersection(new_leaf, left)
	|> bdd_union(union)
	|> bdd_union(bdd_difference(right, leaf))

[josevalim]

This above is more likely to be the correct formula. We rewrite only the leaf in positive position.