Lint bare traits in AstConv. #89090
Lint bare traits in AstConv. #89090
Conversation
rust-highfive
added
the
S-waiting-on-review
| --> $DIR/bad-assoc-ty.rs:33:10 | ||
| | | ||
| LL | type H = Fn(u8) -> (u8)::Output; | ||
| | ^^^^^^ help: use `dyn`: `<dyn Fn(u8)>` |
There was a problem hiding this comment.
This one makes me uncomfortable, but I could not find how to fix it. Suggestions?
There was a problem hiding this comment.
This is outright incorrect, right? The whole type is <dyn Fn(u8) -> (u8)::Output>, if I'm reading this correctly. I think we can special case Fn traits in this format by looking at the poly_trait_ref.
It's also interesting that the error below is already making some suggestions, so it might make sense to only emit this one if E0233 isn't (but this might not be needed in this PR).
| self_ty.span, | ||
| |lint| { | ||
| let mut db = lint.build(msg); | ||
| db.span_suggestion(self_ty.span, "use `dyn`", sugg, app); |
There was a problem hiding this comment.
Can we make this a multipart_suggestion? We previously only highlighted the change to add dyn in most cases, now we underline the whole type, which is fine, but the more targeted spans aid in understanding what the change is supposed to be. Making that change requires instead of dealing with a string for the suggestion to deal with a vec of changes.
There was a problem hiding this comment.
I don't really understand what kind of output you would like to see.
| --> $DIR/bad-assoc-ty.rs:33:10 | ||
| | | ||
| LL | type H = Fn(u8) -> (u8)::Output; | ||
| | ^^^^^^ help: use `dyn`: `<dyn Fn(u8)>` |
There was a problem hiding this comment.
This is outright incorrect, right? The whole type is <dyn Fn(u8) -> (u8)::Output>, if I'm reading this correctly. I think we can special case Fn traits in this format by looking at the poly_trait_ref.
It's also interesting that the error below is already making some suggestions, so it might make sense to only emit this one if E0233 isn't (but this might not be needed in this PR).
| //~^ ERROR trait objects without an explicit `dyn` are deprecated | ||
| //~| WARN this is accepted in the current edition |
There was a problem hiding this comment.
These shouldn't go away, right?
There was a problem hiding this comment.
The type checker skips looking at bodies if there is an error while checking signatures. As a consequence, no error is emitted on the locals because there is some on the parameter types.
On 2021 edition bare traits are a hard error, so we must always report them regardless of their positions. |
This comment has been minimized.
This comment has been minimized.
cjgillot
added
S-waiting-on-author
This comment has been minimized.
This comment has been minimized.
apiraino
added
the
T-compiler
cjgillot
added
S-waiting-on-review
|
☔ The latest upstream changes (presumably #89124) made this pull request unmergeable. Please resolve the merge conflicts. |
|
☔ The latest upstream changes (presumably #90883) made this pull request unmergeable. Please resolve the merge conflicts. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@estebank, I introduced the multipart suggestions. Do you still have unresolved remarks on this PR? |
|
@bors r+ |
|
📌 Commit 6fcbc3a has been approved by |
bors
added
S-waiting-on-bors
…askrgr Rollup of 7 pull requests Successful merges: - rust-lang#87901 (Fix suggestion of additional `pub` when using `pub pub fn ...`) - rust-lang#89090 (Lint bare traits in AstConv.) - rust-lang#91818 (Show the unused type for `unused_results` lint) - rust-lang#91910 (miri: lift restriction on extern types being the only field in a struct) - rust-lang#91928 (Constify (most) `Option` methods) - rust-lang#91975 (Move generator check earlier in inlining.) - rust-lang#92016 (builtin_macros: allow external consumers for AsmArgs parsing) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
Removing the lint from lowering allows to:
r? @estebank