-
Notifications
You must be signed in to change notification settings - Fork 3k
Make the compiler report 'and'/'or' operators as obsolete #9115
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
base: master
Are you sure you want to change the base?
Conversation
CT Test Results 23 files 909 suites 6h 57m 45s ⏱️ Results for commit 054c362. ♻️ 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 |
f87ae5a to
b20ddac
Compare
b20ddac to
aa1be07
Compare
|
Should |
Nah, I thought I'd try to introduce && and || instead. Honestly I'm not sure. I think there's a bit of pedagogical value and visibility in the andalso/orelse keywords. (These names go back to Standard ML, if anyone wonders.) As I was replacing the occurrences of and/or in the code, I saw that a tiny 'or' or 'and' was easy to miss, but 'orelse' and 'andalso' stood out much better. So it might not even be worth doing, even if it saves horizontal space. We can submit a followup EEP in 2030 and see what people think then. 😜 Btw, replacing existing uses showed mainly two categories: 1) valid ways of writing more complex guards or other nested logical expressions (not all that common, in comparison to the total size of the codebase), and 2) as a personal style thing, where you could just as well have used comma and semicolon in a guard instead (only found in a few isolated modules) , possibly due to a lack of understanding of guard syntax. Also, a some of these occurrences probably go back to before semicolon was added to guards. |
Indeed. I just tried finding them in RabbitMQ to fix them and it is clearly non-obvious. The other two have this advantage. |
aa1be07 to
a7f60e4
Compare
|
@essen I did however come up with another use for 'or' (doesn't strictly require removing the original). Here's a very small proof of concept: master...richcarl:otp:or-patterns - e.g. |
That would definitely remove a frequent pain point with regard to case clauses! 👍 |
Yes. It might be technically possible to use |
bjorng
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I think that , and ; should be used if possible. I know that everyone does not agree with that.
As a compromise, if a guard only uses and, replace all and operators with commas. I think that is reasonable because , has been there since the beginning.
I strongly suggest that you remove the unnecessary parentheses on either side of andalso/orelse; not having to add those parentheses is one of the advantages over the older operators.
a7f60e4 to
af25af7
Compare
|
Updated like @bjorng suggested. Definitely yields nicer code, but a bit harder to review. |
af25af7 to
ff70061
Compare
|
Fixed , instead of ; in megaco that failed the type check. |
36a6fd5 to
31d81e6
Compare
|
The windows build died for some reason: |
I re-triggered the windows run and now it seems to have passed. |
31d81e6 to
875a5eb
Compare
False. Sometimes one is after the side effects of an expression that returns a boolean, e.g.: [edited] This code runs several actions and returns
Please stop using this excuse to cripple a language: virtually anything can be confusing for a newcomer, depending on their background. Imagine the bewilderment of someone who switches from COBOL. |
This is typically written over multiple lines, with the result computed from the intermediate results like R1 = precondition(...),
R2 = [...]
R3 = [...]
R4 = [...]
R1 orelse R2 orelse R3 orelse R4It's more verbose but also a lot clearer since you don't need to know the semantics of |
We can go further: |
Let's not exaggerate, the problem is that there's two Some people argue that I don't think I've seen calls to remove anything else. |
I simply explained why "more verbose but also a lot clearer" is flawed argument, because syntactic reductionism leads there: [edited]
There's no such problem. There are two pairs of operators with different semantics, just like & and && in C. These operators have existed in the language since time immemorial, removing them won't make any code clearer, it will just litter the existing code with compiler pragmas: |
|
P.S. I speak on behalf of a small and unimportant group of people: those who're deeply invested in large and/or complex Erlang projects. There's already a growing problem of abandoned dependencies that we have to fork and maintain due to bit-rot. Such arbitrary changes to the language driven by subjective arguments will just waste more of our resources that could be invested into building new things. |
That is not what is happening. Some syntax gets added, some gets removed. We have to remove some things in order to improve the language.
Me too? The removal of or/and has a very small maintenance impact. There's been far more complex removals such as parameterised modules. |
|
Friendly reminder that a compiler warning is just a warning, and that we still retain obsolete things like As such, we are not going to drop If the PR is accepted, you're free to disable the warning project-wide because it's fine not to follow our recommendations. Again, warnings are not an indication that we are about to remove something. |
| warn_obsolete_op(Op, A, Anno, St) -> | ||
| case {Op, A} of | ||
| {'and', 2} -> | ||
| add_warning(Anno, {deprecated, "'and'", "use 'andalso' instead", "OTP 29"}, St); | ||
| {'or', 2} -> | ||
| add_warning(Anno, {deprecated, "'or'", "use 'orelse' instead", "OTP 29"}, St); | ||
| _ -> St | ||
| end. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a way to disable this warning, and keep it disabled by default for now. Regardless of what decision we're going to make, I still see it as a useful optional warning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@richcarl If you do this, and rebase the branch, we can include this in OTP 29. (Feel free to enable the warning for all code in OTP.)
|
There's one more issue with |
lib/public_key/src/public_key.erl
Outdated
| is_binary(CryptDer), | ||
| is_list(Cipher), | ||
| is_binary(Salt), | ||
| (erlang:byte_size(Salt) == 8 orelse erlang:byte_size(Salt) == 16), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note a PR changing this instance of or to orelse has already been merged.
I guess the changes here of andalso to , falls under the category as describe as matter of style. This was discussed in the PR fixing or -> orelse and I could not find why this was done but our conclusion was that it was done for consistency as there are some cases that require more complex Boolean expressions. I do not have a strong opinion here @bjorng, @jhogberg any opinions what makes most sense? Consistency of using only one or the other way or consistency of only using andalso, orelse if some of the expression are complex ?
In any case this PR needs rebasing!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally prefer to use , ; whenever possible, using andalso orelse only as required, but ultimately it's up to the maintainer of each application.
|
As @Maria-12648430 pointed out in a similar PR, there is another difference between This means that sth like Thus, simply replacing all |
lib/crypto/test/crypto_SUITE.erl
Outdated
| case crypto:cipher_info(C) of | ||
| #{prop_aead := true} -> | ||
| true and Ok; | ||
| true andalso Ok; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is (and was) pointless, only Ok suffices. However, this entire test is problematic, see #10138.
c4b379c to
62b17ea
Compare
eaa49f5 to
054c362
Compare
|
Changed to off by default and not replacing any uses in the code, just enabling the flag in the apps where they occur and adding nowarn-flags to specific modules. Can't see what's wrong with the test cases at the moment though. |
The strict 'and'/'or' operators are mostly useless, confusing for newcomers, rarely used in practice, and when used they can always be replaced with andalso/orelse. I think they should be obsoleted and subsequently dropped from the language.