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

The json_term() spec is inconsistent with the README and current implementation #50

Closed
sumerman opened this issue Feb 7, 2014 · 7 comments

Comments

@sumerman
Copy link

sumerman commented Feb 7, 2014

Therefore makes dialyzer real angry sometimes.

The call jsx:encode([{},...]) will never return since it differs in the 1st argument from the success typing arguments: ('end_stream' | 'false' | 'null' | 'true' | binary() | ['false' | 'null' | 'true' | binary() | [any()] | number() | {_,_}] | number())

And then a screen full of warnings which disappear should I comment the clause with jsx:encode([{}]).

@talentdeficit
Copy link
Owner

i don't use dialyzer typically so i'm not sure i'm equipped to tackle this but i'll see what i can do

@sumerman
Copy link
Author

sumerman commented Feb 9, 2014

According to my little experiment, the fix is a simple inclusion of term [{}] to the json_term() type.

-type json_term() :: list({binary(), json_term()}) 
    | [{}]
    | list(json_term())
 ...

@sumerman
Copy link
Author

sumerman commented Feb 9, 2014

And it's also accurate to say that list({binary(), json_term()}) is actually list({binary() | atom(), json_term()})

@talentdeficit
Copy link
Owner

i just ran dialyzer against the current development branch and i didn't get either of these errors. could you let me know what version you ran dialyzer against? also the output from dialyzer would be great too

@sumerman
Copy link
Author

Of course you will not see any warnings running dialyzer against just jsx (without the underspec warning turned on) because in this case dialyzer checks internal jsx consistency. Consider the following example: jsx's code handles A, B and C cases, but its spec denotes it currently handles only A and B — it's perfectly ok for dialyzer; but when dialyzer checks caller's code it complains about using C because jsx's spec discourage it.
Furthermore, projects with lots of deps in order to reduce memory footprint and runtime typically run dialyzer only against it's own codebase with PLTs generated from deps' typespecs, like this:

APP_PLT = ./.app_plt

$(APP_PLT): 
    dialyzer --plt ~/.dialyzer_plt --build_plt --output_plt $(APP_PLT) deps/*/ebin

dialyze: build $(APP_PLT)
    dialyzer --plts ~/.dialyzer_plt $(APP_PLT) --fullpath ebin -Wunmatched_returns -Werror_handling -Wrace_conditions

Here ~/.dialyzer_plt is a pregenerated PLT for the standard OTP distribution, and ./.app_plt is the project local one, which contains typespecs extracted from deps.

And even if we put the dialyzer thing aside for a second inconsistency between the typespec and the README remains.

@talentdeficit
Copy link
Owner

okay thanks for helping me understand this. i think i've fixed it. let me know if you find any other problems

@sumerman
Copy link
Author

You're welcome. 5868430 passes dialyzation, thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants