Replace usages of and and or with andalso and orelse
#10129
Conversation
Co-authored-by: Maria Scott <[email protected]>
CT Test Results 20 files 846 suites 5h 51m 55s ⏱️ Results for commit 0a01fe6. ♻️ 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 |
| 1> dets:open_file(t, []), | ||
| ok = dets:insert(t, [{1,a},{2,b},{3,c},{4,d}]), | ||
| MS = ets:fun2ms(fun({X,Y}) when (X > 1) or (X < 5) -> {Y} end), | ||
| MS = ets:fun2ms(fun({X,Y}) when (X > 1) orelse (X < 5) -> {Y} end), |
There was a problem hiding this comment.
On a side note:
when (X > 1) orelse (X < 5) ->
I may be missing the point here, but this guard makes no sense to me. It will always turn out true, because what is not greater than 1 will still be less than 5. Shouldn't it be andalso (or ,)?
Looks like there were only two usages of |
IngelaAndin
added
team:VM
| if Importance >= (100-VLvl) -> | ||
| CtLogFd = State#logger_state.ct_log_fd, | ||
| DoEscChars = State#logger_state.tc_esc_chars and EscChars, | ||
| DoEscChars = State#logger_state.tc_esc_chars andalso EscChars, |
There was a problem hiding this comment.
It just occurred to me that in usages like this, there is a very subtle change in behavior when using andalso (or orelse for that matter) instead of and (or or).
DoEscChars = State#logger_state.tc_esc_chars and EscChars,If EscChars is not a boolean, this will always fail with a badarg error. Put differently, DoEscChars is guaranteed to contain a boolean.
DoEscChars = State#logger_state.tc_esc_chars andalso EscChars,If State#logger_state.tc_esc_chars is true and EscChars is not a boolean, the result is whatever is in EscChars. That is, DoEscChars is not guaranteed to contain a boolean any more.
The differences in behavior are those:
- with
and...- if
State#logger_state.tc_esc_charsorEscCharsor both are not booleans, there will be abadargerror - otherwise, the result (-->
DoEscChars) is a boolean
- if
- with
andalso...- if
State#logger_state.tc_esc_charsis not a boolean, there will be abadargerror - if
State#logger_state.tc_esc_charsisfalse, the result (-->DoEncChars) will befalse, no matter what is inEncChars - if
State#logger_state.tc_esc_charsistrue, the result (-->DoEncChars) will be whatever is inEncChars
- if
This is harmless in this place AFAICT, but may lead to (or conceal) subtle hard to detect bugs elsewhere.
There was a problem hiding this comment.
As I see it, we could either...
- leave it as is and trust that we always receive boolean
EscChars - append yet another
... andalso true, which would fail ifState#logger_state.tc_esc_charsistrueandEscCharsis a non-boolean (would look weird and justify an explaining comment) - put sth like
true = is_boolean(EscChars)before this line, which would fail ifEscCharsis a non-boolean, regardless of the value ofState#logger_state.tc_esc_chars
|
|
||
| verify_extensions([#'TBSCertList_revokedCertificates_SEQOF'{crlEntryExtensions = Ext} | Rest]) -> | ||
| verify_extensions(pubkey_cert:extensions_list(Ext)) and verify_extensions(Rest); | ||
| verify_extensions(pubkey_cert:extensions_list(Ext)) andalso verify_extensions(Rest); |
There was a problem hiding this comment.
This is also a change in behavior that may lead to verify_extensions behaving differently. Assume that Rest contains some garbage, like foo. With and, verify_extensions would always throw an exception in that case. With andalso, it will not throw an exception (but return false) if any value before lets it result in false.
This tl;dr means that the right-hand sides of updated boolean operators need to be checked for side effects (which could be an exception). While they would always happen with and or or, they may not happen any more with andalso or orelse because of their short-circuit behavior.
There was a problem hiding this comment.
In this case I think it would improve the behavior of verify_extensions as it is not expected to throw an error, and if I did not misread the code right now that would make public_key:validate_crl misbehave.
|
@Maria-12648430 you love to make things difficult, don't you 🤦♂️ But yeah, guess you're right... |
|
Note that there is already #9115, which is similar to this PR. |
Ah, that was what I thought was being discussed on the Erlang Forums, but it was a PR instead. Well... now you have the choice of two PRs 😉 Anyway, what the people in #9115 and @Maria-12648430 here pointed out still holds, so I guess more work needs to be put into it than just replacing the old with the new ones. |
Yes I do 🥰 |
After finding occurrences of the "old" boolean operators in
public_key(see #10114), we went looking and discovered many more. Those are addressed in this PR.The searching was done with the help of AI but it didn't find them all, so @Maria-12648430 also went through the grep results manually. Despite all her efforts, she may still have missed a few.
Replacing the old operators with their modern counterparts obsoletes some of the parentheses that were in place. However, we did not remove them, to avoid the risk of accidentially changing precedences.
We only went through what is under
/lib/*/src(ie, not/ertsetc), to keep the scope of this PR within reasonable limits. For that reason, we also did not go throughwx, which is pretty huge.We also did not look into usage of old operators in tests. That, too, is for a later PR.
On a side note, IIRC it was suggested on the Erlang Forums a while ago that the old boolean operators should be deprecated. I would suggest at least a compiler warning when they are used. I can't think of a single instance where I would want to use them (vs the modern ones). The occurrences we found in this PR fall in the categories "introduced with very old code", "overlooked when refactoring very old code" and "used by accident", as far as I can tell.