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

Implement suggestion for never type fallback lints #132383

Merged
merged 6 commits into from
Nov 2, 2024

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented Oct 31, 2024

r? @WaffleLapkin

Just opening this up for vibes; it's not done yet. I'd still like to make this suggestable in a few more cases before merge:

  • Try to annotate _ -> ()
  • Try to annotate local variables if they're un-annotated: let x = ... -> let x: () = ...
  • Try to annotate the self type of a Trait::method() -> <() as Trait>::method().

The only other case we may want to suggest is a missing turbofish, like f() -> f::<()>(). That may be possible, but seems overly annoying.

This partly addresses #132358; the other half of fixing that would be to make the error message a bit better, perhaps just special casing the ? operator 🤔 I don't think I'll do that part.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 31, 2024
@compiler-errors
Copy link
Member Author

yippee this is ready i think

@compiler-errors
Copy link
Member Author

We could, perhaps, even reuse this suggestion to tell users to annotate their code with () even after edition 2024.

Copy link
Member

@WaffleLapkin WaffleLapkin left a comment

Choose a reason for hiding this comment

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

I love the diagnostic changes, that all look very good!

I have a couple minor questions.

Comment on lines +607 to +618
// We can't turbofish consts :(
&& args.iter().all(|arg| matches!(arg.unpack(), ty::GenericArgKind::Type(_) | ty::GenericArgKind::Lifetime(_)))
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I understand? this checks that there are no const genetics, but you can turbofish those?

Copy link
Member Author

Choose a reason for hiding this comment

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

I wanted to limit the complexity of the suggestion so it would just either have _ for all the non fallback args, but consts cant use _

Copy link
Member

Choose a reason for hiding this comment

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

Ah, ic

}

/// Try to collect a useful suggestion to preserve fallback to `()`.
struct VidVisitor<'a, 'tcx> {
Copy link
Member

Choose a reason for hiding this comment

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

maybe name it daunting else? I'm not sure what tho, but something that highlights that it finds possible suggestion places...

Copy link
Member Author

Choose a reason for hiding this comment

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

Ya i can name it better and give it a pass for more comments

Copy link
Member

Choose a reason for hiding this comment

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

r=me after that then

@compiler-errors
Copy link
Member Author

@bors r=WaffleLapkin

@bors
Copy link
Contributor

bors commented Nov 1, 2024

📌 Commit b4248ae has been approved by WaffleLapkin

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-review Status: Awaiting review from the assignee but also interested parties. labels Nov 1, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 1, 2024
…k-sugg, r=WaffleLapkin

Implement suggestion for never type fallback lints

r? `@WaffleLapkin`

Just opening this up for vibes; it's not done yet. I'd still like to make this suggestable in a few more cases before merge:
- [x] Try to annotate `_` -> `()`
- [x] Try to annotate local variables if they're un-annotated: `let x = ...` -> `let x: () = ...`
- [x] Try to annotate the self type of a `Trait::method()` -> `<() as Trait>::method()`.

The only other case we may want to suggest is a missing turbofish, like `f()` -> `f::<()>()`. That may be possible, but seems overly annoying.

This partly addresses rust-lang#132358; the other half of fixing that would be to make the error message a bit better, perhaps just special casing the `?` operator 🤔 I don't think I'll do that part.
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 1, 2024
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#131829 (Remove support for `-Zprofile` (gcov-style coverage instrumentation))
 - rust-lang#132341 (Reject raw lifetime followed by `'`, like regular lifetimes do)
 - rust-lang#132369 (style-guide: Only use the new binop heuristic for assignments)
 - rust-lang#132383 (Implement suggestion for never type fallback lints)
 - rust-lang#132437 (coverage: Regression test for inlining into an uninstrumented crate)
 - rust-lang#132438 (Remove unncessary option for default rust-analyzer setting)

r? `@ghost`
`@rustbot` modify labels: rollup
jieyouxu added a commit to jieyouxu/rust that referenced this pull request Nov 1, 2024
…k-sugg, r=WaffleLapkin

Implement suggestion for never type fallback lints

r? ``@WaffleLapkin``

Just opening this up for vibes; it's not done yet. I'd still like to make this suggestable in a few more cases before merge:
- [x] Try to annotate `_` -> `()`
- [x] Try to annotate local variables if they're un-annotated: `let x = ...` -> `let x: () = ...`
- [x] Try to annotate the self type of a `Trait::method()` -> `<() as Trait>::method()`.

The only other case we may want to suggest is a missing turbofish, like `f()` -> `f::<()>()`. That may be possible, but seems overly annoying.

This partly addresses rust-lang#132358; the other half of fixing that would be to make the error message a bit better, perhaps just special casing the `?` operator 🤔 I don't think I'll do that part.
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 1, 2024
Rollup of 13 pull requests

Successful merges:

