Wrong type specification inside ssh_client_channel

[Krapaince]

Describe the bug
It seems like there is a type mismatch between the State of the init callback and the CbInitArg of the start_link function in the ssh_client_channel module.

The init callback defines the received Args as a term()

otp/lib/ssh/src/ssh_client_channel.erl

Lines 76 to 78 in 1621565

	-callback init(Args :: term()) ->
	{ok, State :: term()} | {ok, State :: term(), timeout() | hibernate} |
	{stop, Reason :: term()} | ignore.

whereas the CbInitArg type from the start_link function is [term()]

otp/lib/ssh/src/ssh_client_channel.erl

Lines 308 to 315 in 1621565

	-spec start_link(SshConnection, ChannelId, ChannelCb, CbInitArgs) ->
	{ok, ChannelRef} | {error, Reason :: term()}
	when
	SshConnection :: ssh:connection_ref(),
	ChannelId :: ssh:channel_id(),
	ChannelCb :: atom(),
	CbInitArgs :: [term()],
	ChannelRef :: pid().

After looking around the start function also defines CbInitArg as [term()]

otp/lib/ssh/src/ssh_client_channel.erl

Lines 282 to 289 in 1621565

	-spec start(SshConnection, ChannelId, ChannelCb, CbInitArgs) ->
	{ok, ChannelRef} | {error, Reason :: term()}
	when
	SshConnection :: ssh:connection_ref(),
	ChannelId :: ssh:channel_id(),
	ChannelCb :: atom(),
	CbInitArgs :: [term()],
	ChannelRef :: pid().

This makes Dialyzer yield a warning when using :ssh_client_channel.start_link with something else then a list of term. For instance when passing am empty map:

The function call will not succeed.

:ssh_client_channel.start_link(
  _ssh_connection :: any(),
  _channel_id :: any(),
  Ssh.ClientChannel,
  _init_arg :: %{}
)

breaks the contract
(sshConnection, channelId, channelCb, cbInitArgs) ::
  {:ok, :ChannelRef} | {:error, Reason :: term()}
when sshConnection: :ssh.connection_ref(),
     channelId: :ssh.channel_id(),
     channelCb: atom(),
     cbInitArgs: [term()],
     channelRef: pid()

To Reproduce
Call :ssh_client_channel.start_link/4 with a map

{:ok, connection_ref} = :ssh.connect('example.com', 22, [user: 'user'], 5000)
{:ok, channel_id} = :ssh_connection.session_channel(connection_ref, 5000)
:ssh_client_channel.start_link(connection_ref, channel_id,  :placeholder_mod, #{})

Expected behavior
I think the type of CbInitArgs should be term() instead of [term()].

Affected versions
Since it was introduced in OTP 21.

[MarkoMin]

I think that CbInitArgs must be a list because of:

otp/lib/ssh/src/ssh_client_channel.erl

Line 420 in 1621565

proplists:get_value(init_args, Options) ++ [Exec]

That said, Args in init/1 callback should be [term()] instead of term(). Current specs are not wrong, they're just not as specific as they could be. I think it's worth changing it for the sake of clarity.

[Krapaince]

Exec is only given by start/5 and start_link/5 which aren't part of the public API (doc false)

otp/lib/ssh/src/ssh_client_channel.erl

Lines 293 to 300 in 1621565

	-doc false.
	start(ConnectionManager, ChannelId, CallBack, CbInitArgs, Exec) ->
	Options = [{channel_cb, CallBack},
	{channel_id, ChannelId},
	{init_args, CbInitArgs},
	{cm, ConnectionManager},
	{exec, Exec}],
	gen_server:start(?MODULE, [Options], []).

otp/lib/ssh/src/ssh_client_channel.erl

Lines 319 to 326 in 1621565

	-doc false.
	start_link(ConnectionManager, ChannelId, CallBack, CbInitArgs, Exec) ->
	Options = [{channel_cb, CallBack},
	{channel_id, ChannelId},
	{init_args, CbInitArgs},
	{cm, ConnectionManager},
	{exec, Exec}],
	gen_server:start_link(?MODULE, [Options], []).

Although start/4 and start_link/4 both call their /5 counterparts, Exec is set to undefined and its value is later discarded by channel_cb_init_args

otp/lib/ssh/src/ssh_client_channel.erl

Lines 415 to 421 in 1621565

	channel_cb_init_args(Options) ->
	case proplists:get_value(exec, Options) of
	undefined ->
	proplists:get_value(init_args, Options);
	Exec ->
	proplists:get_value(init_args, Options) ++ [Exec]
	end.

I think this is something inherited from the old ssh_channel module where the start/5 and start_link/5 functions used to be exposed publicly

start(ConnectionManager, ChannelId, CallBack, CbInitArgs, Exec) ->

c2ca477#diff-1b29a193cb5ff21ccc00777e3706e6cbb723528e21140e58171c3e3e03877b5cL127

start_link(ConnectionManager, ChannelId, CallBack, CbInitArgs, Exec) ->

c2ca477#diff-1b29a193cb5ff21ccc00777e3706e6cbb723528e21140e58171c3e3e03877b5cL138

The [term()] seems to be there to handle backward compatibility with the old ssh_channel module. However it doesn't seem to be needed. If we look only to the ssh_client_channel on it's own as Exec isn't something that users are aware of or at least should be by the documentation.
Therefore I think CbInitArgs could be only term() instead of [term()].

[MarkoMin]

That would be legit if we remove exec first. We should probably put a deprecation warning to ssh_client_channel:start(_link)/5, ssh_daemon_channel:start_link/5 and ssh_server_channel:start_link/5. Not being documented is not enough to ensure backward compatibility. After that could remove exec from client channel module and then change CbInitArgs to term().

In documentation of this change we have to make sure that this exec option is separated from exec which is passed into ssh:daemon/1,2,3 which is documented and exposed.