-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Silence errors in expressions caused by bare traits in paths in 2021 edition #125784
base: master
Are you sure you want to change the base?
Conversation
HIR ty lowering was modified cc @fmease |
tcx.parent_hir_node(hir_ty.hir_id) | ||
{ | ||
// `tests/ui/suggestions/issue-116434-2021.rs` and others would otherwise | ||
// fail with no error beign emitted. We restrict the silencing only to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by "fail with no error [being] emitted"? In this branch we have guard : ErrorGuaranteed
, we should've emitted at least one error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some reason (which I don't understand, as it goes against my understanding of how this is meant to work), the stashed error ends up never getting emitted, which of course causes an ICE for an error being constructed but not emitted. This check does side step that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nnethercote has dug into this a while back and written a nice explainer: #120482 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love the core change (→ {type error}
) but I'm not fond of the workaround because it doesn't seem very robust and I bet it doesn't fix all pathological cases without having looked further into it.
Would you be willing to look into fixing the stash
/downgrade
issue?
I wonder if we could just ignore the request to downgrade a stashed diagnostic to a bug inside |
@fmease I see the problem:
I changed the code here in hir analysis so that we either stash or delay as bug, but never both. Removed the unnecessary check and now a few more cases get correctly silenced in the test suite. We could look to better account for downgraded diagnostics that got stashed and not ICE in that case, but for now I think it is not as bad an idea to be very noisy about that happening. That way we can actually make a determination (if annoyingly), instead of paper over it and miss potential unintended mistakes. |
These commits modify the If this was unintentional then you should revert the changes before this PR is merged. |
If reviewing all commits is too much (which is likely), feel free to review until the 4th commit and the rest can go on their own PR. |
I guess suggesting |
On a second thought, the help message All that too say, while the output on master is horrendously verbose, the output of the current PR might be too terse because it would take the user an additional edit (addition of I guess I'm fine that as long as we/you/I do follow-up work and make the suggestion more correct. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to self: I've stopped my review right before commit "Add more HIR tracking information to ObligationCauseCode". I see that you're working on sugg'ing <_ as Default>::default
. I've read your comment about splitting this PR. I don't really care as long as the history is self-explanatory.
You might want to update the PR description as well.
Regarding the actual changes I still need to review & re-review things but already have some feedback.
tests/ui/dyn-keyword/trait-missing-dyn-in-qualified-path.edition2021.stderr
Outdated
Show resolved
Hide resolved
tests/ui/resolve/issue-111312.rs
Outdated
HasNot::has(); | ||
//~^ ERROR trait objects must include the `dyn` keyword | ||
//~| ERROR no function or associated item named `has` found for trait `HasNot` | ||
<dyn HasNot>::has(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll see if I can do something in E0782 to at the resolve, but I am generally of the position that if an error leads to another error that has the right suggestion, that is reasonable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now I check if HasNot
is object safe and if the type is part of a qualified path to an associated item that didn't resolve (unresolved assoc item can be because it doesn't exist in the trait or because it exists but an appropriate self ty couldn't be found). If it is, I don't turn it into {type error}
, which makes the error about "has
not found" still be emitted. But if the trait is not object safe, we still turn it into a {type error}
to avoid any further errors. At the same time, I've added more context to the E0782 about object safety so we don't lose any information when emitting only that one.
tests/ui/resolve/issue-111727.rs
Outdated
@@ -3,5 +3,4 @@ | |||
fn main() { | |||
std::any::Any::create(); | |||
//~^ ERROR trait objects must include the `dyn` keyword |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if Some(trait_ref.def_id()) == self.tcx.lang_items().sized_trait() { | ||
if !self.tcx.object_safety_violations(trait_ref.def_id()).is_empty() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sized
is never object-safe. Why the need to check the object_safe_violations
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you maybe add a comment outlining the motivation for delaying the error in these cases for future readers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is now completely gone.
) => { | ||
return err.delay_as_bug(); | ||
} | ||
(ObligationCauseCode::SizedCallReturnType(did), _) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to note that these checks are like super specific / not really general. Cost in complexity vs. benefit.
ObligationCauseCode::WhereClauseInExpr(..), | ||
ty::Dynamic(..), | ||
) => { | ||
if let ObligationCauseCode::SizedCallReturnType(did) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I've just arrived at this commit (the 4th). Noticing you had to make this change less "aggressive" makes me think that this branch is still super fragile. Like I don't know if the delay_as_bug
is correct in all perceivable cases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All delay_as_bug
s are now gone after the "Fix tidy" commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(note to self): I haven't properly reviewed commit "Add more tracking for HIR source well-formedness requirement" yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At commit "Add tests for various dyn Trait
in path cases" right now. I think we should make sure to remove files that are duplicates of the new tests. For example rg 'cannot call associated function on trait without specifying the corresponding `impl` type' tests -t rust
gave me tests/ui/type/issue-101866.rs
, tests/ui/suggestions/issue-104328.rs
, tests/ui/suggestions/issue-104327.rs
. We might want to remove them or move them here. I haven't looked at the contents.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this specific test doesn't actually fit into dyn-keyword/
?
This comment was marked as resolved.
This comment was marked as resolved.
Address rust-lang#51077 in editions>=2021 by treating `<Trait>` as a `<{type error}>` instead of `<dyn Trait>`, silencing subsequent errors. We new emit a single error ("missing `dyn`"). ``` error[E0782]: trait objects must include the `dyn` keyword --> f800.rs:2:15 | 2 | let x: u32 = <Default>::default(); | ^^^^^^^ | help: add `dyn` keyword before this trait | 2 | let x: u32 = <dyn Default>::default(); | +++ ``` instead of 6 ``` error[E0782]: trait objects must include the `dyn` keyword --> f800.rs:2:15 | 2 | let x: u32 = <Default>::default(); | ^^^^^^^ | help: add `dyn` keyword before this trait | 2 | let x: u32 = <dyn Default>::default(); | +++ error[E0038]: the trait `Default` cannot be made into an object --> f800.rs:2:15 | 2 | let x: u32 = <Default>::default(); | ^^^^^^^ `Default` cannot be made into an object | = note: the trait cannot be made into an object because it requires `Self: Sized` = note: for a trait to be "object safe" it needs to allow building a vtable to allow the call to be resolvable dynamically; for more information visit <https://doc.rust-lang.org/reference/items/traits.html#object-safety> error[E0277]: the size for values of type `dyn Default` cannot be known at compilation time --> f800.rs:2:15 | 2 | let x: u32 = <Default>::default(); | ^^^^^^^ doesn't have a size known at compile-time | = help: the trait `Sized` is not implemented for `dyn Default` note: required by a bound in `default` --> /home/gh-estebank/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/default.rs:104:20 | 104 | pub trait Default: Sized { | ^^^^^ required by this bound in `Default::default` ... 136 | fn default() -> Self; | ------- required by a bound in this associated function error[E0308]: mismatched types --> f800.rs:2:14 | 2 | let x: u32 = <Default>::default(); | --- ^^^^^^^^^^^^^^^^^^^^ expected `u32`, found `dyn Default` | | | expected due to this | = note: expected type `u32` found trait object `dyn Default` = help: `u32` implements `Default` so you could change the expected type to `Box<dyn Default>` error[E0038]: the trait `Default` cannot be made into an object --> f800.rs:2:14 | 2 | let x: u32 = <Default>::default(); | ^^^^^^^^^^^^^^^^^^^^ `Default` cannot be made into an object | = note: the trait cannot be made into an object because it requires `Self: Sized` = note: for a trait to be "object safe" it needs to allow building a vtable to allow the call to be resolvable dynamically; for more information visit <https://doc.rust-lang.org/reference/items/traits.html#object-safety> error[E0277]: the size for values of type `dyn Default` cannot be known at compilation time --> f800.rs:2:14 | 2 | let x: u32 = <Default>::default(); | ^^^^^^^^^^^^^^^^^^^^ doesn't have a size known at compile-time | = help: the trait `Sized` is not implemented for `dyn Default` = note: the return type of a function must have a statically known size ```
When encountering an unsized E0277 error on a method that returns `Self` silence that error as it a prior unsized error will have been emitted when constructing the value of `Self`. On E0227 unsized error caused on a bare trait associated function with object safety violations, silence it.
…nce more redundant errors Instead of calling `dowgrade_to_delayed_bug` and then stashing a diagnostic, we either stash the diagnostic or we `delay_as_bug` it. Remove unnecessary restriction of silencing of knock down errors on bare trait objects in edition >=2021.
This data will help with extending E0038 object safety errors.
We use the new HIR node tracking for well-formedness obligations to point at the specific trait object in a path/type (`dyn T` instead of a whole `Box<dyn T>`, for example). ``` error[E0038]: the trait `MapLike` cannot be made into an object --> $DIR/file.rs:47:16 | LL | as Box<dyn Trait>; | ^^^^^^^^^ `Trait` cannot be made into an object ``` We also provide a structured suggestion for `<dyn Trait>::assoc`: ``` = help: when writing `<dyn Trait>::function` you are requiring `Trait` be "object safe", which it isn't help: you might have meant to access the associated function of a specific `impl` to avoid requiring "object safety" from `Trait`, either with some explicit type... | LL | </* Type */ as Trait>::function(); | ~~~~~~~~~~~~~ help: ...or rely on inference if the compiler has enough context to identify the desired type on its own... | LL - <dyn Trait>::function(); LL + Trait::function(); | help: ...which is equivalent to | LL | <_ as Trait>::function(); | ~~~~ ```
…often On casts to `dyn Trait` caused by explicit `as` or return type, we point at the type instead of the expression. Silence note with context about the cast when we'd point at the type, which allows automatic deduplication to happen.
This comment has been minimized.
This comment has been minimized.
The last two commits remove the two |
This comment was marked as resolved.
This comment was marked as resolved.
Given `Trait::foo()` in edition 2021 where `Trait` is object safe and `foo` isn't resolved, emit E0782 instead of marking the expression as `{type error}`, as `foo` not being part of `Trait` is more useful information. If `Trait` *isn't* object safe, then the resolve error isn't as useful because E0782 will have more actionable feedback.
This comment was marked as resolved.
This comment was marked as resolved.
``` error[E0782]: trait objects must include the `dyn` keyword --> $DIR/not-on-bare-trait-2021.rs:8:12 | LL | fn foo(_x: Foo + Send) { | ^^^^^^^^^^ | help: use a new generic type parameter, constrained by `Foo + Send` | LL | fn foo<T: Foo + Send>(_x: T) { | +++++++++++++++ ~ help: you can also use an opaque type, but users won't be able to specify the type parameter when calling the `fn`, having to rely exclusively on type inference | LL | fn foo(_x: impl Foo + Send) { | ++++ help: alternatively, use a trait object to accept any type that implements `Foo + Send`, accessing its methods at runtime using dynamic dispatch | LL | fn foo(_x: &(dyn Foo + Send)) { | +++++ + - help: add `dyn` keyword before this trait - | - LL | fn foo(_x: dyn Foo + Send) { - | +++ ```
``` error[E0782]: trait objects must include the `dyn` keyword --> $DIR/issue-116434-2021.rs:5:17 | LL | type Clone; | ---------- you might have meant to use this associated type LL | fn foo() -> Clone; | ^^^^^ | help: `Clone` is not object safe, use `impl Clone` to return an opaque type, as long as you return a single underlying type | LL | fn foo() -> impl Clone; | ++++ help: there is an associated type with the same name | LL | fn foo() -> Self::Clone; | ++++++ ```
When encountering `static FOO: Trait = ...`, if `Trait` is object safe, suggest `static FOO: Box<dyn Trait> = ...`.
@@ -62,7 +39,7 @@ help: you might have meant to write a bound here | |||
LL | ) -> impl Iterator<Item: SubAssign> { | |||
| ~ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pre-existing, I feel that if we wait long enough to fix it, the syntax will be valid in stable 😈
This comment has been minimized.
This comment has been minimized.
This comment was marked as resolved.
This comment was marked as resolved.
☔ The latest upstream changes (presumably #126439) made this pull request unmergeable. Please resolve the merge conflicts. |
Address #51077 in editions>=2021 by treating
<Trait>
as a<{type error}>
instead of<dyn Trait>
, silencing subsequent errors. We now emit a single error ("missingdyn
").instead of 6
In all editions, whenever we encounter
!Sized
E0277 and object safety E0038 errors for the same expression, silence the former: