Clippy suggestion `range.contains` may cause Yoda condition - is my fix a good idea?

libs
1

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.

11 Likes
2

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()).

3 Likes
3

So try to impl it on PartialOrd and see what crater says? If it's too much breakage fallback to std types?

4

Not on PartialOrd directly; I think it'd have to be a separate InRange trait that has a blanket impl based on PartialOrd.

5

Why not? This seems analogous to the addition of Ord::clamp to me. Something like:

    fn in_range(&self, range: impl crate::ops::RangeBounds<Self>) -> bool;
1 Like
6

Hmm, wouldn't it be a bit annoying having to import it every time?

7

I had figured that'd be more likely to generate conflicts, since things may have PartialOrd imported but wouldn't have the new thing imported.

If we're willing to take that risk, then sure, we could put it on PartialOrd. Though it seems odd to be able to override it.

8

PartialOrd is in the current prelude already. [playground]

9

Yes, the hypothetical InRange trait wouldn't be.

10

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).

3 Likes
11

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.

1 Like
12

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.

1 Like
13

A comment brought up the idea of an in operator in the range contains RFC:

14

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.

13 Likes
15

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.

1 Like
16

I suppose someone could accidentally write v < 1 && v > 42, but I'm not sure how likely this is compared to writing .. when ..= is needed.

1 Like
17

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.

11 Likes
18

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.

19

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.

20

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.

2 Likes