Skip to content
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

Introduce pat_adjustments table #1

Closed
wants to merge 2 commits into from
Closed

Introduce pat_adjustments table #1

wants to merge 2 commits into from

Conversation

tbg
Copy link
Owner

@tbg tbg commented Aug 25, 2017

wip: implement pattern-binding-modes RFC

See the RFC and tracking issue.

This is a work in progress. The basic examples from the RFC are in place and work,
but there are still some obvious deficiencies (some of which documented through
currently problematic tests).

Commentary is sparse. This will be improved before merging.


This change is Reviewable

@tbg
Copy link
Owner Author

tbg commented Aug 25, 2017

Review status: 0 of 6 files reviewed at latest revision, 6 unresolved discussions.


src/librustc/ty/sty.rs, line 160 at r1 (raw file):
@nikomatsakis wrote:

I don't really understand what you are concerned about in this line of code.
here, Thing means "some generic type"?
shouldn't be a problem, I don't think, because when we type-check, we don't know what T is, so we would decide (in this case) not to insert a deref etc
i.e., in the desugared version, after type-check, match t { _ => (), } would be unchanged


src/librustc_const_eval/pattern.rs, line 306 at r1 (raw file):
@nikomatsakis wrote:

the comment on this line sounds about right. And this loop [below] also seems about right; to adjust ty, you can simply match on it --
it must be a TyRef or else something went wrong somewhere
so you could do like ty = match ty.sty { TyRef(_, mt) => mt.ty, _ => bug!() };


