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 suggestion for some #[deprecated] items #113365

Merged
merged 1 commit into from Aug 22, 2023
Merged

Add suggestion for some #[deprecated] items #113365

merged 1 commit into from Aug 22, 2023

Conversation

dima74
Copy link
Contributor

@dima74 dima74 commented Jul 5, 2023

Consider code:

fn main() {
    let _ = ["a", "b"].connect(" ");
}

Currently it shows deprecated warning:

warning: use of deprecated method `std::slice::<impl [T]>::connect`: renamed to join
 --> src/main.rs:2:24
  |
2 |     let _ = ["a", "b"].connect(" ");
  |                        ^^^^^^^
  |
  = note: `#[warn(deprecated)]` on by default

This PR adds suggestion for connect and some other deprecated items, so the warning will be changed to this:

warning: use of deprecated method `std::slice::<impl [T]>::connect`: renamed to join
 --> src/main.rs:2:24
  |
2 |     let _ = ["a", "b"].connect(" ");
  |                        ^^^^^^^
  |
  = note: `#[warn(deprecated)]` on by default
help: replace the use of the deprecated method
  |
2 |     let _ = ["a", "b"].join(" ");
  |                        ^^^^

@rustbot
Copy link
Collaborator

rustbot commented Jul 5, 2023

r? @m-ou-se

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jul 5, 2023
@workingjubilee
Copy link
Contributor

What do the error messages for these look like? At first brush, it seems like the existing notes for some might make the suggestions noisy and confusing? I mean, I can pull your commit, recompile rustc, and then build some sample code with these functions to verify that, but it seems reasonable to ask you to do the footwork and maybe even add a small ui test exercising the suggestions.

r? @workingjubilee
@rustbot author

@rustbot rustbot assigned workingjubilee and unassigned m-ou-se Jul 31, 2023
@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 31, 2023
@dima74
Copy link
Contributor Author

dima74 commented Aug 3, 2023

@workingjubilee updated PR description with example and added test. By the way there are existing deprecated methods with suggestion, e.g. "".trim_left() -> "".trim_start(). Also suggestions can be applied automatically by tools like cargo fix

@workingjubilee
Copy link
Contributor

Oh yes, I agree we should add deprecation suggestions, I'm just not entirely sure if we should have both a note and suggestion.

@dima74
Copy link
Contributor Author

dima74 commented Aug 5, 2023

@workingjubilee so what are next steps here, should I remove note from items for which suggestion was added?

@workingjubilee
Copy link
Contributor

Ah, very good. Hmmm. It feels to me that the notes which are literally just suggestions should be removed...

Except... you can't, I just tried while taking another look at this. Oops? I didn't realize that, sorry. That... should probably be allowed?

@bors r+ rollup=always

@bors
Copy link
Contributor

bors commented Aug 18, 2023

📌 Commit 950a249 has been approved by workingjubilee

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 18, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 18, 2023
…estions, r=workingjubilee

Add `suggestion` for some `#[deprecated]` items

Consider code:
```rust
fn main() {
    let _ = ["a", "b"].connect(" ");
}
```

Currently it shows deprecated warning:
```rust
warning: use of deprecated method `std::slice::<impl [T]>::connect`: renamed to join
 --> src/main.rs:2:24
  |
2 |     let _ = ["a", "b"].connect(" ");
  |                        ^^^^^^^
  |
  = note: `#[warn(deprecated)]` on by default
```

This PR adds `suggestion` for `connect` and some other deprecated items, so the warning will be changed to this:
```rust
warning: use of deprecated method `std::slice::<impl [T]>::connect`: renamed to join
 --> src/main.rs:2:24
  |
2 |     let _ = ["a", "b"].connect(" ");
  |                        ^^^^^^^
  |
  = note: `#[warn(deprecated)]` on by default
help: replace the use of the deprecated method
  |
2 |     let _ = ["a", "b"].join(" ");
  |                        ^^^^
```
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 19, 2023
…estions, r=workingjubilee

Add `suggestion` for some `#[deprecated]` items

Consider code:
```rust
fn main() {
    let _ = ["a", "b"].connect(" ");
}
```

Currently it shows deprecated warning:
```rust
warning: use of deprecated method `std::slice::<impl [T]>::connect`: renamed to join
 --> src/main.rs:2:24
  |
2 |     let _ = ["a", "b"].connect(" ");
  |                        ^^^^^^^
  |
  = note: `#[warn(deprecated)]` on by default
```

This PR adds `suggestion` for `connect` and some other deprecated items, so the warning will be changed to this:
```rust
warning: use of deprecated method `std::slice::<impl [T]>::connect`: renamed to join
 --> src/main.rs:2:24
  |
2 |     let _ = ["a", "b"].connect(" ");
  |                        ^^^^^^^
  |
  = note: `#[warn(deprecated)]` on by default
help: replace the use of the deprecated method
  |
2 |     let _ = ["a", "b"].join(" ");
  |                        ^^^^
```
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 19, 2023
…estions, r=workingjubilee

Add `suggestion` for some `#[deprecated]` items

