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

Add #![deny(rust_2018_idioms)] to all crates #451

Merged
merged 2 commits into from
Mar 13, 2024
Merged

Conversation

jpschorr
Copy link
Contributor

@jpschorr jpschorr commented Mar 13, 2024

Add #![deny(rust_2018_idioms)] to all crates

The wording is a bit confusing for this, but this will result in an error for any code that uses pre-rust 2018 idioms (for this codebase, most notably the elided_lifetimes_in_paths lint).

Elided lifetime parameters can make it difficult to see at a glance that borrowing is occurring. This lint ensures that lifetime parameters are always explicitly stated, even if it is the '_ placeholder lifetime.

This lint is “allow” by default because it has some known issues, and may require a significant transition for old code.

-- from: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_lint/builtin/static.ELIDED_LIFETIMES_IN_PATHS.html


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Copy link

github-actions bot commented Mar 13, 2024

Conformance comparison report

Base (af599b6) 7ae1c68 +/-
% Passing 90.35% 90.35% 0.00%
✅ Passing 5731 5731 0
❌ Failing 612 612 0
🔶 Ignored 0 0 0
Total Tests 6343 6343 0

Number passing in both: 5731

Number failing in both: 612

Number passing in Base (af599b6) but now fail: 0

Number failing in Base (af599b6) but now pass: 0

@jpschorr jpschorr force-pushed the feat-cleanup-warnings branch 2 times, most recently from f9c7bba to ce45715 Compare March 13, 2024 22:26
find . -iname lib.rs ! -path "*target/*" -exec sed -i '' -e '1s;^;#![deny(rust_2018_idioms)]\n\n;' {} +
@jpschorr jpschorr force-pushed the feat-cleanup-warnings branch from ce45715 to 3db585c Compare March 13, 2024 22:29
@jpschorr jpschorr marked this pull request as ready for review March 13, 2024 22:34
@jpschorr jpschorr requested review from am357 and alancai98 March 13, 2024 22:34
@jpschorr jpschorr changed the title Feat cleanup warnings Add #![deny(rust_2018_idioms)] to all crates Mar 13, 2024
Copy link

codecov bot commented Mar 13, 2024

Codecov Report

Attention: Patch coverage is 60.90909% with 43 lines in your changes are missing coverage. Please review.

Project coverage is 79.26%. Comparing base (af599b6) to head (3db585c).

Files Patch % Lines
...sion/partiql-extension-visualize/src/ast_to_dot.rs 0.00% 36 Missing ⚠️
...ion/partiql-extension-visualize/src/plan_to_dot.rs 0.00% 2 Missing ⚠️
partiql-value/src/lib.rs 77.77% 2 Missing ⚠️
partiql-eval/src/error.rs 0.00% 1 Missing ⚠️
partiql-eval/src/eval/evaluable.rs 94.73% 1 Missing ⚠️
partiql-value/src/tuple.rs 80.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #451   +/-   ##
=======================================
  Coverage   79.26%   79.26%           
=======================================
  Files          66       66           
  Lines       17749    17752    +3     
  Branches    17749    17752    +3     
=======================================
+ Hits        14069    14072    +3     
  Misses       3110     3110           
  Partials      570      570           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -27,6 +27,7 @@ use partiql_source_map::metadata::LocationMap;
#[allow(clippy::unused_unit)]
#[allow(unused_variables)]
#[allow(dead_code)]
#[allow(rust_2018_idioms)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason we're choosing to allow rust_2018_idioms here while in partiql-parser/src/lib.rs, we deny it?

Clippy's also giving the following warning:

allow(rust_2018_idioms) is ignored unless specified at crate level

warning: allow(rust_2018_idioms) is ignored unless specified at crate level
  --> partiql-parser/src/parse/mod.rs:30:9
   |
30 | #[allow(rust_2018_idioms)]
   |         ^^^^^^^^^^^^^^^^
   |
   = note: `#[warn(unused_attributes)]` on by default

Copy link
Contributor Author

@jpschorr jpschorr Mar 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The allow here is for the inclusion of the LALRPOP generated code.

Without it cargo build yields:

error: `extern crate` is not idiomatic in the new edition
  --> .../partiql-lang-rust/target/debug/build/partiql-parser-12b4184392ff8cc1/out/partiql.rs:15:1
   |
15 | extern crate core;
   | ^^^^^^^^^^^^^^^^^^
   |
note: the lint level is defined here
  --> partiql-parser/src/lib.rs:1:9
   |
1  | #![deny(rust_2018_idioms)]
   |         ^^^^^^^^^^^^^^^^
   = note: `#[deny(unused_extern_crates)]` implied by `#[deny(rust_2018_idioms)]`
help: convert it to a `use`
   |
15 | use core;
   | ~~~

error: `extern crate` is not idiomatic in the new edition
  --> .../partiql-lang-rust/target/debug/build/partiql-parser-12b4184392ff8cc1/out/partiql.rs:34:5
   |
34 |     extern crate core;
   |     ^^^^^^^^^^^^^^^^^^
   |
help: convert it to a `use`
   |
34 |     use core;
   |     ~~~

error: outlives requirements can be inferred
    --> ...partiql-lang-rust/target/debug/build/partiql-parser-12b4184392ff8cc1/out/partiql.rs:6301:28
     |
6301 |     where Id: IdGenerator, 'input: 'state, Id: 'state
     |                            ^^^^^^^^^^^^^^^^^^^^^^^^^^
     |
     = note: `#[deny(explicit_outlives_requirements)]` implied by `#[deny(rust_2018_idioms)]`
help: remove these bounds
     |
6301 -     where Id: IdGenerator, 'input: 'state, Id: 'state
6301 +     where Id: IdGenerator,
     |

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. Guess we'll need the allow for now then

@jpschorr jpschorr merged commit 6d31b39 into main Mar 13, 2024
18 of 19 checks passed
@jpschorr jpschorr deleted the feat-cleanup-warnings branch March 13, 2024 23:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants