Clippy "unnecessary_wraps" lint should be off by default, isn't

[smklein]

Currently, we run clippy as a part of CI with default options. Default clippy settings should "allow" pedantic lints - such as unnecessary_wraps - but for some reason, this lint is triggering warnings.

This bug tracks the explicit "allow" for this lint (a workaround) which may be removed when this is resolved.

[davepacheco]

Here's a bit more background.

1. We saw this warning in early discussions on Fix clippy lints and run clippy in CI #29, discussed it, and decided to ignore it.
2. By the time Fix clippy lints and run clippy in CI #29 was integrated, we did not see the warning any more, but we also hadn't done anything to ignore it.
3. We do not see the warning in the latest "master" commit's CI run.
4. We do see this on the latest "steno" branch CI run, even though no relevant code or configuration has changed.
5. It's reproducible in a very simple repo on 1.50.
6. It's not reproducible in a very simple repo on 1.49.

I gather what must have happened was that:

• This lint was introduced as non-pedantic in 1.50 (either a new lint or else an existing lint that was made not-pedantic in 1.50).
• This lint was moved to the "pedantic" category on clippy's master, which we presumably won't see until at least 1.51.
• The GitHub Actions CI environment was updated from 1.49 to 1.50 (presumably by virtue of an Ubuntu 18.04 LTS update) within the last week (i.e., after the most recent "master" commit and before the recent "steno" branch commits). This seems confirmed by this commit.

So the explanations for the above data points are:

1. Sean initially saw this because he started Fix clippy lints and run clippy in CI #29 using nightly clippy, which was post-1.49 and after "unnecessary_wraps" was added in a non-pedantic category.
2. By the time Fix clippy lints and run clippy in CI #29 was integrated, it had switched to stable clippy, which was 1.49. That either didn't have this check at all or it wasn't pedantic. So we didn't see this warning, despite not doing anything to ignore it.
3. The latest master commit's CI run was on Ubuntu 18.04 prior to the 1.50 update, so it was still on 1.49, which again either didn't have the check or it wasn't pedantic.
4. The latest steno branch commit CI runs are on Ubuntu 18.04 after the 1.50 update, so it's seeing the lint again.

(5) and (6) follow directly from above.

---

Given all of the above, I think the fix to disable this makes sense. Two caveats: (1) I wonder if this will needless break developers using 1.49 and other recent versions, if those don't have this check at all. (2) We should remember to remove this once we update to 1.51 (or whatever version of Rust includes a clippy where the warning is pedantic).

The obvious alternative is to pin the CI build to 1.49 until an updated Rust is released with the clippy change. In an ideal world, I think I would pin CI to a particular stable version, but whenever a new one is released, I would have a bot file a PR to update it. Most of the time, I would expect that to be a trivial change that just updates the version and passes all CI tests. In cases like this, it would require someone to add a commit that fixes the breakage as well. Short of that automation, though, I'm more worried about the problem of getting pinned to an ancient toolchain.

[smklein]

Agree with this, a couple comments inline.

1. Sean initially saw this because he started Fix clippy lints and run clippy in CI #29 using nightly clippy, which was post-1.49 and after "unnecessary_wraps" was added in a non-pedantic category.
2. By the time Fix clippy lints and run clippy in CI #29 was integrated, it had switched to stable clippy, which was 1.49. That either didn't have this check at all or it wasn't pedantic. So we didn't see this warning, despite not doing anything to ignore it.

Yeah, it didn't exist on stable at that time - I tried adding an allow(clippy::unnecessary_wraps) at the time, but it threw an "Unknown clippy lint" warning, which became an error on CI. As a result, we waited.

3. The latest master commit's CI run was on Ubuntu 18.04 prior to the 1.50 update, so it was still on 1.49, which again either didn't have the check or it wasn't pedantic.
4. The latest steno branch commit CI runs are on Ubuntu 18.04 after the 1.50 update, so it's seeing the lint again.

(5) and (6) follow directly from above.

Given all of the above, I think the fix to disable this makes sense. Two caveats: (1) I wonder if this will needless break developers using 1.49 and other recent versions, if those don't have this check at all. (2) We should remember to remove this once we update to 1.51 (or whatever version of Rust includes a clippy where the warning is pedantic).

Agreed.

The obvious alternative is to pin the CI build to 1.49 until an updated Rust is released with the clippy change. In an ideal world, I think I would pin CI to a particular stable version, but whenever a new one is released, I would have a bot file a PR to update it. Most of the time, I would expect that to be a trivial change that just updates the version and passes all CI tests. In cases like this, it would require someone to add a commit that fixes the breakage as well. Short of that automation, though, I'm more worried about the problem of getting pinned to an ancient toolchain.

Yeah, this decision seems fine to me - the cost for "a line or two" sorta fixes seems relatively quick, and probably worth it to be aware of new updates (so we can stay on top of changes) as soon as possible. I'd probably have a different opinion if we were less active in development, but for now, these sorts of changes seem like they (should? I hope???) be relatively small fixes.

[smklein]

Since the workaround now landed, I'll close this issue, with follow-up in #40 .