Lint against iterator functions that panic when N is zero

[Urgau]

View all comments

This PR adds new variant to the deny-by-default unconditional_panic lint, by linting on iterator functions that panics when N (chunks/windows size) is zero¹.

Those methods are documented to panic if N is zero.

error: this operation will panic at runtime
  --> $DIR/const-n-is-zero.rs:11:13
   |
LL |     let _ = s.array_windows::<0>();
   |             ^^^^^^^^^^^^^^^^^^^^^^ const parameter `N` is zero
   |
   = note: `#[deny(unconditional_panic)]` on by default

cc @rust-lang/libs-api

Footnotes

1. this is done by introducing a new internal attribute on the const parameter: #[rustc_panics_when_zero] ↩

[rustbot]

Some changes occurred in compiler/rustc_hir/src/attrs

cc @jdonszelmann, @JonathanBrouwer

This PR changes MIR

cc @oli-obk, @RalfJung, @JakobDegen, @vakaras

Some changes occurred in compiler/rustc_passes/src/check_attr.rs

cc @jdonszelmann, @JonathanBrouwer

This PR changes rustc_public

cc @oli-obk, @celinval, @ouz-a, @makai410

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred in compiler/rustc_attr_parsing

cc @jdonszelmann, @JonathanBrouwer

[rustbot]

r? @JohnTitor

rustbot has assigned @JohnTitor.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler
• compiler expanded to 69 candidates
• Random selection from 16 candidates

[RalfJung]

This PR adds new kind of variant to the deny-by-default unconditional_panic lint, by linting on iterator functions that panics when N (chunks/windows size) is zero.

This seems to do much more than that. It adds a new language primitive, a new kind of built-in assertion. Could you motivate that?

[Urgau]

This seems to do much more than that. It adds a new language primitive, a new kind of built-in assertion. Could you motivate that?

The code is unfortunately tangled, it doesn't add a new language primitive per se, but report_assert_as_lint requires a AssertKind variant even if it's just for linting in one place.

I could probably modify that function so it doesn't take an AssertKind, but I wasn't sure about that.

[RalfJung]

Given that AssertKind is part of the MIR syntax, I feel very strongly that we shouldn't modify it for non-MIR/opsem purposes.

[Urgau]

Understandable, I've reworked the PR to avoid touching at AssertKind.

[JohnTitor]

I love the idea but it should better for someone more familiar with this topic other than me to review. @RalfJung Could you take over? Otherwise I'd reroll.

View changes since this review

[RalfJung]

I'm afraid not, I don't have capacity at the moment.

@rustbot reroll

[fee1-dead]

I think the overall idea is good and I could see myself approving down the line, however I think we should see if we can have an attribute on the generic parameter itself instead of applying rustc_panics_when_n_is_zero to the enclosing function. If not, I'd still want something more verbose, such as #[rustc_panics_when_const_param_is_zero(N)]

I don't think this should be touching AssertLint. I feel like it might be simpler to just add a separate error for this rather than using existing mechanisms.

View changes since this review

[Urgau]

I had to add the encoding of const-params in rmeta, but otherwise I've moved the attribute directly to the const generic.

I've also removed any interaction with AssertLint, but kept the reporting behind the unconditional_panic lint.

@rustbot ready

[fee1-dead]

overall looks good, would like someone else to sign off with me on this together though

@rustbot reroll

[jackh726]

Before I review, I'm going to wait for the lang team to +1.

[joshtriplett]

The use of a runtime panic here (rather than a compile-time error like a const panic) was intentional to allow for cases like this:

if N > 0 {
    ... array_chunks::<N>() ..
}

If we used a compile-time error, people couldn't write code like that (without some kind of special "const if" that we don't yet have).

Does the lint handle that case? It needs to.

Please add this as a test case and make sure it does not produce a lint.

[Urgau]

I didn't know that, good to know.

Fortunately, the lint as currently implemented does not lint on this case, due to only analyzing the generic MIR.

I added a test case for it.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[scottmcm]

This seems like clearly worth linting, under the usual intent of "we should lint anything that will always panic if reached".
@rfcbot merge lang

---

Not this PR and not this FCP: I would love to see attributes like this move into the diagnostics namespace to let more things be self-serve for libraries to get lints. If we could no longer need lints like https://rust-lang.github.io/rust-clippy/stable/index.html?search=step_#iterator_step_by_zero because the definition of Iterator::step_by includes #[diagnostics::always_panics_if(eq(0)) step: usize, that'd be wonderful. (No idea if that's the right way to spell it or the right way to encode a predicate in a macro, but hopefully gets the idea across.)

[rust-rfcbot]

Team member @scottmcm has proposed to merge this. The next step is review by the rest of the tagged team members:

• @joshtriplett
• @nikomatsakis
• @scottmcm
• @tmandry
• @traviscross

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

[traviscross]

Makes sense to me. It's important that it not lint the if N > 0 { array_windows::<N>() } case. It doesn't, since it doesn't resolve N here. So let's do it.

@rfcbot reviewed