misleading warning message about Range.new/3 in 1.18-rc.0

[woylie]

Elixir and Erlang/OTP versions

Erlang/OTP 27 [erts-15.1.2] [source] [64-bit] [smp:12:12] [ds:12:12:10] [async-threads:1] [jit]

Elixir 1.18.0-rc.0 (compiled with Erlang/OTP 27)

Operating system

macOS

Current behavior

After upgrading from 1.17.3 to 1.18.0-rc.0, I'm seeing this warning message:

.warning: Range.new/2 has a default step of -1, please call Range.new/3 explicitly passing the step of -1 instead
  (flop_phoenix 0.23.1) lib/flop_phoenix.ex:502: Flop.Phoenix."page_links (overridable 1)"/1
  (phoenix_live_view 1.0.0) lib/phoenix_live_view/tag_engine.ex:96: Phoenix.LiveView.TagEngine.component/3
  (flop_phoenix 0.23.1) lib/flop_phoenix.ex:477: anonymous fn/2 in Flop.Phoenix."pagination (overridable 1)"/1
  (phoenix_live_view 1.0.0) lib/phoenix_live_view/engine.ex:149: Phoenix.HTML.Safe.Phoenix.LiveView.Rendered.to_iodata/1
  (phoenix_live_view 1.0.0) lib/phoenix_live_view/engine.ex:165: Phoenix.HTML.Safe.Phoenix.LiveView.Rendered.to_iodata/3
  (phoenix_html 4.1.1) lib/phoenix_html.ex:134: Phoenix.HTML.html_escape/1
  (phoenix_live_view 1.0.0) lib/phoenix_live_view/test/live_view_test.ex:518: Phoenix.LiveViewTest.rendered_to_string/1
  (flop_phoenix 0.23.1) test/support/test_helpers.ex:23: Flop.Phoenix.TestHelpers.parse_heex/1
  test/flop_phoenix_test.exs:589: Flop.PhoenixTest."test pagination/1 doesn't render pagination links if set to hide when passing event"/1
  (ex_unit 1.18.0-rc.0) lib/ex_unit/runner.ex:511: ExUnit.Runner.exec_test/2
  (stdlib 6.1.2) timer.erl:590: :timer.tc/2
  (ex_unit 1.18.0-rc.0) lib/ex_unit/runner.ex:433: anonymous fn/6 in ExUnit.Runner.spawn_test_monitor/4

This is occurring on the main branch of Flop Phoenix.

git clone git@github.com:woylie/flop_phoenix.git
cd flop_phoenix
git checkout f0d145dbef77c0e41b3cde6ce3e84d2e3e6f6e26
mix deps.get
mix test

The referenced line 502 contains a call to the library function get_page_link_range (a range is matched on one line prior). Range.new/2 is not called.

    range =
      first..last//_ =
      Pagination.get_page_link_range(
        meta.current_page,
        max_pages,
        meta.total_pages
      )

The called function builds ranges like this:

  def get_page_link_range(current_page, max_pages, total_pages) do
    # number of additional pages to show before or after current page
    additional = ceil(max_pages / 2)

    cond do
      max_pages >= total_pages ->
        1..total_pages

      current_page + additional > total_pages ->
        (total_pages - max_pages + 1)..total_pages

      true ->
        first = max(current_page - additional + 1, 1)
        last = min(first + max_pages - 1, total_pages)
        first..last
    end
  end

The warning disappears after changing the code in that function to 1..total_pages//1 etc.

Expected behavior

I'd expect the warning message to a) point to the file and line that causes the issue, b) explain the issue using the range syntax instead of mentioning Range.new, and c) not tell me to pass -1 although no negative range is used.

[sabiwara]

Hi @woylie, thank you for the detailed issue 💜

a) a..b expands to Range.new(a, b) but strips the location, so it's not available in the stacktrace - this should be fixed indeed (edit: actually it's TCO, see below)
b) because the range syntax eventually gets expanded to the function call, I'm not sure if we could/should change this
c) good point, we should probably mention the step 1 fix as well

[sabiwara]

Diving more into a), this seems actually due to a limit of Process.info(self(), :current_stacktrace) which doesn't contain
lib/flop_phoenix/pagination.ex:70: Flop.Phoenix.Pagination.get_page_link_range/3, most likely due to TCO.
Each get_page_link_range finishes by a call to Range.new/2, so it just gets popped from the stacktrace.

If I replace first..last by IO.inspect(first..last) for example on line 70, the stacktrace is complete.
I'm not sure how/if this could be fixed.

[sabiwara]

Regarding c), the message has actually already been improved in #14062 (will be part of the next rc), @woylie do you think it is clear enough?

[woylie]

Regarding c), the message has actually already been improved in #14062 (will be part of the next rc), @woylie do you think it is clear enough?

That's certainly better. Do you think it would make sense to account for the possibility that first..last was used if this cannot be detected? Something like: Range.new/2 and first..last default to a step of -1 when last < first, please use Range.new/3 with step -1 or first..last//-1 or Range.new/2 and first..last default to a step of -1 when last < first, please use Range.new(first, last, -1) or first..last//-1

[sabiwara]

@woylie I think it's a good idea. Perhaps we should also mention the fact that 1 should be passed if it was non intentional, but I fear it might become too verbose and should probably be split in several sentences.

[woylie]

"Range.new/2 and first..last default to a step of -1 when last < first. Use Range.new(first, last, -1) or first..last//-1, or pass 1 if that was your intention"?

(I'll open a PR)

[josevalim]

The issue with a) is tail call optimization. I will wrap it in a no-op call for now but only for v1.18 so we don't pay a long term cost to avoid tail call.

[josevalim]

All fixed, thank you!

[pnezis]

@woylie @sabiwara regarding c)

• the warning is only emitted if last < first, so even if you expect the opposite there might be some edge case that produces such a condition (the same happened to me). In your case if total_pages is 0 you would have such a range.
• if you set the step to 1 you change the current behaviour of your code possibly introducing a bug:

# now
iex> Enum.to_list(3..1//1)
[]

# before
iex> Enum.to_list(3..1)
[3, 2, 1]

In order to be safe and keep the current semantics you should step based on the values of first and last:

step = if last < first, do: -1, else: 1
first..last//step

• imho the , or pass 1 if that was your intention part from the warning message should be removed since it may cause subtle bugs.

[sabiwara]

imho the , or pass 1 if that was your intention part from the warning message should be removed since it may cause subtle bugs.

The subtle bug here is having the step unintentionally being inferred as negative, which is necessary for backwards compatibility but is the very reason for the warning.
e.g. imagine you write Enum.map(1..n, fn _ -> :rand.uniform() end) with the intent of generating n numbers, but for n=0 you're accidentally generating 2 (for [1, 0]).

Only by specifying the step explicitly to -1 or 1, perhaps conditionally as you suggested if you want to replicate the historical behavior, we can make sure the behavior is actually the one intended.