src/librustc_typeck/check/_match.rs, line 167 at r1 (raw file):

            }
            PatKind::Path(ref qpath) => {
                // FIXME(tschottdorf): careful about const ref types.

@nikomatsakis here I'm just pointing out that it is where we'll end up for const CONST_REF_TYPE: &[u8] = b"foo"; match something { CONST_REF_TYPE => ... }. I.e. if the qpath comes from such a const then we must treat it as a ref pattern and can't use default binding modes. (Right?).


src/librustc_typeck/check/_match.rs, line 631 at r1 (raw file):
@nikomatsakis wrote:

this seems right, but I suspect that the code will not go in check_pat_path() but rather somewhere more general-- that is, I think we want to insert an autoderef whenever:
the type is & (TyRef)
the pat is one of those enumerated kinds that cannot be a tuple (e.g., (), non-binding path, ...)
I forget where we resolve whether a path is a binding or not
I think before type-check though


src/librustc_typeck/check/_match.rs, line 684 at r1 (raw file):
@nikomatsakis wrote:

similarly, on this line, I don't really expect there to be many changes needed -- that is, I don't think that check_pat_tuple_struct() itself will have to change much. I expect the adaptation to happen at the top of check_pat_arg()


src/librustc_typeck/check/_match.rs, line 695 at r1 (raw file):
@nikomatsakis wrote:

my expectation is that check_pat_arg() will grow a 4th argument


Comments from Reviewable

@tbg
Copy link
Owner Author

tbg commented Aug 25, 2017

Made some progress writing code that I think is "roughly correct" to populate pat_adjustments. Let me know what you think!


Review status: 0 of 5 files reviewed at latest revision, 7 unresolved discussions.


src/librustc_const_eval/pattern.rs, line 318 at r1 (raw file):

                // Probably involves ty::TypeVariants::TyRef.
                //
                // NB: trying to stuff new things into the HIR is probably a bad idea.

What do you think about this, @nikomatsakis? In particular, I don't know how to generate a new id and hir_id.


src/librustc_typeck/check/_match.rs, line 631 at r1 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

@nikomatsakis wrote:

this seems right, but I suspect that the code will not go in check_pat_path() but rather somewhere more general-- that is, I think we want to insert an autoderef whenever:
the type is & (TyRef)
the pat is one of those enumerated kinds that cannot be a tuple (e.g., (), non-binding path, ...)
I forget where we resolve whether a path is a binding or not
I think before type-check though

Ok. How do I find out? I'm not sure what to look for.


src/librustc_typeck/check/_match.rs, line 695 at r1 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

@nikomatsakis wrote:

my expectation is that check_pat_arg() will grow a 4th argument

Did the plumbing. Please check that I terminated it correctly - the binding mode begins in check_match and check_local_decl as ty::BindingMode::BindByValue(hir::Mutability::MutImmutable). I think this roughly corresponds to let <pat> = ... and match x { <pats> }. Am I somehow missing other things that want to match patterns?


src/librustc/ty/sty.rs, line 160 at r1 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

@nikomatsakis wrote:

I don't really understand what you are concerned about in this line of code.
here, Thing means "some generic type"?
shouldn't be a problem, I don't think, because when we type-check, we don't know what T is, so we would decide (in this case) not to insert a deref etc
i.e., in the desugared version, after type-check, match t { _ => (), } would be unchanged

Ack, removed.


Comments from Reviewable

@nikomatsakis
Copy link

LGTM


Review status: 0 of 5 files reviewed at latest revision, 7 unresolved discussions.


src/librustc_const_eval/pattern.rs, line 318 at r1 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

What do you think about this, @nikomatsakis? In particular, I don't know how to generate a new id and hir_id.

I think the thing to do here is not construct new HIR, but rather to just change the pattern that gets returned. I think what I would expect is that we do not generate a new pat, but rather we dereference ty once for each autoderef, and then, at the end, we take the Pattern<'tcx> that would have been returned, and wrap it in various PatternKind::Deref wrappers. Does that make sense?


src/librustc_typeck/check/_match.rs, line 167 at r1 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

@nikomatsakis here I'm just pointing out that it is where we'll end up for const CONST_REF_TYPE: &[u8] = b"foo"; match something { CONST_REF_TYPE => ... }. I.e. if the qpath comes from such a const then we must treat it as a ref pattern and can't use default binding modes. (Right?).

yes -- I think that's right


src/librustc_typeck/check/_match.rs, line 631 at r1 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

Ok. How do I find out? I'm not sure what to look for.

based on the function check_pat_path in _match.rs, it looks like PatKind::Path refers to a const variant (e.g., None) or a constant


src/librustc_typeck/check/_match.rs, line 695 at r1 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

Did the plumbing. Please check that I terminated it correctly - the binding mode begins in check_match and check_local_decl as ty::BindingMode::BindByValue(hir::Mutability::MutImmutable). I think this roughly corresponds to let <pat> = ... and match x { <pats> }. Am I somehow missing other things that want to match patterns?

Looks...pretty good to me!


Comments from Reviewable

@tbg
Copy link
Owner Author

tbg commented Sep 14, 2017

Ok, I think this is now functional enough to open a PR on the main repo. What do you think, @nikomatsakis?


Review status: 0 of 8 files reviewed at latest revision, 7 unresolved discussions.


src/librustc_const_eval/pattern.rs, line 318 at r1 (raw file):

Previously, nikomatsakis (Niko Matsakis) wrote…

I think the thing to do here is not construct new HIR, but rather to just change the pattern that gets returned. I think what I would expect is that we do not generate a new pat, but rather we dereference ty once for each autoderef, and then, at the end, we take the Pattern<'tcx> that would have been returned, and wrap it in various PatternKind::Deref wrappers. Does that make sense?

Done.


src/librustc_typeck/check/_match.rs, line 167 at r1 (raw file):

Previously, nikomatsakis (Niko Matsakis) wrote…

yes -- I think that's right

Haven't done this yet, but the comment is still in the code, so I'll get around to it.


src/librustc_typeck/check/_match.rs, line 631 at r1 (raw file):

Previously, nikomatsakis (Niko Matsakis) wrote…

based on the function check_pat_path in _match.rs, it looks like PatKind::Path refers to a const variant (e.g., None) or a constant

Done.


Comments from Reviewable

@tbg tbg force-pushed the pat_adjustments branch 2 times, most recently from 55c2434 to bb34d50 Compare September 14, 2017 14:09
@nikomatsakis
Copy link

Reviewed 206 of 206 files at r2.
Review status: all files reviewed at latest revision, 4 unresolved discussions.


src/librustc_const_eval/pattern.rs, line 306 at r1 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

@nikomatsakis wrote:

the comment on this line sounds about right. And this loop [below] also seems about right; to adjust ty, you can simply match on it --
it must be a TyRef or else something went wrong somewhere
so you could do like ty = match ty.sty { TyRef(_, mt) => mt.ty, _ => bug!() };

OK


src/librustc_typeck/check/_match.rs, line 684 at r1 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

@nikomatsakis wrote:

similarly, on this line, I don't really expect there to be many changes needed -- that is, I don't think that check_pat_tuple_struct() itself will have to change much. I expect the adaptation to happen at the top of check_pat_arg()

LGTM


src/librustc_typeck/check/_match.rs, line 51 at r2 (raw file):

            PatKind::Tuple(..) |
            PatKind::Box(_) |
            // PatKind::Lit(_) | // FIXME(tschottdorf): causes lots of errors

Hmm I guess I have to check out what produces a PatKind::Lit. I am not crazy about the idea of poking at the type of the literal. If I had to guess, I would guess that the problems with literals come about because of things like matching "foo" against an &str, maybe?


Comments from Reviewable

See the [RFC] and [tracking issue].

This is a work in progress. The basic examples from the RFC are in place and work,
but there are still some obvious deficiencies (some of which documented through
currently problematic tests).

Commentary is sparse. This will be improved before merging.

[tracking issue]: rust-lang#42640
[RFC]: https://github.com/rust-lang/rfcs/blob/491e0af/text/2005-match-ergonomics.md
These updates look like the are unintentional, but I am not sure
how to fix them.
@tbg
Copy link
Owner Author

tbg commented Sep 15, 2017

Closing this. Will open a PR against the main repo next.

@tbg tbg closed this Sep 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants