add dyn to display of dynamic (trait) types
#51104
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rust-highfive
added
the
S-waiting-on-review
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Legit test failures. |
sanxiyn
added
S-waiting-on-author
|
@zackmdavis (btw, |
nikomatsakis
requested changes
May 31, 2018
There was a problem hiding this comment.
This seems... basically right? The main thing I see is the precedence issue, but perhaps I am overlooking some subtlety? (cc @eddyb @petrochenkov)
src/librustc/util/ppaux.rs
Outdated
| @@ -1063,6 +1063,7 @@ define_print! { | |||
| TyParam(ref param_ty) => write!(f, "{}", param_ty), | |||
| TyAdt(def, substs) => cx.parameterized(f, substs, def.did, &[]), | |||
| TyDynamic(data, r) => { | |||
| write!(f, "dyn ")?; | |||
There was a problem hiding this comment.
Hmm, I think this may do the wrong thing in terms of precedence in some cases. For example, &dyn Foo + 'a needs some parens, right? I'm not 100% sure how this is handled already, since the same applies to &Foo + 'a... it kind of looks like we just get it wrong now?
Maybe we can concoct a test case to demonstrate this...
|
Oh, I sort of missed this:
Or at least forgot about it. Let me look at that specifically. |
So, all of that eventually ends up probably in the |
|
Right, right. Let me take a look. I forget how it works now; I know I have had at least one branch that tried to refactor precisely this point but I think it never landed (was part of a lazy norm push on my part)... |
nikomatsakis
reviewed
Jun 5, 2018
src/test/ui/const-unsized.stderr
Outdated
| @@ -16,13 +16,13 @@ LL | const CONST_FOO: str = *"foo"; | |||
| = help: the trait `std::marker::Sized` is not implemented for `str` | |||
| = note: constant expressions must have a statically known size | |||
|
|
|||
| error[E0277]: the trait bound `std::fmt::Debug + std::marker::Sync + 'static: std::marker::Sized` is not satisfied | |||
| error[E0277]: the trait bound `dyn std::fmt::Debug + std::marker::Sync + 'static: std::marker::Sized` is not satisfied | |||
There was a problem hiding this comment.
I have to admit — tracing through ppaux.rs — I couldn't quite tel how this arises. Can somebody clarify for me? I imagine that this message is produced from traits/error_reporting.rs:
rust/src/librustc/traits/error_reporting.rs
Lines 598 to 601 in 90f34b5
But it seems like that is printing a PolyTraitRef, and I'm not clear on the path that leads from that code to the TyDynamic code. What am I missing?
There was a problem hiding this comment.
The TyDynamic is the Self type, printed by this:
rust/src/librustc/util/ppaux.rs
Line 1258 in 4a9c58c
There was a problem hiding this comment.
Oh; in that case, this seems...correct.
There was a problem hiding this comment.
it might be nicer to print "the trait Sized is not implemented for dyn ...", like we used to, but that seems separate
|
@zackmdavis so specifically which cases were worrying you? It seems like (apart from the matter of |
The message that says "the trait bound |
|
@zackmdavis IMO it's fine, since "the trait bound |
|
Oh, right! 😳 OK, so then the only things left to do are:
—which I should get to early next week. |
|
☔ The latest upstream changes (presumably #51042) made this pull request unmergeable. Please resolve the merge conflicts. |
|
(note for future triage: the author has not so much time for rust at the moment, let's keep this open though #51149 (comment)) |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
@nikomatsakis updated to add parentheses when there's a |
|
☔ The latest upstream changes (presumably #51463) made this pull request unmergeable. Please resolve the merge conflicts. |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The `dyn Trait` syntax was stabilized in 199ee32. Resolves rust-lang#49277.
|
@nikomatsakis rebased 🏁 |
|
@bors r+ |
|
📌 Commit 4b18085 has been approved by |
bors
added
S-waiting-on-bors
|
⌛ Testing commit 4b18085 with merge 9b4bd1dfe1d2889a24b16f6f4a62dc13ba254c8b... |
|
💔 Test failed - status-travis |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
bors
added
S-waiting-on-review
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
bors
added
S-waiting-on-bors
pietroalbini
added a commit
to pietroalbini/rust
that referenced
this pull request
Jun 26, 2018
add `dyn ` to display of dynamic (trait) types
~~I'm not sure we want the `dyn` in the E0277 "trait bound [...] is not satisfied" messages ("bound" sounds like a different thing in contrast to the names of specific trait-object types like `Box<dyn Trait>`), but I'm finding the code I would need to change that hard to follow—the [display object seems to](https://github.com/rust-lang/rust/blob/f0805a4421449bd6fe3096d63820fbebe2bfcd1d/src/librustc/traits/error_reporting.rs#L600) be a [`Predicate::Trait`](https://github.com/rust-lang/rust/blob/f0805a4421449bd6fe3096d63820fbebe2bfcd1d/src/librustc/ty/mod.rs#L962) variant, whose [`Display` implementation](https://github.com/rust-lang/rust/blob/f0805a4421449bd6fe3096d63820fbebe2bfcd1d/src/librustc/util/ppaux.rs#L1309) calls `.print` on its `PolyTraitPredicate` member, [which is a type alias](https://github.com/rust-lang/rust/blob/f0805a4421449bd6fe3096d63820fbebe2bfcd1d/src/librustc/ty/mod.rs#L1112) for `ty::Binder<TraitPredicate<'tcx>>`, whose [`Display` implementation](https://github.com/rust-lang/rust/blob/f0805a4421449bd6fe3096d63820fbebe2bfcd1d/src/librustc/util/ppaux.rs#L975-L985) ... _&c._— so maybe it's time to pull-request this and see what reviewers think.~~
Resolves rust-lang#49277 (?).
r? @nikomatsakis
bors
added a commit
that referenced
this pull request
Jun 26, 2018
Rollup of 11 pull requests Successful merges: - #51104 (add `dyn ` to display of dynamic (trait) types) - #51153 (Link panic and compile_error docs) - #51642 (Fix unknown windows build) - #51730 (New safe associated functions for PinMut) - #51731 (Fix ICEs when using continue as an array length inside closures (inside loop conditions)) - #51747 (Add error for using null characters in #[export_name]) - #51769 (Update broken rustc-guide links) - #51786 (Remove unnecessary stat64 pointer casts) - #51788 (Fix typo) - #51789 (Don't ICE when performing `lower_pattern_unadjusted` on a `TyError`) - #51791 (Minify css) Failed merges: r? @ghost
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I'm not sure we want thedynin the E0277 "trait bound [...] is not satisfied" messages ("bound" sounds like a different thing in contrast to the names of specific trait-object types likeBox<dyn Trait>), but I'm finding the code I would need to change that hard to follow—the display object seems to be aPredicate::Traitvariant, whoseDisplayimplementation calls.printon itsPolyTraitPredicatemember, which is a type alias forty::Binder<TraitPredicate<'tcx>>, whoseDisplayimplementation ... &c.— so maybe it's time to pull-request this and see what reviewers think.Resolves #49277 (?).
r? @nikomatsakis