#[unhandled] attribute for trait methods that should error at compile time

language design
1

Hey everyone o/

I was staring again at the classic infinite-iterator footgun:

let x = std::iter::repeat(42).last();  // silently loops forever today
let n = std::iter::repeat(42).count(); // same story

We all know these methods can never do anything useful on an infinite iterator. Right now the solutions are:

  • loop forever
  • panic
  • lints
  • pray

All of them are runtime or “soft” solutions.

So a dumb idea popped into my head that feels almost too simple:

What if we could just write this in the actual impl?

impl<A: Clone> Iterator for Repeat<A> {
    // normal stuff...

    #[unhandled]  // or #[invalid], #[forbidden], #[nope], whatever
    fn last(self) -> Option<Self::Item>;

    #[unhandled(message = "count() never terminates on an infinite iterator")]
    fn count(self) -> usize;
}

And then:

std::iter::repeat(42).last();
// error: method `last` is marked as unhandled for `Repeat<_>`
// note: this iterator is infinite; there is no last element

It would be a hard compile-time error, zero runtime cost, fully backward compatible (if you don’t write the attribute, everything works exactly like today).

Questions:

  • Has something exactly like this been proposed before and I just missed it?
  • Does this break any deep trait philosophy I’m not seeing?
  • Is it actually as easy to implement as it feels (the compiler already knows which methods exist on which impls)?
5 Likes
2

This can be a lint ala clippy::infinite_iter (which catches count but doesn't catch last).

The usage of count or last or similar can be buried behind layers of indirection, and in any case is not a part of public API (and so could be added in the future). If there were a way for an API to declare that it takes Iterator + !impl Iterator::count + !impl Iterator::last ... then you'd have something, until somebody comes along and writes a Collatz iterator.

1 Like
3

Take this code for example:

fn count_this_iter<T: Iterator>(iter: T) -> usize {
    iter.count()
}

This could get infinitely more complicated, and at a certain point there isn't a way for the compiler to ensure that count isn't used for Iterators where it is unhandled (so it would have to emit an implementation of count that panics anyways).

Given that it would be possible to call an unhandled function regardless, it's a pretty weak compiler error (and I agree it should be a lint instead).

1 Like
4

The problem of making this a clippy lint is that then it can catch some specific cases, but doesn't let libraries and user code to define their own impls with unhandled methods.

In this specific case, I think that adding a post monomorphization error would be helpful (if I happen to pass a guaranteed-to-be-infinite iterator error to this function, I would like to know). Or, post-mono lint, if it's to be a lint.

But in any case, even if it catches things on a best effort basis, it's still useful

5

You're completely right: in fully generic code like

fn count_this_iter<T: Iterator>(iter: T) -> usize {
    iter.count()
}

the compiler can't know at the definition site that T might sometimes be an infinite iterator, so it couldn't reject the call to .count() upfront.

However, once monomorphization happens and the compiler instantiates

count_this_iter(std::iter::repeat(42usize))

it suddenly knows the concrete type perfectly well. At that point it could perfectly emit a hard error (a post-monomorphization error) with something like:

error[E9999]: call to `count` on an iterator known to be infinite
  --> src/lib.rs:10:10
   |
10 |     iter.count()
   |          ^^^^^
   |
note: `std::iter::Repeat<usize>` has marked `count` as unhandled

The remaining hard case is indeed is dyn Iterator:

fn oops(iter: &mut dyn Iterator<Item = usize>) {
    println!("{}", iter.count()); // no concrete type → can't prove anything
}

For that case there are basically three options:

  1. Require the trait object handled all methods.
  2. Silently generate a panic body for the unhandled method on the vtable (so dyn always works but panics if you call the forbidden method).
  3. Just fall back to a lint (allow by default), so the guarantee is best-effort.

Personally, I’m not a fan of leaving this kind of thing to best-effort / chance.

6

What about this code?

fn process(preallocate: bool, values: impl Iterator<Item = u32> + Clone) {
  let mut buffer = Vec::with_capacity(
    if preallocate { values.clone().count() } else { 0 }
  );
  // use buffer…
}

(Admittedly contrived and highly suboptimal, this should use ExactSizeIterator but then I don’t have an example.)

Simply having a call to a method that always panics or infinite loops isn’t necessarily wrong, because dynamically it might not be called. Your new attribute is basically the same, though attached to the declaration so it doesn’t depend on inlining. I don’t object to having some kind of check like this, but it’s never going to be exhaustive.

2 Likes
7

No I mean it is literally a clippy lint already. The lint could be made better. Not "static analysis" better, but they should add last() to that list.

@darilrt Zooming out a bit from your specific question: Yes, static analysers exist targetting rust and its various lower level compilation stages. They could all be improved, and several allow for extra annotations of the code. I don't know to what extent any are seeking improved integration with rustc. I don't think catching accidental infinite loops would be the motivating example for that integration, but it's certainly in the pile.

8

But Map<F, Repeat<T>> isn't going to be diagnosed statically either then. Filter<F, Repeat<T>> might be infinite depending on F's runtime behavior.

3 Likes
9

I think better than making a special attribute for it would be to improve the existing machinery to use const-time panics as compiler errors.

When I first read this post, my gut instinct was to do something like this:

pub struct InfiniteIterator;

impl Iterator for InfiniteIterator {
    type Item = ();

    fn next(&mut self) -> Option<Self::Item> {
        Some(())
    }

    fn count(self) -> usize {
        const { panic!("Is infinite") }
    }
}

Per the reference, this snippet is guaranteed to cause a compiler error if <u8 as Foo>::bar is ever called, though there aren't guarantees about compiler errors or not if it isn't called.

Sadly, it seems like right now this approach doesn't work, as this by itself fails to compile without any calls to count.

1 Like
10

I don't actually know if this is bad though. In this case I would not mind an error forcing someone to write

if let Some(_) = it.next() {
    loop {}
}

or something equally explicit and obvious. Way more obvious than my_complicated_iter.count(). Really it's going to take a base iterator of std::iter::from_fn(something_mathy) to be a real counterexample.

See Should we have another non-type never?

(wherein I'd rather we spelled it fn count(self) -> const ! {} but otherwise: agreed, this would be nice functionality)

11

I don't think that allowing you to not provide an impl makes sense, because of all the complex questions that people are asking about what happens when it gets called in non-obvious ways. (Unless you're thinking that this would use the default impl?)

I think it'd be much easier to phrase this as

#[diagnostics::lint_if_called(last, count)]
impl Iterator for Foo { ... }

or similar rather than trying to have this affect behaviour.

4 Likes
12

Note that this would still only catch very simple cases where they are directly called on the Repeat instance. Doing a .filter(...).count() will break the lint, even if post-mono based, because it won't end up calling .count() on the underlying iterator.

1 Like