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

OTP-28 readiness (part 1) #2937

Merged
merged 2 commits into from
Feb 19, 2025
Merged

OTP-28 readiness (part 1) #2937

merged 2 commits into from
Feb 19, 2025

Conversation

ferd
Copy link
Collaborator

@ferd ferd commented Feb 14, 2025

Covers portions of #2936, namely the bits about Dialyzer checks for xrl and yrl compilers, and for Common Test and EUnit.

Dependencies will need to be done in a different pull request.

@ferd ferd requested a review from tsloughter February 14, 2025 01:02
@tsloughter
Copy link
Collaborator

Was changing append to maybe_append because it was broken and dialyzer caught it now or did another functions return type change?

@ferd
Copy link
Collaborator Author

ferd commented Feb 15, 2025

Was changing append to maybe_append because it was broken and dialyzer caught it now or did another functions return type change?

Basically we used GoodResult | {error, Reason} as a return type internally, but GoodResult is a dict(), which is an opaque type, so any matching on it is raising opacity red flags. The append/2 function wrapped error as passthrough and assumed non-error results could be appended safely. lists:append/2 is called elsewhere in the module.

So to respect opacity constraints, I wrapped internal to uses {ok, GoodResult} | {error, Reason} all the way through, which lets us properly avoid opacity issues. I rename append to maybe_append to better represent its role as "append but with error handling".

Specifically, I think Dialyzer got better at catching some stuff it ignored before?

@ferd ferd merged commit 9311637 into erlang:main Feb 19, 2025
6 checks passed
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

Successfully merging this pull request may close these issues.

2 participants