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

dialyzer: Document the meaning of specs #6281

Merged
merged 1 commit into from
Sep 22, 2022
Merged

dialyzer: Document the meaning of specs #6281

merged 1 commit into from
Sep 22, 2022

Conversation

TD5
Copy link
Contributor

@TD5 TD5 commented Sep 6, 2022

Add a section in Dialyzer's docs to expand on how specs are checked and used to refine Dialyzer's inferred types, since this is often a point of confusion.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 6, 2022

CT Test Results

    2 files    38 suites   16m 45s ⏱️
450 tests 447 ✔️ 3 💤 0
536 runs  533 ✔️ 3 💤 0

Results for commit 5853e6f.

♻️ This comment has been updated with latest results.

To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.

See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.

Artifacts

// Erlang/OTP Github Action Bot

@garazdawi
Copy link
Contributor

Would it make sense to add some examples to showcase some common mistakes in this area?

@TD5
Copy link
Contributor Author

TD5 commented Sep 7, 2022

Thanks for your suggestion @garazdawi. I've added some examples of how specs and inferred type interact, and some context explaining the potential implications. I am open to suggestions around the wording, etc.

@garazdawi
Copy link
Contributor

One additional thought, would it make sense to mention -Woverspecs in the text as a tool to find these problem areas?

The text looks good to me, but I'll leave the final review to @bjorng as he is both better at Dialyzer and reviewing English than I am.

@TD5
Copy link
Contributor Author

TD5 commented Sep 9, 2022

One additional thought, would it make sense to mention -Woverspecs in the text as a tool to find these problem areas?

The text looks good to me, but I'll leave the final review to @bjorng as he is both better at Dialyzer and reviewing English than I am.

Good question. In short, I am not entirely sure of the meaning of -Woverspecs. For example, overspec'ing a function argument would be bad, but overspec'ing a return would be fine, but I am not sure whether this new option considers variance like that (most of the code I've seen does not).

I also suspect it would suffer from the problem of generating false-positives, as with #6243

@rickard-green rickard-green added the team:VM Assigned to OTP team VM label Sep 12, 2022
@bjorng
Copy link
Contributor

bjorng commented Sep 15, 2022

I find the examples confusing. I have pushed a suggestion for making the example more concrete as a fixup commit. Please have a look and feel free to further clarify and improve.

Add a section in Dialyzer's docs to expand on how specs are checked and
used to refine Dialyzer's inferred types, since this is often a point
of confusion.
@TD5
Copy link
Contributor Author

TD5 commented Sep 21, 2022

Thanks for your touch ups to make these docs much more clear, @garazdawi & @bjorng: I've squashed them into the original commit.

@bjorng bjorng merged commit 621f292 into erlang:master Sep 22, 2022
@bjorng
Copy link
Contributor

bjorng commented Sep 22, 2022

Thanks!

@hanssv
Copy link
Contributor

hanssv commented Sep 23, 2022

@TD5 there is a typo on line 291... Dialyzer will infer a | b -> binary() not b | c -> binary().

@bjorng
Copy link
Contributor

bjorng commented Sep 26, 2022

@hanssv Thanks! Corrected.

@TD5 TD5 deleted the DialyzerDocs branch September 29, 2022 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team:VM Assigned to OTP team VM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants