search

jeremyjh / dialyxir

Mix tasks to simplify use of Dialyzer in Elixir projects.
Apache License 2.0
1.69k stars 141 forks source link

Issue with pot OTP library #407

Closed liamwhite closed 3 years ago

liamwhite commented 4 years ago

Environment

Current behavior

Given the following code which uses the pot OTP library:

:pot.valid_totp("", "", [window: 1])

Dialyxir generates a nonsense warning [1] [2] [3]:

The function call will not succeed.

:pot.valid_totp(_token :: binary(), binary(), [{:window, 1}])

will never return since it differs in arguments with
positions 3rd from the success typing arguments:

(binary(), any(), [{:token_length, pos_integer()}])

This seems to be an issue with dialyxir, because running dialyzer on the tests in the pot repository, such as this code, does not produce any warnings except specdiffs.

validating_past_future_totp_too_small_window(Secret) ->
    IntervalOpts = [{timestamp, os:timestamp()}],
    N = 5,
    [?_assertNot(pot:valid_totp(pot:totp(Secret, [{addwindow, AW} | IntervalOpts]),
                                Secret,
                                [{window, W} | IntervalOpts]))
     || W <- lists:seq(0, N), AW <- lists:seq(-N, N), W < abs(AW)].

Expected behavior

No warnings should be generated.

jeremyjh commented 4 years ago

Dialyxir does not produce warnings; that warning is coming from Dialyzer.

kenny-evitt commented 3 years ago

@jeremyjh Any ideas why Dialyzer (not Dialyxir) is producing that warning? I ran into it as well and it doesn't make sense given the types:

...

-type secret() :: binary().
-type token() :: binary().

-type token_option() :: {token_length, pos_integer()}.

-type time_interval_option() :: {addwindow, integer()} |
                                {interval_length, pos_integer()} |
                                {timestamp, erlang:timestamp()}.

-type hotp_option() :: token_option() |
                       {digest_method, atom()}.

-type totp_option() :: hotp_option() | time_interval_option().

...

-type valid_totp_option() :: totp_option() |
                             {window, non_neg_integer()}.

...
-type valid_totp_options() :: [valid_totp_option()].

...

From my local copy of the relevant file that's the same as the current version in the upstream GitHub repo:

It seems like Dialyzer expects the options to always or only contain {:token_length, pos_integer()}.

kenny-evitt commented 3 years ago

Adding the :token_length option to the :pot.valid_totp/3 call resolves the warning:

    :pot.valid_totp(token, secret, token_length: 6, window: 1)

Weird!

kenny-evitt commented 3 years ago

@jeremyjh Is it sensible to report this upstream to Erlang/OTP?

jeremyjh commented 3 years ago

@kenny-evitt only if you can reproduce the problem in pure Erlang and can determine it isn't a problem in pot. If the types are correct the problem may be the implementation in pot, is the success typing of valid_totp actually the same as its spec ? (e.g. does dialyzer pass on pot itself )? If all the answers are yes then you'd want to develop the simplest example that can reproduce the problem and report it on Erlang bugs. Usually the bug is in the library, not in Erlang but we've seen it happen.

kenny-evitt commented 3 years ago

@jeremyjh Dialyzer passes on pot itself.

You didn't mention it, and I imagine it's rare too, but what would point to a problem with Elixir with respect to Dialyzer?

kenny-evitt commented 3 years ago

@jeremyjh I was able to reproduce the warning with a minimal Elixir (Mix) project:

The relevant (and only) code:

defmodule PotDialyzerWarningRepro do
  @moduledoc """
  Documentation for `PotDialyzerWarningRepro`.
  """

  def totp_valid?(token, secret) do
    :pot.valid_totp(token, secret, [{:window, 1}])
  end

end

I wasn't able to reproduce the same warning (or any warnings) with what seems to me like the functionally equivalent Erlang code in a separate minimal Erlang (Rebar3) project:

The relevant (and only) code:

-module(pot_dialyzer_warning_repro_erl).

-export([is_totp_valid/2]).

is_totp_valid(Token, Secret) ->
    pot:valid_totp(Token, Secret, [{window, 1}]).

Any ideas on what to look at or do next? Open an issue for pot? Ask on the Elixir forum or mailing list?

jeremyjh commented 3 years ago

@kenny-evitt The forums are the best place to get help on this, yes. A better minimal example would be one without pot. That would clarify exactly where in the definition or implementation the issue lies. When you say you can't reproduce the same error with the Erlang code, are you running dialyzer the same way in both cases? Can you reproduce it against the compiled Elixir beams by just running the dialyzer CLI?

kenny-evitt commented 3 years ago

@jeremyjh Thanks!

I created a post on Elixir Forum for this (and some other warnings):

A better minimal example would be one without pot. That would clarify exactly where in the definition or implementation the issue lies.

I'm not sure I understand what you're suggesting. Copy the pot code to the minimal repro repo? I'm a little confused because I'm not sure whether the error is because the pot code is a dependency, nor do I know, or even suspect, what in particular is causing the problem.

Having written the preceding though, I do think I might understand your reasons (and wisdom) for offering that suggestion. I'll try to replace the pot dependency in the repro code and test whether I can still repro the Dialyzer warnings.

When you say you can't reproduce the same error with the Erlang code, are you running dialyzer the same way in both cases? Can you reproduce it against the compiled Elixir beams by just running the dialyzer CLI?

No and I don't know yet how to do either of those things.

In the repro-Erlang code, I'm running rebar3 dialyzer in the root directory of the repo. For the Elixir code, I'm using Dialyxir via mix dialyzer.

How can I run Dialyzer the same way for both repro-repos?

Also, how can I run the Dialyzer CLI against the compiled Elixir BEAM files?

If those questions are 'too big' to easily answer, can you suggest search terms to hopefully point me in the right direction? I'll try to search anyways myself when I have some time.

jeremyjh commented 3 years ago

How can I run Dialyzer the same way for both repro-repos?

@kenny-evitt You can run dialyzer from the command line like so, just substitute the proper plt file name and target path.

$ dialyzer --plt _build/dev/dialyxir_erlang-23.1.1_elixir-1.10.1_deps-dev.plt _build/dev/lib/diatarget/ebin/*.beam
kenny-evitt commented 3 years ago

@jeremyjh Thanks!

I suspect I'm doing something wrong. Here's what I tried for both of the minimal-repro-repos.

I ran dialyzer --plt _build/dev/dialyxir_erlang-21.2.3_elixir-1.10.4_deps-dev.plt _build/dev/lib/dialyxir/ebin/*.beam for the Elixir repo:

✔ ~/@code/pot_dialyzer_warning_repro [master L|✔] 
(ins)12:01 $ dialyzer --plt _build/dev/dialyxir_erlang-21.2.3_elixir-1.10.4_deps-dev.plt _build/dev/lib/dialyxir/ebin/*.beam
  Checking whether the PLT _build/dev/dialyxir_erlang-21.2.3_elixir-1.10.4_deps-dev.plt is up-to-date... yes
  Compiling some key modules to native code... done in 0m36.92s
  Proceeding with analysis...=ERROR REPORT==== 20-Nov-2020::12:04:02.882576 ===
Error in process <0.3632.0> with exit value:
{undef,
 [{elixir_erl,debug_info,
   [core_v1,'Elixir.Dialyxir.Dialyzer',
    ...
The full output:

``` ✔ ~/@code/pot_dialyzer_warning_repro [master L|✔] (ins)12:01 $ dialyzer --plt _build/dev/dialyxir_erlang-21.2.3_elixir-1.10.4_deps-dev.plt _build/dev/lib/dialyxir/ebin/*.beam Checking whether the PLT _build/dev/dialyxir_erlang-21.2.3_elixir-1.10.4_deps-dev.plt is up-to-date... yes Compiling some key modules to native code... done in 0m36.92s Proceeding with analysis...=ERROR REPORT==== 20-Nov-2020::12:04:02.882576 === Error in process <0.3632.0> with exit value: {undef, [{elixir_erl,debug_info, [core_v1,'Elixir.Dialyxir.Dialyzer', {elixir_v1, #{attributes => [],compile_opts => [], definitions => [{{warnings_msg,0}, defp, [{line,101}], [{[{line,101}], [],[], {{'.',[{line,101}],['Elixir.Dialyxir.Output',color]}, [{line,101}], [<<"done (warnings were emitted)">>,yellow]}}]}, {{success_msg,0}, defp, [{line,99}], [{[{line,99}], [],[], {{'.',[{line,99}],['Elixir.Dialyxir.Output',color]}, [{line,99}], [<<"done (passed successfully)">>,green]}}]}, {{dialyze,3}, def, [{defaults,2},{line,74}], [{[{line,74}], [{args,[{version,0},{line,74}],nil}, {runner,[{version,1},{line,74}],nil}, {filterer,[{version,2},{line,74}],nil}], [], {'case', [{line,75}], [{{'.',[{line,75}],[{runner,[{version,1},{line,75}],nil},run]}, [{line,75}], [{args,[{version,0},{line,75}],nil}, {filterer,[{version,2},{line,75}],nil}]}, [{do, [{'->', [{line,76}], [[{ok, {'{}', [{line,76}], [{time,[{version,3},{line,76}],nil}, [], {formatted_unnecessary_skips, [{version,4},{line,76}], nil}]}}], {'{}', [{line,77}], [ok,0, [{time,[{version,3},{line,77}],nil}, {formatted_unnecessary_skips, [{version,4},{line,77}], nil}, {success_msg,[{line,77}],[]}]]}]}, {'->', [{line,79}], [[{ok, {'{}', [{line,79}], [{time,[{version,5},{line,79}],nil}, {result,[{version,6},{line,79}],nil}, {formatted_unnecessary_skips, [{version,7},{line,79}], nil}]}}], {'__block__',[], [{'=', [{line,80}], [{warnings,[{version,9},{line,80}],nil}, {{'.',[{line,80}],['Elixir.Enum',map]}, [{line,80}], [{result,[{version,6},{line,80}],nil}, {fn, [{line,80}], [{'->', [{line,80}], [[{x1,[{version,8}],elixir_fn}], {{'.', [{line,80}], ['Elixir.Dialyxir.Output',color]}, [{line,80}], [{x1,[{version,8}],elixir_fn},red]}]}]}]}]}, {'{}', [{line,82}], [warn,2, {{'.',[{line,83}],[erlang,'++']}, [{line,83}], [[{time,[{version,5},{line,83}],nil}], {{'.',[{line,83}],[erlang,'++']}, [{line,83}], [{warnings,[{version,9},{line,83}],nil}, [{formatted_unnecessary_skips, [{version,7},{line,83}], nil}, {warnings_msg,[{line,83}],[]}]]}]}]}]}]}, {'->', [{line,85}], [[{warn, {'{}', [{line,85}], [{time,[{version,10},{line,85}],nil}, {result,[{version,11},{line,85}],nil}, {formatted_unnecessary_skips, [{version,12},{line,85}], nil}]}}], {'__block__',[], [{'=', [{line,86}], [{warnings,[{version,14},{line,86}],nil}, {{'.',[{line,86}],['Elixir.Enum',map]}, [{line,86}], [{result,[{version,11},{line,86}],nil}, {fn, [{line,86}], [{'->', [{line,86}], [[{x1,[{version,13}],elixir_fn}], {{'.', [{line,86}], ['Elixir.Dialyxir.Output',color]}, [{line,86}], [{x1,[{version,13}],elixir_fn},red]}]}]}]}]}, {'{}', [{line,88}], [warn,2, {{'.',[{line,89}],[erlang,'++']}, [{line,89}], [[{time,[{version,10},{line,89}],nil}], {{'.',[{line,89}],[erlang,'++']}, [{line,89}], [{warnings,[{version,14},{line,89}],nil}, [{formatted_unnecessary_skips, [{version,12},{line,89}], nil}, {warnings_msg,[{line,89}],[]}]]}]}]}]}]}, {'->', [{line,91}], [[{error, {{msg,[{version,15},{line,91}],nil}, {formatted_unnecessary_skips, [{version,16},{line,91}], nil}}}], {'{}', [{line,92}], [error,1, [{{'.',[{line,92}],['Elixir.Dialyxir.Output',color]}, [{line,92}], [{formatted_unnecessary_skips, [{version,16},{line,92}], nil}, red]}, {{'.',[{line,92}],['Elixir.Dialyxir.Output',color]}, [{line,92}], [{msg,[{version,15},{line,92}],nil},red]}]]}]}, {'->', [{line,94}], [[{error,{msg,[{version,17},{line,94}],nil}}], {'{}', [{line,95}], [error,1, [{{'.',[{line,95}],['Elixir.Dialyxir.Output',color]}, [{line,95}], [{msg,[{version,17},{line,95}],nil},red]}]]}]}]}]]}}]}, {{dialyze,2}, def, [{line,74}], [{[{line,74}], [{x0,[{generated,true},{version,0}],elixir_def}, {x1,[{generated,true},{version,1}],elixir_def}], [], {super, [{super,{def,dialyze}},{line,74}], [{x0,[{generated,true},{version,0}],elixir_def}, {x1,[{generated,true},{version,1}],elixir_def}, 'Elixir.Dialyxir.Project']}}]}, {{dialyze,1}, def, [{line,74}], [{[{line,74}], [{x0,[{generated,true},{version,0}],elixir_def}], [], {super, [{super,{def,dialyze}},{line,74}], [{x0,[{generated,true},{version,0}],elixir_def}, 'Elixir.Dialyxir.Dialyzer.Runner', 'Elixir.Dialyxir.Project']}}]}], deprecated => [], file => <<"/Users/kenny/@code/pot_dialyzer_warning_repro/deps/dialyxir/lib/dialyxir/dialyzer.ex">>, is_behaviour => false,line => 1,module => 'Elixir.Dialyxir.Dialyzer', relative_file => <<"lib/dialyxir/dialyzer.ex">>,unreachable => []}, []}, [no_copt,to_core,binary,return_errors,no_inline,strict_record_tests, strict_record_updates,dialyzer]], []}, {dialyzer_utils,get_core_from_beam,2, [{file,"dialyzer_utils.erl"},{line,116}]}]} dialyzer: Analysis failed with error: {undef, [{elixir_erl,debug_info, [core_v1,'Elixir.Dialyxir.Dialyzer', {elixir_v1, #{attributes => [],compile_opts => [], definitions => [{{warnings_msg,0}, defp, [{line,101}], [{[{line,101}], [],[], {{'.',[{line,101}],['Elixir.Dialyxir.Output',color]}, [{line,101}], [<<"done (warnings were emitted)">>,yellow]}}]}, {{success_msg,0}, defp, [{line,99}], [{[{line,99}], [],[], {{'.',[{line,99}],['Elixir.Dialyxir.Output',color]}, [{line,99}], [<<"done (passed successfully)">>,green]}}]}, {{dialyze,3}, def, [{defaults,2},{line,74}], [{[{line,74}], [{args,[{version,0},{line,74}],nil}, {runner,[{version,1},{line,74}],nil}, {filterer,[{version,2},{line,74}],nil}], [], {'case', [{line,75}], [{{'.',[{line,75}],[{runner,[{version,1},{line,75}],nil},run]}, [{line,75}], [{args,[{version,0},{line,75}],nil}, {filterer,[{version,2},{line,75}],nil}]}, [{do, [{'->', [{line,76}], [[{ok, {'{}', [{line,76}], [{time,[{version,3},{line,76}],nil}, [], {formatted_unnecessary_skips, [{version,4},{line,76}], nil}]}}], {'{}', [{line,77}], [ok,0, [{time,[{version,3},{line,77}],nil}, {formatted_unnecessary_skips, [{version,4},{line,77}], nil}, {success_msg,[{line,77}],[]}]]}]}, {'->', [{line,79}], [[{ok, {'{}', [{line,79}], [{time,[{version,5},{line,79}],nil}, {result,[{version,6},{line,79}],nil}, {formatted_unnecessary_skips, [{version,7},{line,79}], nil}]}}], {'__block__',[], [{'=', [{line,80}], [{warnings,[{version,9},{line,80}],nil}, {{'.',[{line,80}],['Elixir.Enum',map]}, [{line,80}], [{result,[{version,6},{line,80}],nil}, {fn, [{line,80}], [{'->', [{line,80}], [[{x1,[{version,8}],elixir_fn}], {{'.', [{line,80}], ['Elixir.Dialyxir.Output',color]}, [{line,80}], [{x1,[{version,8}],elixir_fn},red]}]}]}]}]}, {'{}', [{line,82}], [warn,2, {{'.',[{line,83}],[erlang,'++']}, [{line,83}], [[{time,[{version,5},{line,83}],nil}], {{'.',[{line,83}],[erlang,'++']}, [{line,83}], [{warnings,[{version,9},{line,83}],nil}, [{formatted_unnecessary_skips, [{version,7},{line,83}], nil}, {warnings_msg,[{line,83}],[]}]]}]}]}]}]}, {'->', [{line,85}], [[{warn, {'{}', [{line,85}], [{time,[{version,10},{line,85}],nil}, {result,[{version,11},{line,85}],nil}, {formatted_unnecessary_skips, [{version,12},{line,85}], nil}]}}], {'__block__',[], [{'=', [{line,86}], [{warnings,[{version,14},{line,86}],nil}, {{'.',[{line,86}],['Elixir.Enum',map]}, [{line,86}], [{result,[{version,11},{line,86}],nil}, {fn, [{line,86}], [{'->', [{line,86}], [[{x1,[{version,13}],elixir_fn}], {{'.', [{line,86}], ['Elixir.Dialyxir.Output',color]}, [{line,86}], [{x1,[{version,13}],elixir_fn},red]}]}]}]}]}, {'{}', [{line,88}], [warn,2, {{'.',[{line,89}],[erlang,'++']}, [{line,89}], [[{time,[{version,10},{line,89}],nil}], {{'.',[{line,89}],[erlang,'++']}, [{line,89}], [{warnings,[{version,14},{line,89}],nil}, [{formatted_unnecessary_skips, [{version,12},{line,89}], nil}, {warnings_msg,[{line,89}],[]}]]}]}]}]}]}, {'->', [{line,91}], [[{error, {{msg,[{version,15},{line,91}],nil}, {formatted_unnecessary_skips, [{version,16},{line,91}], nil}}}], {'{}', [{line,92}], [error,1, [{{'.',[{line,92}],['Elixir.Dialyxir.Output',color]}, [{line,92}], [{formatted_unnecessary_skips, [{version,16},{line,92}], nil}, red]}, {{'.',[{line,92}],['Elixir.Dialyxir.Output',color]}, [{line,92}], [{msg,[{version,15},{line,92}],nil},red]}]]}]}, {'->', [{line,94}], [[{error,{msg,[{version,17},{line,94}],nil}}], {'{}', [{line,95}], [error,1, [{{'.',[{line,95}],['Elixir.Dialyxir.Output',color]}, [{line,95}], [{msg,[{version,17},{line,95}],nil},red]}]]}]}]}]]}}]}, {{dialyze,2}, def, [{line,74}], [{[{line,74}], [{x0,[{generated,true},{version,0}],elixir_def}, {x1,[{generated,true},{version,1}],elixir_def}], [], {super, [{super,{def,dialyze}},{line,74}], [{x0,[{generated,true},{version,0}],elixir_def}, {x1,[{generated,true},{version,1}],elixir_def}, 'Elixir.Dialyxir.Project']}}]}, {{dialyze,1}, def, [{line,74}], [{[{line,74}], [{x0,[{generated,true},{version,0}],elixir_def}], [], {super, [{super,{def,dialyze}},{line,74}], [{x0,[{generated,true},{version,0}],elixir_def}, 'Elixir.Dialyxir.Dialyzer.Runner', 'Elixir.Dialyxir.Project']}}]}], deprecated => [], file => <<"/Users/kenny/@code/pot_dialyzer_warning_repro/deps/dialyxir/lib/dialyxir/dialyzer.ex">>, is_behaviour => false,line => 1,module => 'Elixir.Dialyxir.Dialyzer', relative_file => <<"lib/dialyxir/dialyzer.ex">>,unreachable => []}, []}, [no_copt,to_core,binary,return_errors,no_inline,strict_record_tests, strict_record_updates,dialyzer]], []}, {dialyzer_utils,get_core_from_beam,2, [{file,"dialyzer_utils.erl"},{line,116}]}]} Last messages in the log cache: Reading files and computing callgraph... ```

For the Erlang repo, I ran dialyzer --plt _build/default/rebar3_21.2.3_plt _build/default/lib/pot_dialzer_warning_repor_erl/ebin/*.beam:

✔ ~/@code/pot_dialyzer_warning_repro_erl [master L|✔] 
(ins)12:08 $ dialyzer --plt _build/default/rebar3_21.2.3_plt _build/default/lib/pot_dialzer_warning_repor_erl/ebin/*.beam
  Checking whether the PLT _build/default/rebar3_21.2.3_plt is up-to-date... yes
  Proceeding with analysis...
Unknown functions:
  pot:valid_totp/3
 done in 0m0.15s
done (passed successfully)
jeremyjh commented 3 years ago

I'm not sure what that output means for the Elixir project but you are running the analysis on Dialyxir's beams instead of your project's beams, if your project is named pot_dialyzer_warning_repro then use that in the path: 

dialyzer --plt _build/dev/dialyxir_erlang-21.2.3_elixir-1.10.4_deps-dev.plt _build/dev/lib/pot_dialyzer_warning_repro/ebin/*.beam

The output for the Erlang example suggests pot has not been added to that PLT, hence unknown function error.

kenny-evitt commented 3 years ago

@jeremyjh I finally tried your suggestion:

✔ ~/@code/pot_dialyzer_warning_repro [master L|✔] 
(ins)15:14 $ dialyzer --plt _build/dev/dialyxir_erlang-21.2.3_elixir-1.10.4_deps-dev.plt _build/dev/lib/pot_dialyzer_warning_repro/ebin/*.beam
  Checking whether the PLT _build/dev/dialyxir_erlang-21.2.3_elixir-1.10.4_deps-dev.plt is up-to-date... yes
  Proceeding with analysis...
dialyzer: Analysis failed with error:
{undef,
    [{elixir_erl,debug_info,
         [core_v1,'Elixir.PotDialyzerWarningRepro',
          {elixir_v1,
              #{attributes => [],compile_opts => [],
                definitions =>
                    [{{'totp_valid?',2},
                      def,
                      [{line,6}],
                      [{[{line,6}],
                        [{token,[{version,0},{line,6}],nil},
                         {secret,[{version,1},{line,6}],nil}],
                        [],
                        {{'.',[{line,7}],[pot,valid_totp]},
                         [{line,7}],
                         [{token,[{version,0},{line,7}],nil},
                          {secret,[{version,1},{line,7}],nil},
                          [{window,1}]]}}]}],
                deprecated => [],
                file =>
                    <<"/Users/kenny/@code/pot_dialyzer_warning_repro/lib/pot_dialyzer_warning_repro.ex">>,
                is_behaviour => false,line => 1,
                module => 'Elixir.PotDialyzerWarningRepro',
                relative_file => <<"lib/pot_dialyzer_warning_repro.ex">>,
                unreachable => []},
              []},
          [no_copt,to_core,binary,return_errors,no_inline,strict_record_tests,
           strict_record_updates,dialyzer]],
         []},
     {dialyzer_utils,get_core_from_beam,2,
         [{file,"dialyzer_utils.erl"},{line,116}]},
     {dialyzer_analysis_callgraph,compile_byte,5,
         [{file,"dialyzer_analysis_callgraph.erl"},{line,416}]},
     {dialyzer_worker,loop,2,[{file,"dialyzer_worker.erl"},{line,90}]}]}
...
kenny-evitt commented 3 years ago

I tried a new command for the Erlang version too and reproduced the same original error:

✔ ~/@code/pot_dialyzer_warning_repro_erl [master L|✔] 
(ins)15:58 $ dialyzer --plt /Users/kenny/.cache/rebar3/rebar3_21.2.3_plt -c _build/default/lib/pot/ebin/*.beam _build/default/lib/pot_dialyzer_warning_repro_erl/ebin/*.beam
  Checking whether the PLT /Users/kenny/.cache/rebar3/rebar3_21.2.3_plt is up-to-date... yes
  Proceeding with analysis...
pot_dialyzer_warning_repro_erl.erl:5: Function is_totp_valid/2 has no local return
pot_dialyzer_warning_repro_erl.erl:6: The call pot:valid_totp(Token::any(),Secret::any(),[{'window',1},...]) will never return since it differs in the 3rd argument from the success typing arguments: (binary(),any(),[{'token_length',pos_integer()}])
 done in 0m0.23s
done (warnings were emitted)