Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose getting process dictionary value for specific key in Process.info/2 #13505

Merged
merged 4 commits into from
Apr 22, 2024
+6 −2
Merged

Expose getting process dictionary value for specific key in Process.info/2 #13505

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8e53a61
Expose getting process dictionary value for specific key in Process.i…
michallepicki Apr 22, 2024
7c9e2dc
Revert the change to info/1 spec
michallepicki Apr 22, 2024
16ae690
Remove guard from Process.info/2
michallepicki Apr 22, 2024
0354937
Improve Process.info/2 spec by defining it for two non overlaping cla…
michallepicki Apr 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions lib/elixir/lib/process.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -826,6 +826,9 @@ defmodule Process do
@spec flag(pid, :save_calls, 0..10000) :: 0..10000
defdelegate flag(pid, flag, value), to: :erlang, as: :process_flag

@type process_info_item :: atom | {:dictionary, term}
@type process_info_result_item :: {process_info_item, term}

@doc """
Returns information about the process identified by `pid`, or returns `nil` if the process
is not alive.
Expand All @@ -834,7 +837,7 @@ defmodule Process do

See `:erlang.process_info/1` for more information.
"""
@spec info(pid) :: keyword | nil
@spec info(pid) :: [process_info_result_item] | nil
Copy link
Contributor

@sabiwara sabiwara Apr 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] Maybe this one is too imprecise here? Since the key will always be an atom, keyword might be more accurate.

Or should we refer :erlang.process_info_result_item() which is a bit more precise? But dialyzer would generalize to atom anyway.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this function can actually return individual dictionary keys? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] Maybe this one is too imprecise here? Since the key will always be an atom, keyword might be more accurate.

I don't think this function can actually return individual dictionary keys? 🤔

Good points about this function clause, I will revert this change to info/1 spec

Or should we refer :erlang.process_info_result_item() which is a bit more precise? But dialyzer would generalize to atom anyway.

Unfortunately the type is not exported, Dialyzer says:

lib/process.ex:840:30: Unknown type erlang:process_info_result_item/0

def info(pid) do
nilify(:erlang.process_info(pid))
end
Expand All @@ -845,7 +848,8 @@ defmodule Process do

See `:erlang.process_info/2` for more information.
"""
@spec info(pid, atom | [atom]) :: {atom, term} | [{atom, term}] | nil
@spec info(pid, process_info_item | [process_info_item]) ::
process_info_result_item | [process_info_result_item] | nil
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
@spec info(pid, process_info_item | [process_info_item]) ::
process_info_result_item | [process_info_result_item] | nil
@spec info(pid, process_info_item) :: process_info_result_item | nil
@spec info(pid, [process_info_item]) :: [process_info_result_item] | nil

I think this could work as well and be better for documentation (althoguh maybe Dialyzer would unify these types internally?)

Copy link
Contributor Author

@michallepicki michallepicki Apr 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pleasantly surprised to report that this improves the spec for Dialyzer as well:
for {:should_warn, _} = Process.info(self(), [:some_atom]):

The pattern
          {'should_warn', _} can never match the type
          'nil' | [{atom() | {'dictionary', _}, _}]

and for [{:should_warn, _}] = Process.info(self(), :some_atom)

The pattern
          [{'should_warn', _}] can never match the type
          'nil' | {atom() | {'dictionary', _}, _}

def info(pid, spec)

def info(pid, :registered_name) do
Expand All @@ -856,6 +860,10 @@ defmodule Process do
end
end

def info(pid, {:dictionary, key}) do
nilify(:erlang.process_info(pid, {:dictionary, key}))
end

def info(pid, spec) when is_atom(spec) or is_list(spec) do
nilify(:erlang.process_info(pid, spec))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would actually remove the specific clauses. Now that Erlang/OTP has good error messages, they will probably be more decent than function clause error?

Suggested change
def info(pid, {:dictionary, key}) do
nilify(:erlang.process_info(pid, {:dictionary, key}))
end
def info(pid, spec) when is_atom(spec) or is_list(spec) do
nilify(:erlang.process_info(pid, spec))
def info(pid, spec) do
nilify(:erlang.process_info(pid, spec))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Erlang 26.2.2 the error looks like:

iex(1)> Process.info(self(), :unknown)
** (ArgumentError) errors were found at the given arguments:

  * 2nd argument: invalid item or item list

    :erlang.process_info(#PID<0.114.0>, :unknown)
    (elixir 1.17.0-dev) lib/process.ex:864: Process.info/2
    iex:1: (file)

end
Expand Down