I'm posting here instead of in clippy issues because to fix the issue a new method could be added to numeric types.
If you have this piece of code (pretty common as far as I can see):
if version >= 1 && version < 42 {
// ...
}
clippy suggests to use range, however the resulting code is:
if (0..42).contains(version) {
// ...
}
Which reads strange - Yoda condition. On top of it it contains annoying parentheses. A simple solution would be to add in_range(range: Range) method for all numbers that just calls contains on Range. The code would then become:
if version.in_range(0..42) {
// ...
}
Which I believe reads very well. Note that contains is still reasonable for the opposite situation (range.contains(42))
My question is if it'd be reasonable to add such method. It's trivial, so maintenance burden would be minimal but also the issue is trivial. I'm not sure what's better. What do you think?
Aside: it'd look cool to have Python-like in operator but I think that's overengineering.
I'd love to have such a method. One potential hazard: this method would need to get implemented via a blanket impl on any PartialOrd, so it'd show up on many types and potentially introduce conflicts.
That said, I think it'd be worth trying, and I've wanted this in the past.
I don't feel like (0..42).contains(thing) is always a problem, but it does sometimes feel like a reversal, and it doesn't chain well; if you have x.y().z(), it's nicer to write x.y().z().in_range(0..42) rather than (0..42).contains(x.y().z()).
The same applies to most of Iterator's default methods as well (and in fact some of them (potentially, didn't actually check) can't be correctly implemented if the type they return doesn't have a constructor beyond the Iterator method).
For better or for worse it's already standard to provide "there's no reason to override this" default methods on traits.
IIRC the prelude glob import is weaker than an explicit glob import. The breakage potential is specifically method resolution if a type has an in_range trait method currently (an inherent method always shadows a trait method). In theory we could make non-prelude trait methods shadow preluded trait methods to make adding methods to prelude traits never cause method resolution ambiguity.
Whether we'd want to is a different question ofc. It'd not be a nice bit of underhanded code to shadow standard trait methods with an innocuous looking import (say, a typo squatted itertools).
Would it be possible to add the in operator to Rust? I think it could desugar as
foo in bar ==> bar.contains(&foo)
Besides the prettier syntax and the somewhat more natural ordering, it would also help to avoid the annoying & when testing that a value is in collection. This is, personally, a common yet trivial error when I'm dealing with containers of primitives.
With regards to the precedent, such a feature is present in Python and Kotlin.
An obvious objection is that currently the contains method is an inherent method, not a trait one, and that would likely cause some compatibility issues.
It is a good thing that in is already a keyword, and with a very restricted usage. Syntax-wise, I don't believe there could be any blockers.
I would add a new Contains trait for that desugaring. Ranges and most collection types could implement this, and a lang-aware trait is better than duck-typing to inherent methods.
Maybe the simplest solution is to stop clippy from pushing the contains form?
I find version >= 1 && version < 42 perfectly adequate. This is a universal syntax that works the same in almost any programming language. OTOH the range syntax is inconsistent between programming languages and different from mathematical notation.
I think clippy is overzealous here. The .contains() alternative could make sense if the expression was very long or computationally expensive, but for a simple numeric variable it's more trouble than it's worth.
I thought about it but I like the argument that people sometimes get the operators wrong, so a method is better. Besides, in_range is immediately obvious what it means - no need to parse the operators. Still, is it worth it?
@afetisov@cuviper if there's in definitely with Contains trait. But as I said it looks over-engineered to e. A simple method can be implemented in library today without changes to the language.
It's not more overengineered than literally any other overloadable operator. It also provides a consistent interface for collection membership (which allow e.g. generic programming) and a convincing nudge to consider implementing the required methods.
A method with inversed order of arguments, on the other hand, provides none of those benefits and just a minor syntactic benefit, which is likely to not be realised since most collections in the wild will never implement it.
Personally I prefer to write such chained comparisons as 1 <= version && version < 42. This makes it clear that version is just contained between two numbers. Rust doesn't support literal chained comparisons 1 <= version < 42, unlike Python, but that syntactic form is imho the next best approximation.
In my experience the other overloadable operators are used much more often. If there's a significant number of people wanting this I'd be fine with it, just not going to push for it.
I find this super jarring, it seems like lazy design. Not only is it confusing, it also has a performance cost -- if you have a dyn Iterator, there is an unnecessary dynamic dispatch to these functions that shouldn't be virtual. I would say: don't make this antipattern more widespread.
There should be a very high bar to add new default trait methods, especially to universally implemented traits like PartialOrd and Ord. I think that adding clamp was not the right call, for example.
I think traits that were not designed with this pattern in mind at first (but for example Iterator and Read were) - they should not be expanded to suddenly be in the extension methods style.
The reason is that while these methods work for the use case they are designed for but may not be a good fit for every existing type. (I could see there are exceptions when the new method fits so exactly into the existing trait's definition.)
A late tweak to existing APIs and types that are already out there is not so nice and it can lead to conceptual conflicts - maybe they already have an in_range method with a different meaning. Their inherent method would still have preference - but conceptually it's now a clash.
Best not to change a trait's status w.r.t this later in the game.