 - rust-lang#131829 (Remove support for `-Zprofile` (gcov-style coverage instrumentation))
 - rust-lang#132369 (style-guide: Only use the new binop heuristic for assignments)
 - rust-lang#132383 (Implement suggestion for never type fallback lints)
 - rust-lang#132437 (coverage: Regression test for inlining into an uninstrumented crate)
 - rust-lang#132438 (Remove unncessary option for default rust-analyzer setting)
 - rust-lang#132439 (Add `f16` and `f128` to `invalid_nan_comparison`)
 - rust-lang#132445 (Cleanup attributes around unchecked shifts and unchecked negation in const)
 - rust-lang#132448 (Add missing backtick)
 - rust-lang#132450 (Show actual MIR when MIR building forgot to terminate block)
 - rust-lang#132451 (remove some unnecessary rustc_allow_const_fn_unstable)
 - rust-lang#132455 (make const_alloc_layout feature gate only about functions that are already stable)
 - rust-lang#132456 (Move remaining inline assembly test files into asm directory)
 - rust-lang#132459 (feat(byte_sub_ptr): unstably add ptr::byte_sub_ptr)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 1, 2024
…llaumeGomez

Rollup of 14 pull requests

Successful merges:

 - rust-lang#131829 (Remove support for `-Zprofile` (gcov-style coverage instrumentation))
 - rust-lang#132369 (style-guide: Only use the new binop heuristic for assignments)
 - rust-lang#132383 (Implement suggestion for never type fallback lints)
 - rust-lang#132413 (update offset_of! docs to reflect the stabilization of nesting)
 - rust-lang#132438 (Remove unncessary option for default rust-analyzer setting)
 - rust-lang#132439 (Add `f16` and `f128` to `invalid_nan_comparison`)
 - rust-lang#132444 (rustdoc: Directly use rustc_abi instead of reexports)
 - rust-lang#132445 (Cleanup attributes around unchecked shifts and unchecked negation in const)
 - rust-lang#132448 (Add missing backtick)
 - rust-lang#132450 (Show actual MIR when MIR building forgot to terminate block)
 - rust-lang#132451 (remove some unnecessary rustc_allow_const_fn_unstable)
 - rust-lang#132455 (make const_alloc_layout feature gate only about functions that are already stable)
 - rust-lang#132456 (Move remaining inline assembly test files into asm directory)
 - rust-lang#132459 (feat(byte_sub_ptr): unstably add ptr::byte_sub_ptr)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit e31a5ca into rust-lang:master Nov 2, 2024
6 checks passed
@rustbot rustbot added this to the 1.84.0 milestone Nov 2, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Nov 2, 2024
Rollup merge of rust-lang#132383 - compiler-errors:never-type-fallback-sugg, r=WaffleLapkin

Implement suggestion for never type fallback lints

r? ```@WaffleLapkin```

Just opening this up for vibes; it's not done yet. I'd still like to make this suggestable in a few more cases before merge:
- [x] Try to annotate `_` -> `()`
- [x] Try to annotate local variables if they're un-annotated: `let x = ...` -> `let x: () = ...`
- [x] Try to annotate the self type of a `Trait::method()` -> `<() as Trait>::method()`.

The only other case we may want to suggest is a missing turbofish, like `f()` -> `f::<()>()`. That may be possible, but seems overly annoying.

This partly addresses rust-lang#132358; the other half of fixing that would be to make the error message a bit better, perhaps just special casing the `?` operator 🤔 I don't think I'll do that part.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants