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.

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

uri_string:percent_decode has incorrect return spec #8755

Closed
gabrieltholsgard opened this issue Aug 27, 2024 · 2 comments
Closed

uri_string:percent_decode has incorrect return spec #8755

gabrieltholsgard opened this issue Aug 27, 2024 · 2 comments
Assignees
Labels
bug Issue is reported as a bug team:PS Assigned to OTP team PS

Comments

@gabrieltholsgard
Copy link

Description
The function spec of uri_string:percent_decode/1 is not complete when it comes to the return values.
It is possible to get a return value of format {error, atom(), term()} as spec:ed by uri_string:raw_decode/1, which is called directly when a binary() or list() is passed as value to uri_string:percent_decode/1.
When handling this possible output Dialyzer will complain about not being able to match the output.

To Reproduce

In an erlang shell:

12> uri_string:percent_decode("foo%f2bar").
{error,invalid_utf8,<<"fooòbar">>}
-module(foo).

-export([bar/1]).

-spec bar(PercentText :: list() | binary()) -> {error, atom()} | list() | binary().
bar(PercentText) ->
   case uri_string:percent_decode(PercentText) of
      {error, Reason, _} -> {error, Reason};
      Result -> Result
   end.

When running dialyzer on module:

foo.erl:8:7: The pattern 
          {'error', Reason, _} can never match the type 
          binary() |
          maybe_improper_list(binary() |
                              maybe_improper_list(any(), binary() | []) |
                              byte(),
                              binary() | []) |
          {'error', {'invalid', {atom(), {_, _}}}} |
          #{'fragment' =>
                binary() |
                maybe_improper_list(binary() |
                                    maybe_improper_list(any(),
                                                        binary() | []) |
                                    char(),
                                    binary() | []),
            'host' =>
                binary() |
                maybe_improper_list(binary() |
                                    maybe_improper_list(any(),
                                                        binary() | []) |
                                    char(),
                                    binary() | []),
            'path' =>
                binary() |
                maybe_improper_list(binary() |
                                    maybe_improper_list(any(),
                                                        binary() | []) |
                                    char(),
                                    binary() | []),
            'port' => 'undefined' | non_neg_integer(),
            'query' =>
                binary() |
                maybe_improper_list(binary() |
                                    maybe_improper_list(any(),
                                                        binary() | []) |
                                    char(),
                                    binary() | []),
            'scheme' =>
                binary() |
                maybe_improper_list(binary() |
                                    maybe_improper_list(any(),
                                                        binary() | []) |
                                    char(),
                                    binary() | []),
            'userinfo' =>
                binary() |
                maybe_improper_list(binary() |
                                    maybe_improper_list(any(),
                                                        binary() | []) |
                                    char(),
                                    binary() | [])}

Expected behavior
I would expect that the potential return value {error, atom(), term()} is handled in some way, be it that percent_decode handles it and return a uniform error structure or that a secondary error structure is returned and the spec reflecting that, such that dialyzer stops complaining.
It can also be worth mentioning that same percent_decode may also throw an error if the input is binary.
For instance:

1> uri_string:percent_decode(<<"foo%f2bar">>).
** exception throw: {error,invalid_utf8,<<"fooòbar">>}
     in function  uri_string:check_utf8/1 (uri_string.erl, line 1575)

Affected versions
Erlang/OTP 26 [erts-14.2.5]
Running this version and have only checked on this version.

@gabrieltholsgard gabrieltholsgard added the bug Issue is reported as a bug label Aug 27, 2024
@IngelaAndin IngelaAndin added the team:PS Assigned to OTP team PS label Aug 28, 2024
@u3s
Copy link
Contributor

u3s commented Sep 11, 2024

thanks for reporting the issue. your observations are correct. we will plan fixing it.

@Whaileee
Copy link
Contributor

Whaileee commented Dec 3, 2024

The change is merged without a PR, so I'll close the issue. Thanks for noticing.
Best regards,
Konrad

@Whaileee Whaileee closed this as completed Dec 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue is reported as a bug team:PS Assigned to OTP team PS
Projects
None yet
Development

No branches or pull requests

4 participants