Uh oh!
There was an error while loading. Please reload this page.
forbid dyn Trait in patterns - #71038
Merged
Merged
Conversation
lcnrforce-pushed
the
dyn_trait_structural_match
branch
from
April 11, 2020 20:12
21185be to
b373917CompareContributor
IMO this PR's title/description should focus on pattern-matching. r? @pnkfelix |
eddyb
reviewed
Apr 11, 2020
Uh oh!
There was an error while loading. Please reload this page.
eddyb
reviewed
Apr 11, 2020
Uh oh!
There was an error while loading. Please reload this page.
eddyb
reviewed
Apr 11, 2020
Uh oh!
There was an error while loading. Please reload this page.
rust-highfive
commented
Apr 11, 2020
Contributor
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
Contributor
There was a problem hiding this comment.
Doesn't this stop your ICE as well?
ContributorAuthor
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
constF:&'staticdynSend = &7u32;fnmain(){let a:&dynSend = &7u32;match a {F => panic!(),//~^ ERROR trait object cannot be used in patterns
_ => {}}}Contributor
There was a problem hiding this comment.
ContributorAuthor
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).
lcnrforce-pushed
the
dyn_trait_structural_match
branch
from
April 12, 2020 13:34
460b8c4 to
1c4fc95Comparerust-highfive
commented
Apr 12, 2020
Contributor
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 |
lcnrforce-pushed
the
dyn_trait_structural_match
branch
from
April 12, 2020 14:57
1c4fc95 to
4549ef5CompareDylan-DPC-zz
commented
Apr 21, 2020
@pnkfelix this is ready for review |
| impl A for B {} | ||
| fn test<const T: &'static dyn A>() { | ||
| //~^ ERROR the types of const generic parameters must derive `PartialEq` and `Eq` |
Contributor
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.
ContributorAuthor
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
pnkfelix
commented
Apr 28, 2020
Contributor
@bors r+ rollup |
bors
commented
Apr 28, 2020
Collaborator
📌 Commit 4549ef5 has been approved by |
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. closesrust-lang#63322closesrust-lang#70972 r? @eddyb
Dylan-DPC-zz
commented
Apr 28, 2020
failed in rollup @bors r- |
lcnrforce-pushed
the
dyn_trait_structural_match
branch
from
April 29, 2020 10:02
4549ef5 to
ecf574fComparelcnr
commented
Apr 29, 2020
ContributorAuthor
fixed |
bors
commented
Apr 29, 2020
Collaborator
☔ The latest upstream changes (presumably #67343) made this pull request unmergeable. Please resolve the merge conflicts. |
lcnrforce-pushed
the
dyn_trait_structural_match
branch
from
April 29, 2020 17:34
ecf574f to
2f5c0f5Comparelcnr
commented
May 3, 2020
ContributorAuthor
@pnkfelix This should be ready for merge afaict. |
pnkfelix
commented
May 4, 2020
Contributor
@bors r+ |
bors
commented
May 4, 2020
Collaborator
📌 Commit 2f5c0f5 has been approved by |
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
Uh oh!
There was an error while loading. Please reload this page.
16 tasks
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
Sign up for freeto 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