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

Tracking issue for warning for rust_2018_idioms by default #54910

Open
4 of 11 tasks
shepmaster opened this issue Oct 8, 2018 · 12 comments
Open
4 of 11 tasks

Tracking issue for warning for rust_2018_idioms by default #54910

shepmaster opened this issue Oct 8, 2018 · 12 comments
Labels
A-edition-2018-lints Area: Lints supporting the 2018 edition C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC S-tracking-impl-incomplete Status: The implementation is incomplete. WG-epoch Working group: Epoch (2018) management

Comments

@shepmaster
Copy link
Member

shepmaster commented Oct 8, 2018 •
edited by tmandry
Loading

#[warn(rust_2018_idioms)] is not going to be enabled by default for Rust 2018 because we are taking a conservative stance and we aren't sure about how good the suggestions are yet.

The plan is to enable this lint by default some number of releases / months after Rust 2018 ships.

Progress

This summary was last updated from this comment; check to see if there are new comments since then.

/cc @Centril @aturon @Mark-Simulacrum
@shepmaster shepmaster added C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC A-edition-2018-lints Area: Lints supporting the 2018 edition labels Oct 8, 2018
@aturon aturon mentioned this issue Oct 8, 2018
Closed
7 tasks
@Centril Centril added the WG-epoch Working group: Epoch (2018) management label Oct 8, 2018
@oberien
Copy link
Contributor

oberien commented Mar 14, 2019

Is there an update on this? I'd like to see at least some of the new currently allow-by-default lints to become warn-by-default. Personally I'm missing elided_lifetime_in_path the most, because these kinds of elided lifetimes regularly lead to hard to spot bugs.

@hellow554
Copy link
Contributor

@oberien you could modify your environment variables and add

RUSTFLAGS="$RUSTFLAGS -W rust_2018_idioms"

and you have it enabled for all your projects on your local machine.

@memoryruins
Copy link
Contributor

It would be great to have bare_trait_objects set to warn by default. I often see users in chats accidentally use trait objects when they did not intend to.

@Centril
Copy link
Contributor

Centril commented May 13, 2019

I have a few PRs:

which, unfortunately, I don't have time for at the moment.

Feel free to pick up any of them.

@memoryruins
Copy link
Contributor

After setting bare_trait_objects to warn and running --bless on the tests, 300+ ui stderr files changed due to the warnings (expected after seeing the output from the previous PR ).

If there is still interest to see a crater run with deny set, I think it's possible to run crater without CI tests passing as long as if the build succeeds.

If we continued with warn, we could change many of the tests to use dyn, avoiding committing too much stderr noise. Some tests would be left bare to continue testing the ui of the lint. What do you think?

@Centril
Copy link
Contributor

Centril commented May 25, 2019

@memoryruins Feel free to send in a PR for warnings and I'll FCP that one. We can crank up to deny later.

@jebrosen jebrosen mentioned this issue May 25, 2019
Closed
@memoryruins memoryruins mentioned this issue May 26, 2019
Merged
bors added a commit that referenced this issue May 29, 2019
@bors
Auto merge of #61203 - memoryruins:bare_trait_objects, r=Centril
4137901 
Warn on bare_trait_objects by default

The `bare_trait_objects` lint is set to `warn` by default.
Most ui tests have been updated to use `dyn` to avoid creating noise in stderr files.

r? @Centril

cc #54910
@memoryruins memoryruins mentioned this issue May 30, 2019
Merged
Centril added a commit to Centril/rust that referenced this issue May 30, 2019
@Centril
Rollup merge of rust-lang#61342 - memoryruins:ellipsis_inclusive_rang…
60b05fe 
…e_patterns, r=Centril

Set ellipsis_inclusive_range_patterns lint to warn

Continuing rust-lang#54910, the `ellipsis_inclusive_range_patterns` lint is set to warn.

r? @Centril
Centril added a commit to Centril/rust that referenced this issue May 30, 2019
@Centril
Rollup merge of rust-lang#61342 - memoryruins:ellipsis_inclusive_rang…
d1d35ce 
…e_patterns, r=Centril

Set ellipsis_inclusive_range_patterns lint to warn

Continuing rust-lang#54910, the `ellipsis_inclusive_range_patterns` lint is set to warn.

r? @Centril
@memoryruins
Copy link
Contributor

memoryruins commented May 30, 2019 •
edited
Loading

Update:

To-do:

If anyone has an interest in helping these lints be enabled, e.g. by tackling any of those potentially blocking diagnostic message issues, go for it!

Centril added a commit to Centril/rust that referenced this issue May 30, 2019
@Centril
Rollup merge of rust-lang#61342 - memoryruins:ellipsis_inclusive_rang…
30b3139 
…e_patterns, r=Centril

Set ellipsis_inclusive_range_patterns lint to warn

Continuing rust-lang#54910, the `ellipsis_inclusive_range_patterns` lint is set to warn.

r? @Centril
pietroalbini added a commit to pietroalbini/rust that referenced this issue May 31, 2019
@pietroalbini
Rollup merge of rust-lang#61342 - memoryruins:ellipsis_inclusive_rang…
860deae 
…e_patterns, r=Centril

Set ellipsis_inclusive_range_patterns lint to warn

Continuing rust-lang#54910, the `ellipsis_inclusive_range_patterns` lint is set to warn.

r? @Centril
@ehuss ehuss mentioned this issue Jun 13, 2019
Open
@zackmdavis zackmdavis mentioned this issue Jun 14, 2019
Closed
@zackmdavis
Copy link
Member

#55768 also impacts (the rustfixability of) elided_lifetimes_in_paths.

facebook-github-bot pushed a commit to facebookarchive/mononoke that referenced this issue Jul 12, 2019
@facebook-github-bot
rust: Head start on some upcoming warnings
fed2ac8 
Summary:
This diff sets two Rust lints to warn in fbcode:

```
[rust]
  warn_lints = bare_trait_objects, ellipsis_inclusive_range_patterns
```

and fixes occurrences of those warnings within common/rust, hg, and mononoke.

Both of these lints are set to warn by default starting with rustc 1.37. Enabling them early avoids writing even more new code that needs to be fixed when we pull in 1.37 in six weeks.

Upstream tracking issue: rust-lang/rust#54910

Reviewed By: Imxset21

Differential Revision: D16200291

fbshipit-source-id: aca11a7a944e9fa95f94e226b52f6f053b97ec74
@parasyte parasyte mentioned this issue May 21, 2020
Merged
@lberrymage
Copy link

#71957 is another open issue for elided_lifetimes_in_paths, although to my (very limited) knowledge it shouldn't affect making the lint warn-by-default.

@avitex
Copy link
Contributor

avitex commented Dec 28, 2021

Just bumping this issue seeing #55768 and #71957 are now resolved.

With a quick check, looks like the remaining issues as linked above copied here are centered around unused_extern_crates:

A more recent issue opened after the linked comment:

hhirtz added a commit to LIHPC-Computational-Geometry/coupe that referenced this issue Jun 13, 2022
@hhirtz
Enable the rust_2018_idioms lint as warning
bd9f9b1 
This should become the default in the future:
rust-lang/rust#54910

Reason this is put in lib.rs instead of CI: to ensure local invocations
of cargo build and cargo clippy pick up the warning.

Reason this is made a warning: so that code builds and runs anyway. On
the CI, the `-D warnings` clippy argument will treat it as a hard error.
@hhirtz hhirtz mentioned this issue Jun 13, 2022
Merged
hhirtz added a commit to LIHPC-Computational-Geometry/coupe that referenced this issue Jun 13, 2022
@hhirtz
Enable the rust_2018_idioms lint as warning
48c2d88 
This should become the default in the future:
rust-lang/rust#54910

Reason this is put in lib.rs instead of CI: to ensure local invocations
of cargo build and cargo clippy pick up the warning.

Reason this is made a warning: so that code builds and runs anyway. On
the CI, the `-D warnings` clippy argument will treat it as a hard error.
@ComputerDruid
Copy link
Contributor

Quick note: elided_lifetimes_in_paths as warn-by-default is tracked in #91639

@Mirko-von-Leipzig Mirko-von-Leipzig mentioned this issue Jun 14, 2022
Merged
3 tasks
@wesleywiser wesleywiser added the S-tracking-impl-incomplete Status: The implementation is incomplete. label Jun 24, 2022
@ratmice ratmice mentioned this issue Nov 3, 2022
Merged
@Veykril Veykril mentioned this issue Jun 5, 2023
Merged
@alice-i-cecile alice-i-cecile mentioned this issue Oct 15, 2024
Open
@BenjaminBrienen
Copy link

I tried denying elided_lifetimes_in_paths in bevyengine/bevy#15916. Is this the new preferred style?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-edition-2018-lints Area: Lints supporting the 2018 edition C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC S-tracking-impl-incomplete Status: The implementation is incomplete. WG-epoch Working group: Epoch (2018) management
Projects
Lang Edition 2024
Archived in project
Milestone
No milestone
Development

No branches or pull requests