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.

Sign up for GitHub

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

Jump to bottom

dialyzer: Fix bug in bind_checked_inf/4 #6588

Merged
merged 2 commits into from
Dec 22, 2022
Merged

dialyzer: Fix bug in bind_checked_inf/4 #6588

merged 2 commits into from
Dec 22, 2022

Conversation

jhogberg
Copy link
Contributor

@jhogberg jhogberg commented Dec 21, 2022 •
edited
Loading

?unit can't be bound any more than ?none can.

While we're at it, rename t_is_none_or_unit/1 to t_is_impossible/1 because the former is as silly as saying t_is_integer_or_float/1 instead of t_is_number/1.

Fixes #6580

@jhogberg jhogberg added team:VM Assigned to OTP team VM fix testing currently being tested, tag is used by OTP internal CI labels Dec 21, 2022
@jhogberg jhogberg requested a review from bjorng December 21, 2022 07:45
@jhogberg jhogberg self-assigned this Dec 21, 2022
@jhogberg jhogberg mentioned this pull request Dec 21, 2022
Closed
@github-actions
Copy link
Contributor

github-actions bot commented Dec 21, 2022 •
edited
Loading

CT Test Results

       5 files     137 suites   42m 23s ⏱️
2 295 tests 2 220 ✔️ 72 💤 3 ❌
2 679 runs  2 598 ✔️ 78 💤 3 ❌

For more details on these failures, see this check.

Results for commit 0c59cb0.

♻️ 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

@jhogberg jhogberg linked an issue Dec 21, 2022 that may be closed by this pull request
[dialyzer] crash in erl_types:cons_hd #6580
Closed
@aronisstav
Copy link
Contributor

Fair point about the name of the none or unit, but maybe t_is_bottom is even better?

@jhogberg
Copy link
Contributor Author

jhogberg commented Dec 21, 2022 •
edited
Loading

I thought about that and it didn't feel right to have "two" bottom types, but I'll happily change it if that's what everyone else prefers.

Edit: to elaborate, the lattice can only have one bottom type, so if I saw t_is_bottom/1 I'd have a hard time not reading it as t_is_none/1. I thought t_is_impossible/1 was an okay name since both ?unit and ?none describe a value that cannot exist.

@jhogberg jhogberg force-pushed the john/dialyzer/erl_types-cons_hd-crash/GH-6580/OTP-18372 branch from 27c1c63 to 4921ae8 Compare December 21, 2022 08:59
@jhogberg
dialyzer_dataflow: Fix bug in bind_checked_inf/4
3dc51db 
`unit` can't be bound any more than `none` can.

This resulted in slightly improved analysis in some cases, taking
notice of more functions in OTP that lacked a local return, so I've
added specs to those functions in order to silence the warnings.

The pattern for that is:

```erlang
try function_that_returns_unit() catch _:_ -> error(xyz) end
```

Previously, this expression was treated as returning unit() even
though it could only ever return none(), as the wrapped function
would never return.
@jhogberg
dialyzer: Rename t_is_none_or_unit/1 to t_is_impossible/1
0c59cb0 
Calling this function t_is_none_or_unit/1 is like saying
t_is_integer_or_float/1 instead of t_is_number/1. I wouldn't be
the least bit surprised if this weird naming was the root cause of
erlangGH-6580.
@jhogberg jhogberg force-pushed the john/dialyzer/erl_types-cons_hd-crash/GH-6580/OTP-18372 branch from 4921ae8 to 0c59cb0 Compare December 21, 2022 09:01
@jhogberg jhogberg added this to the OTP-25.3 milestone Dec 22, 2022
@jhogberg jhogberg merged commit 348718d into erlang:maint Dec 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bjorng bjorng bjorng approved these changes

Assignees

@jhogberg jhogberg

Labels
fix team:VM Assigned to OTP team VM testing currently being tested, tag is used by OTP internal CI
Projects
None yet
Milestone
OTP-25.3
Development

Successfully merging this pull request may close these issues.

[dialyzer] crash in erl_types:cons_hd
3 participants
@jhogberg @aronisstav @bjorng