trait-based structural match implementation #65519
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 has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
(I do have other experimental work that goes further in fixing bugs in our structural-match system. However, I decided this piece of the puzzle was worth landing on its own, since it is both broad in the amount of code it touches, but very narrow in its actual user-visible effects...) |
|
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 latest upstream changes (presumably #65495) made this pull request unmergeable. Please resolve the merge conflicts. |
Centril
reviewed
Oct 18, 2019
|
cc @varkor |
(Hmm apparently I need to include a few extra suites in my local test runs.) |
|
One thing we probably will want is a perf run on this. I do not know if there might be an impact from the way I implemented (If necessary, we could batch up the obligations and try to solve them all at once, but its tricky because we want to have hard errors in some cases and lints in others, due to future-compatibility concerns. That trickiness is why I opted to take this approach first.) |
pnkfelix
force-pushed
the
issue-63438-trait-based-structural-match
branch
from
f4626f6
to
17231a0
Compare
varkor
reviewed
Oct 20, 2019
varkor
reviewed
Oct 20, 2019
| // keep old code until future-compat upgraded to errors. | ||
| ty::Ref(_, adt_ty @ ty::TyS { kind: ty::Adt(_, _), .. }, _) | ||
| if !self.type_marked_structural(adt_ty) => { | ||
| let adt_def = if let ty::Adt(adt_def, _) = adt_ty.kind { |
There was a problem hiding this comment.
The identation here is a bit confusing
There was a problem hiding this comment.
heh, this is the bit of code that inspired me to file #65490
|
☔ The latest upstream changes (presumably #65632) made this pull request unmergeable. Please resolve the merge conflicts. |
pnkfelix
force-pushed
the
issue-63438-trait-based-structural-match
branch
from
dceb5e3
to
ee61fd1
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 |
hmm, whoopsie. |
|
@bors r+ |
|
📌 Commit c7da944105fd62d29f0378c20346ca1994b4a84c has been approved by |
|
🌲 The tree is currently closed for pull requests below priority 1000, this pull request will be tested once the tree is reopened |
bors
added
S-waiting-on-bors
|
☔ The latest upstream changes (presumably #65771) made this pull request unmergeable. Please resolve the merge conflicts. |
bors
removed
the
S-waiting-on-bors
bors
added
the
S-waiting-on-author
bors
added a commit
that referenced
this pull request
Oct 25, 2019
Rollup of 9 pull requests Successful merges: - #62959 (Add by-value iterator for arrays ) - #65390 (Add long error explanation for E0576) - #65408 (reorder config.toml.example options and add one missing option) - #65414 (ignore uninhabited non-exhaustive variant fields) - #65666 (Deprecated proc_macro doesn't trigger warning on build library) - #65742 (Pre-expansion gate most of the things) - #65747 (Adjust the tracking issue for `untagged_unions`.) - #65763 (Changed APIT with explicit generic args span to specific arg spans) - #65775 (Fix more `ReEmpty` ICEs) Failed merges: - #65519 (trait-based structural match implementation) r? @ghost
(Or more precisely, a pair of such traits: one for `derive(PartialEq)` and one for `derive(Eq)`.) ((The addition of the second marker trait, `StructuralEq`, is largely a hack to work-around `fn (&T)` not implementing `PartialEq` and `Eq`; see also issue rust-lang#46989; otherwise I would just check if `Eq` is implemented.)) Note: this does not use trait fulfillment error-reporting machinery; it just uses the trait system to determine if the ADT was tagged or not. (Nonetheless, I have kept an `on_unimplemented` message on the new trait for structural_match check, even though it is currently not used.) Note also: this does *not* resolve the ICE from rust-lang#65466, as noted in a comment added in this commit. Further work is necessary to resolve that and other problems with the structural match checking, especially to do so without breaking stable code (adapted from test fn-ptr-is-structurally-matchable.rs): ```rust fn r_sm_to(_: &SM) {} fn main() { const CFN6: Wrap<fn(&SM)> = Wrap(r_sm_to); let input: Wrap<fn(&SM)> = Wrap(r_sm_to); match Wrap(input) { Wrap(CFN6) => {} Wrap(_) => {} }; } ``` where we would hit a problem with the strategy of unconditionally checking for `PartialEq` because the type `for <'a> fn(&'a SM)` does not currently even *implement* `PartialEq`. ---- added review feedback: * use an or-pattern * eschew `return` when tail position will do. * don't need fresh_expansion; just add `structural_match` to appropriate `allow_internal_unstable` attributes. also fixed example in doc comment so that it actually compiles.
(My inference is that the number changed from 4 to 5 because `derive(PartialEq)` now injects an extra trait impl before.)
pnkfelix
force-pushed
the
issue-63438-trait-based-structural-match
branch
from
c7da944
to
f645e90
Compare
|
@bors r+ |
|
📌 Commit f645e90 has been approved by |
|
🌲 The tree is currently closed for pull requests below priority 1000, this pull request will be tested once the tree is reopened |
bors
added
S-waiting-on-bors
bors
added a commit
that referenced
this pull request
Oct 27, 2019
…tch, r=matthewjasper trait-based structural match implementation Moves from using a `#[structural_match]` attribute to using a marker trait (or pair of such traits, really) instead. Fix #63438. (This however does not remove the hacks that I believe were put into place to support the previous approach of injecting the attribute based on the presence of both derives... I have left that for follow-on work.)
|
☀️ Test successful - checks-azure |
Centril
added a commit
to Centril/rust
that referenced
this pull request
Nov 5, 2019
…trochenkov Remove `PartialEq` and `Eq` from the `SpecialDerives`. Now that PR rust-lang#65519 landed, this is the follow-on work of removing `PartialEq` and `Eq` from the set of `SpecialDerives` .
Centril
added a commit
to Centril/rust
that referenced
this pull request
Nov 6, 2019
…trochenkov Remove `PartialEq` and `Eq` from the `SpecialDerives`. Now that PR rust-lang#65519 landed, this is the follow-on work of removing `PartialEq` and `Eq` from the set of `SpecialDerives` .
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.
Moves from using a
#[structural_match]attribute to using a marker trait (or pair of such traits, really) instead.Fix #63438.
(This however does not remove the hacks that I believe were put into place to support the previous approach of injecting the attribute based on the presence of both derives... I have left that for follow-on work.)