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

Initial implementation of or-pattern handling in MIR #63688

Open
wants to merge 2 commits into
base: master
from

Conversation

@dlrobertson
Copy link
Contributor

commented Aug 18, 2019

Building on #61708 this PR adds:

  • Initial implementation of or-pattern lowering to MIR.
  • Basic ui tests for or-patterns

CC #54883

r? @matthewjasper

src/librustc_mir/build/matches/mod.rs Outdated Show resolved Hide resolved
src/librustc_mir/build/matches/test.rs Outdated Show resolved Hide resolved
src/librustc_mir/build/matches/test.rs Outdated Show resolved Hide resolved
src/librustc_mir/build/matches/test.rs Outdated Show resolved Hide resolved
src/librustc_mir/build/matches/test.rs Outdated Show resolved Hide resolved
src/test/ui/or-patterns/basic-switch.rs Show resolved Hide resolved
src/test/ui/or-patterns/basic-switchint.rs Outdated Show resolved Hide resolved
src/test/ui/or-patterns/mix-with-wild.rs Show resolved Hide resolved

@rust-lang rust-lang deleted a comment Aug 20, 2019

@dlrobertson dlrobertson force-pushed the dlrobertson:or-patterns-1 branch from 0a13430 to 1991dbd Aug 21, 2019

dlrobertson added 2 commits Jul 14, 2019
implementation of or-pattern lowering to MIR
Initial implementation of or-pattern lowering to MIR.
basic run-pass tests for or-patterns
Add some basic run-pass ui tests for or-patterns.
@dlrobertson

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2019

@Centril thanks for the comments! Updated to take into account your feedback. Let me know if I missed something.

@dlrobertson

This comment has been minimized.

Copy link
Contributor Author

commented Aug 25, 2019

ping @matthewjasper any thoughts on how I build the MIR for the or-pattern?

@matthewjasper
Copy link
Contributor

left a comment

This seems like a reasonable first step. I think you might be better off using match_candidates for Or tests, since handling complex or patterns (e.g. (1, 1, _) | (_, 2, 2)) would require duplication of most of the remaining match_candidates code.

I've left a couple of comments for other things I noticed when looking through the changes.

pub struct FieldPattern<'tcx> {
pub field: Field,
pub pattern: Pattern<'tcx>,
}

#[derive(Clone, Debug)]
#[derive(Clone, Debug, PartialEq)]

This comment has been minimized.

Copy link
@matthewjasper

matthewjasper Aug 26, 2019

Contributor

I can't see where these PartialEq impls are used. What is the intended meaning of == here?

// `SwitchInt` (an `enum`).
// run-pass
#![feature(or_patterns)]
//~^ WARN the feature `or_patterns` is incomplete and may cause the compiler to crash

This comment has been minimized.

Copy link
@matthewjasper

matthewjasper Aug 26, 2019

Contributor

Can you allow this warning for the tests here?

@dlrobertson

This comment has been minimized.

Copy link
Contributor Author

commented Aug 26, 2019

@matthewjasper I couldn't figure out a way to use match_candidates that didn't result in a blow-up of code evaluated as noted in the RFC implementation notes. Given Foo(A | B, C | D) wouldn't we need to expand the or-pattern to the following candidates?

Candidate 1 match pairs:
  .0: A
  .1: C
Candidate 2 match pairs:
  .0: A
  .1: D
Candidate 3 match pairs:
  .0: B
  .1: C
Candidate 4 match pairs:
  .0: B
  .1: D

It may be that I overlooked something, so please let me know if there is another way to generate the correct candidates for an or-pattern, or if I am misunderstanding Candidates.

@matthewjasper

This comment has been minimized.

Copy link
Contributor

commented Aug 26, 2019

An or pattern would create a new set of candidates for just that pattern.

First Or pattern
Candidate 1 match pairs:
  .0: A
Candidate 2 match pairs:
  .0: B

Second or pattern
Candidate 1 match pairs:
  .1: C
Candidate 2 match pairs:
  .1: D
@dlrobertson

This comment has been minimized.

Copy link
Contributor Author

commented Aug 26, 2019

Ah I see... Yeah that could work. I'll try that out.

@ProgrammaticNajel

This comment has been minimized.

Copy link

commented Sep 6, 2019

Ping from triage. @dlrobertson any updates on this? Thanks.

@rustbot modify labels to +S-waiting-on-author, -S-waiting-on-review

@Centril

This comment has been minimized.

Copy link
Member

commented Sep 6, 2019

@dlrobertson Now that #64111 landed, you might want to try tests with bindings as well if this PR would support that. See https://github.com/rust-lang/rust/pull/64111/files#diff-03c5a135508e329a1f3eae00e79f9ad4R42 for a relevant test.

@dlrobertson

This comment has been minimized.

Copy link
Contributor Author

commented Sep 8, 2019

Awesome! Thanks... I think it should work more or less without much effort once I move to using a Candidate instead of manually manipulating the CFG

@joelpalmer

This comment has been minimized.

Copy link

commented Sep 16, 2019

Ping from Triage, @dlrobertson any updates?

@dlrobertson

This comment has been minimized.

Copy link
Contributor Author

commented Sep 16, 2019

Working on figuring out where the best place to put the expansion of candidates is (Placing it with match pair simplification would cause exponential blow-up). And we currently don't assume that the list of candidates is grow-able, so I'm also working on correctly handling that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.