forbid dyn Trait in patterns #71038
Merged
forbid dyn Trait in patterns #71038
Conversation
This file contains hidden or 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 
lcnr force-pushed the dyn_trait_structural_match branch from 21185be to b373917 Compare 
| IMO this PR's title/description should focus on pattern-matching. r? @pnkfelix |
eddyb reviewed Apr 11, 2020
eddyb reviewed Apr 11, 2020
eddyb reviewed Apr 11, 2020
| 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 |
eddyb reviewed Apr 12, 2020
Comment on lines 147 to +159
There was a problem hiding this comment.
Doesn't this stop your ICE as well?
There was a problem hiding this comment.
nope. ty_is_partial_eq is true for &dyn PartialEq<u32>. It is fixed for all other traits though.
This fails correctly
const F: &'static dyn Send = &7u32; fn main() { let a: &dyn Send = &7u32; match a { F => panic!(), //~^ ERROR trait object cannot be used in patterns _ => {} } }There was a problem hiding this comment.
There was a problem hiding this comment.
I changed the test to dyn Send and rebased.
Don't know how to test for an ICE, we could just open an issue for dyn PartialEq after this is merged (which is then added to glacier).
lcnr force-pushed the dyn_trait_structural_match branch from 460b8c4 to 1c4fc95 Compare | 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 |
lcnr force-pushed the dyn_trait_structural_match branch from 1c4fc95 to 4549ef5 Compare 
joelpalmer added S-waiting-on-review 
Dylan-DPC-zz added S-waiting-on-author 
| @pnkfelix this is ready for review |
pnkfelix reviewed Apr 28, 2020
| impl A for B {} | ||
| | ||
| fn test<const T: &'static dyn A>() { | ||
| //~^ ERROR the types of const generic parameters must derive `PartialEq` and `Eq` |
There was a problem hiding this comment.
The PR seems fine and I'm planning to r+ it, but I wanted to ask about this part:
Do you see the error here as being the absence of PartialEq and Eq, as indicated by the diagnostic? Or is it that any use of dyn A will be broken here?
I personally suspect that, unless we expose a StructuralMatch marker trait (at which point maybe one would be able to do trait A: StructuralMatch {}), the user error here is in the use of dyn A, and therefore the diagnostic message in this case is misleading.
But I'm willing to deal with that in a followup patch.
There was a problem hiding this comment.
Not sure tbh, I think that we should allow something like trait A: StructuralMatch {} in the future.
cc @eddyb i guess
| @bors r+ rollup |
| 📌 Commit 4549ef5 has been approved by |
bors added S-waiting-on-bors Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Apr 28, 2020
…pnkfelix forbid `dyn Trait` in patterns Do not allow `&dyn Trait` as a generic const parameters. This also changes dyn trait in pattern from ICE to error. closes rust-lang#63322 closes rust-lang#70972 r? @eddyb
| failed in rollup @bors r- |
bors added S-waiting-on-author lcnr force-pushed the dyn_trait_structural_match branch from 4549ef5 to ecf574f Compare 
| fixed |
| ☔ The latest upstream changes (presumably #67343) made this pull request unmergeable. Please resolve the merge conflicts. |
lcnr force-pushed the dyn_trait_structural_match branch from ecf574f to 2f5c0f5 Compare | @pnkfelix This should be ready for merge afaict. |
| @bors r+ |
| 📌 Commit 2f5c0f5 has been approved by |
bors added S-waiting-on-bors bors added a commit to rust-lang-ci/rust that referenced this pull request May 4, 2020
Rollup of 5 pull requests Successful merges: - rust-lang#71038 (forbid `dyn Trait` in patterns) - rust-lang#71697 (Added MIR constant propagation of Scalars into function call arguments) - rust-lang#71773 (doc: misc rustdoc things) - rust-lang#71810 (Do not try to find binop method on RHS `TyErr`) - rust-lang#71877 (Use f64 in f64 examples) Failed merges: r? @ghost
RalfJung reviewed May 5, 2020
16 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
9 participants
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.
Do not allow
&dyn Traitas a generic const parameters.This also changes dyn trait in pattern from ICE to error.
closes #63322
closes #70972
r? @eddyb