Consider code:
```rust
fn main() {
    let _ = ["a", "b"].connect(" ");
}
```

Currently it shows deprecated warning:
```rust
warning: use of deprecated method `std::slice::<impl [T]>::connect`: renamed to join
 --> src/main.rs:2:24
  |
2 |     let _ = ["a", "b"].connect(" ");
  |                        ^^^^^^^
  |
  = note: `#[warn(deprecated)]` on by default
```

This PR adds `suggestion` for `connect` and some other deprecated items, so the warning will be changed to this:
```rust
warning: use of deprecated method `std::slice::<impl [T]>::connect`: renamed to join
 --> src/main.rs:2:24
  |
2 |     let _ = ["a", "b"].connect(" ");
  |                        ^^^^^^^
  |
  = note: `#[warn(deprecated)]` on by default
help: replace the use of the deprecated method
  |
2 |     let _ = ["a", "b"].join(" ");
  |                        ^^^^
```
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 19, 2023
…estions, r=workingjubilee

Add `suggestion` for some `#[deprecated]` items

Consider code:
```rust
fn main() {
    let _ = ["a", "b"].connect(" ");
}
```

Currently it shows deprecated warning:
```rust
warning: use of deprecated method `std::slice::<impl [T]>::connect`: renamed to join
 --> src/main.rs:2:24
  |
2 |     let _ = ["a", "b"].connect(" ");
  |                        ^^^^^^^
  |
  = note: `#[warn(deprecated)]` on by default
```

This PR adds `suggestion` for `connect` and some other deprecated items, so the warning will be changed to this:
```rust
warning: use of deprecated method `std::slice::<impl [T]>::connect`: renamed to join
 --> src/main.rs:2:24
  |
2 |     let _ = ["a", "b"].connect(" ");
  |                        ^^^^^^^
  |
  = note: `#[warn(deprecated)]` on by default
help: replace the use of the deprecated method
  |
2 |     let _ = ["a", "b"].join(" ");
  |                        ^^^^
```
#[deprecated(
since = "1.6.0",
note = "replaced by `std::sync::Condvar::wait_timeout`",
suggestion = "wait_timeout"
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem correct, as the suggestion lint message is Applicability::MachineApplicable, and wait_timeout accepts a different type to wait_timeout_ms. This means that cargo fix would break working code.

@KamilaBorowska
Copy link
Contributor

KamilaBorowska commented Aug 19, 2023

I don't think cause, before_exec, wait_timeout_ms, sleep_ms, and park_timeout_ms methods should have suggestion as long the lint in question is MachineApplicable because then cargo fix would try to fix those, which could generate broken code.

@workingjubilee
Copy link
Contributor

Oh, you're right, I didn't think about the actual lint mechanics.
@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 19, 2023
@workingjubilee
Copy link
Contributor

@xfix Thank you for catching that. Though I think source should work in most cases, it is still more like MachineAttemptable? The _ms functions definitely don't work out due to the type change, and I think we don't want rustfix to attempt to write unsafe code on principle.

@dima74 That knocks this down to connect, min_align_of, min_align_of_val, and lines_any, I believe.

@KamilaBorowska
Copy link
Contributor

KamilaBorowska commented Aug 19, 2023

cause to source is more like MaybeIncorrect because a type could have cause implementation, but no source implementation (this can happen with very old code). This would alter the meaning of the code.

@workingjubilee
Copy link
Contributor

Yes. It feels somewhat stronger than an "average" MaybeIncorrect, where a MaybeIncorrect may send a Rust programmer, especially a learner, down a wildly incorrect path, at least in my experience. But it is much weaker than a MachineApplicable, which should be exactly right.

@dima74
Copy link
Contributor Author

dima74 commented Aug 21, 2023

@dima74 That knocks this down to connect, min_align_of, min_align_of_val, and lines_any, I believe.

@workingjubilee fixed

@workingjubilee
Copy link
Contributor

Thank you!
@bors r+ rollup=always

@bors
Copy link
Contributor

bors commented Aug 21, 2023

📌 Commit 07b57f9 has been approved by workingjubilee

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 21, 2023
@bors
Copy link
Contributor

bors commented Aug 22, 2023

⌛ Testing commit 07b57f9 with merge 795ade0...

@bors
Copy link
Contributor

bors commented Aug 22, 2023

☀️ Test successful - checks-actions
Approved by: workingjubilee
Pushing 795ade0 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 22, 2023
@bors bors merged commit 795ade0 into rust-lang:master Aug 22, 2023
12 checks passed
@rustbot rustbot added this to the 1.74.0 milestone Aug 22, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (795ade0): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
1.6% [1.6%, 1.6%] 1
Improvements ✅
(primary)
-1.3% [-1.3%, -1.3%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -1.3% [-1.3%, -1.3%] 1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 634.918s -> 634.501s (-0.07%)
Artifact size: 346.88 MiB -> 347.06 MiB (0.05%)

@dima74 dima74 deleted the diralik/add-deprecated-suggestions branch August 22, 2023 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants