Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

v0.1.x: Apply dyn keywords to avoid fatal warnings on recent nightlies #1351

Closed
wants to merge 4 commits into from

Conversation

dekellum
Copy link
Contributor

@dekellum dekellum commented Jul 24, 2019

This is part (1) of proposed changes in #1348. It may need to wait for stronger motivation, but I wanted to make the changes available. This depends on an MSRV of at least 1.27.0.

Motivation

On the v0.1.x branch, the use of #[deny(warnings)] makes the following warnings fatal on recent nighty compilers (in my case currently: rustc 1.38.0-nightly (83e4eed16 2019-07-14)), when building a local git tree.

error: trait objects without an explicit `dyn` are deprecated
   --> tokio-current-thread/src/scheduler.rs:128:23
    |
128 | struct Task(Spawn<Box<Future<Item = (), Error = ()>>>);
    |                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `dyn`: `dyn Future<Item = (), Error = ()>`
    |
note: lint level defined here
   --> tokio-current-thread/src/lib.rs:2:9
    |
2   | #![deny(warnings, missing_docs, missing_debug_implementations)]
    |         ^^^^^^^^
    = note: #[deny(bare_trait_objects)] implied by #[deny(warnings)]

When built as a dependency of a normal application, cargo passes --cap-lints=allow to rustc (or for cargo -vv, --cap-lints=warn at most), so these are not fatal in that case. The current azure-pipelines CI appears to only test with stable rust (e.g. 1.36.0) or older nightlies, so this hasn't broken CI, yet.

Solution

I applied cargo fix followed by cargo fmt and then filtered through the changes (git add --patch) to isolate these dyn additions. Also this bumps the check-minrust MSRV to 1.27.2, just to hopefully to get CI passing.

recent rust nightly started warning that not using `dyn` was
deprecated. This requires MSRV 1.27.0+.
@dekellum
Copy link
Contributor Author

dekellum commented Jul 24, 2019

CI will fail on 1.26 (for no dyn support) and 1.27.2 for various reasons, including:

  • tempfile 3.1.0 more recent release (dev dependency) is MSRV 1.32.0

  • and (new in 1.27.2):

error: unknown lint: `rust_2018_idioms`
 --> tokio-executor/src/lib.rs:4:10
  |
4 | #![allow(rust_2018_idioms)]
  |          ^^^^^^^^^^^^^^^^
  |
note: lint level defined here
 --> tokio-executor/src/lib.rs:1:54
  |
1 | #![deny(missing_docs, missing_debug_implementations, warnings)]
  |                                                      ^^^^^^^^
  = note: #[deny(unknown_lints)] implied by #[deny(warnings)]
  • and (also new in 1.27.2):
error: failed to parse manifest at `/home/david/src/tokio/tokio-macros/Cargo.toml`

Caused by:
  editions are unstable

Caused by:
  feature `edition` is required

So it falls back to the question of the proposal MSRV.

@dekellum
Copy link
Contributor Author

Note that when rust 1.37.0 (stable) is released in ~3 weeks, with the change to warn for lack of dyn (e.g. bare_trait_objects), then CI will presumably be broken for v0.1.x branch.

https://github.com/rust-lang/rust/blob/f2f4c0ed6c247eb8e94845c3e37eb6799ec74885/RELEASES.md

@dekellum
Copy link
Contributor Author

In 45108c2 removed the allow(rust_2018_idioms) which was selectively applied and caused a failure on 1.27.2 atleast, in order to cargo fix previously missed dyn cases (d350014). CI continues to fail as expected here, but should work when merged with the MSRV increase in #1358.

@dekellum
Copy link
Contributor Author

dekellum commented Aug 1, 2019

CI now passing again, as merged in #1358. @LucioFranco are you good with this (to the extent that MSRV 1.31.0 is accepted) or would you prefer to keep all the warnings in output, but not fail, as in #1368?

@LucioFranco
Copy link
Member

@dekellum hey sorry for the delay on this! I've been quite swamped at work and we really want to get the first few alpha versions out. I want you to know I'm not ignoring this at all! I am going to try to give this and the other PRs reviews once we get the alphas out in the next few days. Let me know if you need anything. And again sorry its taking this long 😭

@LucioFranco LucioFranco added the T-v0.1.x Topic: tokio 0.1.x label Aug 15, 2019
@dekellum
Copy link
Contributor Author

This was merged by me as part of #1451, squashed and applied to v0.1.x in c9532e4. Closing.

@dekellum dekellum closed this Aug 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-v0.1.x Topic: tokio 0.1.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants