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.

Sign up for GitHub

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

Jump to bottom

Fix new Rust unused warnings #4130

Merged
merged 6 commits into from
Jul 26, 2024
Merged

Fix new Rust unused warnings #4130

merged 6 commits into from
Jul 26, 2024

Conversation

Ten0
Copy link
Member

@Ten0 Ten0 commented Jul 25, 2024 •
edited
Loading

New compiler warnings seem to indicate that this wasn't used.
Instead the CombinationClause type holds both right hand side and left side of combinations, and implements Query directly.

@Ten0
Remove unused NoCombinationClause type
c7ab84f 
New compiler warnings seem to highlight that this wasn't used
@Ten0
Copy link
Member Author

Ten0 commented Jul 25, 2024 •
edited
Loading

Ah that error seems wrong:
User is constructed here: https://github.com/Ten0/diesel/blob/c7ab84fc71fc637cb0cbd1f93e5730bcae6eb189/examples/postgres/all_about_inserts/src/lib.rs#L273
EDIT: Ah that's probably complaining that it's unused when not #[cfg(test)] because the Queryable implementation is never called.

@Ten0 Ten0 force-pushed the remove_unused_NoCombinationClause branch from 550c19c to 74209c4 Compare July 25, 2024 14:42
@Ten0 Ten0 changed the title Remove unused NoCombinationClause type Fix new rust warnings Jul 25, 2024
@Ten0 Ten0 changed the title Fix new rust warnings Fix new Rust unused warnings Jul 25, 2024
@Ten0
Copy link
Member Author

Ten0 commented Jul 25, 2024 •
edited
Loading

I'm unclear on how to update these, I ran TRYBUILD=overwrite cargo +stable test --manifest-path diesel_compile_tests/Cargo.toml but I get diffs on every file:
image

as if verbose was force-enabled on my computer but I don't know by what since the CI doesn't have that.

@weiznich
Copy link
Member

I'm not sure what's going on with the compile tests update for you. It doesn't produce that output locally for me. I just pushed the changes produced locally, hopefully that is also what the CI produces.

I also fixed the new clippy warnings + bumped the toolchain version to 1.80

weiznich
weiznich reviewed Jul 26, 2024
View reviewed changes
diesel_compile_tests/tests/fail/only_only_on_table.stderr
@@ -74,4 +74,4 @@ note: the trait `Table` must be implemented
= note: the following trait defines an item `only`, perhaps you need to implement it:
candidate #1: `diesel::dsl::OnlyDsl`

error: internal compiler error: compiler/rustc_infer/src/infer/at.rs:400:21: relating different kinds: diesel::pg::Pg '?2
error: internal compiler error: compiler/rustc_infer/src/infer/at.rs:364:21: relating different kinds: diesel::pg::Pg '?2
Copy link
Member

Choose a reason for hiding this comment

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

rust-lang/rust#126129

@weiznich weiznich force-pushed the remove_unused_NoCombinationClause branch from 1cde89a to 223b8eb Compare July 26, 2024 07:05
@Ten0
Copy link
Member Author

Ten0 commented Jul 26, 2024

Thanks a lot!

@weiznich weiznich added this pull request to the merge queue Jul 26, 2024
Merged via the queue into diesel-rs:master with commit c75870c Jul 26, 2024
49 checks passed
weiznich added a commit to weiznich/diesel that referenced this pull request Aug 23, 2024
@weiznich
Merge pull request diesel-rs#4130 from Ten0/remove_unused_NoCombinati…
8a8d9da 
…onClause

Fix new Rust unused warnings
@weiznich weiznich mentioned this pull request Aug 23, 2024
Merged
weiznich added a commit to weiznich/diesel that referenced this pull request Aug 23, 2024
@weiznich
Merge pull request diesel-rs#4130 from Ten0/remove_unused_NoCombinati…
fcd602f 
…onClause

Fix new Rust unused warnings
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@weiznich weiznich weiznich approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Ten0 @weiznich