Disallow compile warnings in CI

[mightyiam]

Co-authored-by: Shahar "Dawn" Or [email protected]

[mightyiam]

I see that CI is waiting on approval from a maintainer. So we will explain ourselves. Our intention in this PR is to disallow compile warnings in the codebase. And, of course, to eventually "take care" of each existing warning. The reason that this draft PR currently only attempts to make CI fail on compile warnings and does not yet fix the individual warnings is that we wanted to see it fail, first.

The warnings are one of:

• warning: a method with this name may be added to the standard library in the future
• warning: use of deprecated method ...

In the cases of the former kind, we will use fully qualified paths to disambiguate, thereby eliminating the warning.

In the cases of the latter kind, we can place #[allow(deprecated)] to avoid the warnings. This would make sense, because the deprecated items are our own.

If maintainers agree with this intention of not having warnings in the codebase, consider allowing the CI run. And we will attempt to fix all warnings in reasonable ways.

[phimuemue]

Hi there, thanks for this.

Do you happen to have the fix for the warnings at hand? If so, could you open a PR?

[mightyiam]

@phimuemue sorry for taking so long. We do intend to work on this. @warren2k and I. If no one else intends to pick this up, we would appreciate the patience. Also, will our subsequent pull requests CI runs require approval or is it "first contribution"?

[mightyiam]

@phimuemue ready!

[Philippe-Cholet]

I'm no maintainer here but I would surely appreciate to see no warning while experimenting on Itertools.

I'm not sure to understand why u32 is there in the first place, but why put it behind the "use_alloc" feature? Obviously only used in alloc context (Powerset).

[Philippe-Cholet]

Note that I just removed pow_scalar_base in 4671345 while updating Powerset.

[mightyiam]

Oh, that's nice. Thank you for your work, @Philippe-Cholet. And thank you for letting us know. We're working on this in mob programming format. We've made progress today. It's not in this branch yet. Another session scheduled for tomorrow.

Hey, do you suppose we could get exempt from workflows needing approval, perhaps?

[Philippe-Cholet]

I just wanted to let you know about the little upcoming conflict.
As a non-maintainer, I don't know about workflows approval. From what I understand, you need it (from a maintainer) in your first PR and I guess it's automatic in future PR.

[mightyiam]

@phimuemue it seems done. Should work. So un-drafting. Please approve workflow.

[mightyiam]

Converted to draft because requires rebase.

[mightyiam]

Please approve workflow and review.

[mightyiam]

All checks passed!

[jswrenn]

I'd prefer that we didn't create a top-level wrappers.rs file, and instead #[allow(deprecated, unstable_name_collisions)] as needed.

[mightyiam]

Just a bit for you to consider, @jswrenn. Other than the sheer number of #[allow()]s, some of these warnings are in expressions that include multiple chained method calls. So the span of the #[allow()] is more than we intend. Feels clumsy to me.
Unless, of course, we extract the very call to which it is intended, which gets ugly immediately.

Call it and we'll do it.

[jswrenn]

I think we should blanket-allow both deprecated and unstable_name_collisions.

For deprecated:

• We don't care when it's our own deprecated methods.
• We only take a dependency on one other crate, and I'm not particularly worried about it deprecating anything.
• We don't really care when the standard library deprecates methods. It's rare, and the standard library doesn't ever remove deprecated methods.

For unstable_name_collisions: This is a non-actionable warning at this time. Until RFC 2845 is implemented, both itertools and the standard library is deadlocked on moving forward. We can't remove methods that trigger unstable_name_collisions without breaking our dependents, and the standard library can't stabilize unstable conflicting methods without breaking itertools dependants.

[mightyiam]

Sounds like you've thought it through. Will do.

[mightyiam]

Done.

[warren2k]

Rebased. Have yet to address the feedback.

[jswrenn]

I attempted to fix a merge conflict so I could merge this, but ultimately broke CI. I'm not sure why.

Could you roll back to 352bd2d, fix the merge conflict, and re-request my review?

[mightyiam]

Sure. Likely this weekend.

[warren2k]

All checks have passed. We think this is ready for merge.

[mightyiam]

Yes. Even though these two commits are included in #740 , they already provide value and so we'd love for them to be merged 😉 @jswrenn .