Stream: t-compiler

Topic: #54883 -- or-patterns


centril (Oct 18 2019 at 12:57, on Zulip):

@dlrobertson @varkor @Matthew Jasper :wave: -- I thought I'd make a Zulip thread for the work

dlrobertson (Oct 18 2019 at 13:15, on Zulip):

Awesome

dlrobertson (Oct 18 2019 at 13:15, on Zulip):

I'm a little stuck on generating the correct slice or vector of Candidates

dlrobertson (Oct 18 2019 at 13:17, on Zulip):

The problem boils down to the fact that the current algorithm assumes the number of patterns/match pairs is not growable, but we're obviously changing that

dlrobertson (Oct 18 2019 at 13:27, on Zulip):

The case <pat>(<pat> | <pat>) is super simple and is very easy to get working

dlrobertson (Oct 18 2019 at 13:28, on Zulip):

But when you have multiple match pairs that are or-patterns that's when things get way more complicated

dlrobertson (Oct 18 2019 at 13:29, on Zulip):

In every implementation I've tried so far I either don't generate the correct test or don't clear the candidates match pairs correctly, so I hit an assertion later

dlrobertson (Oct 18 2019 at 13:30, on Zulip):

Thoughts and feedback would be super helpful

centril (Oct 18 2019 at 13:30, on Zulip):

@dlrobertson Is there perhaps a more targeted PR that we can land first for the easy subset that doesn't create roadblocks for the more advanced case?

centril (Oct 18 2019 at 13:30, on Zulip):

Might be good purely for testing purposes

dlrobertson (Oct 18 2019 at 13:30, on Zulip):

Maybe the single or pattern?

centril (Oct 18 2019 at 13:31, on Zulip):

Yeah that's what I'm thinking

dlrobertson (Oct 18 2019 at 13:31, on Zulip):

E.g. Foo(1 | 2, 3 | 4) throws a bug

centril (Oct 18 2019 at 13:32, on Zulip):

:+1:

dlrobertson (Oct 18 2019 at 13:33, on Zulip):

I can post an update to the PR that implements this in a Jacky manner and then maybe @varkor or @Matthew Jasper will spot the reason why multiple or patterns fail

centril (Oct 18 2019 at 13:34, on Zulip):

@dlrobertson you could also file a smaller extraction-PR for the subset

centril (Oct 18 2019 at 13:34, on Zulip):

(I find that easier so I don't lose my original work)

centril (Oct 18 2019 at 13:36, on Zulip):

cc also @nikomatsakis, @pnkfelix

dlrobertson (Oct 27 2019 at 22:59, on Zulip):

@centril been working on an update today... I'll post a PR shortly

centril (Oct 27 2019 at 22:59, on Zulip):

Nice!

dlrobertson (Oct 27 2019 at 22:59, on Zulip):

Most everything is passing locally at the moment

dlrobertson (Oct 27 2019 at 23:00, on Zulip):

I'm sure there are holes in it and it is super hacky

centril (Oct 27 2019 at 23:00, on Zulip):

Glad to see progress nonetheless; soon we'll be able to announce to the world that they should try the implementation out

dlrobertson (Oct 27 2019 at 23:02, on Zulip):

as long as all bugs are attributed to work you did :smiley:

centril (Oct 27 2019 at 23:02, on Zulip):

=P

dlrobertson (Oct 27 2019 at 23:02, on Zulip):

jk if anything the errors will surface in the lowering

dlrobertson (Oct 27 2019 at 23:03, on Zulip):

I was curious how rustfmt works with the or-pattern?

centril (Oct 27 2019 at 23:03, on Zulip):

I'd be very happy if bugs are found and reported; test cases for free! :tada:

centril (Oct 27 2019 at 23:03, on Zulip):

@dlrobertson no idea if it is implemented or not

centril (Oct 27 2019 at 23:03, on Zulip):

but it should probably be since we made AST level changes

dlrobertson (Oct 27 2019 at 23:04, on Zulip):

I know they have special rules for | at the end vs at the beginning

dlrobertson (Oct 27 2019 at 23:04, on Zulip):

yeah I'd think it would _just work_ but

dlrobertson (Oct 27 2019 at 23:04, on Zulip):

was curious

centril (Oct 27 2019 at 23:06, on Zulip):

Seems like it is implemented:

fn foo() {
    match 0 {
        Some(
            VeryLongThing
            | JesusHowLongThisIs
            | IMeanHaveYouSeenSomethingSoLong
            | AndItGetsEvenLonger,
        ) => {}
    }
}
centril (Oct 27 2019 at 23:07, on Zulip):

@dlrobertson looks reasonable

dlrobertson (Oct 27 2019 at 23:07, on Zulip):

:tada: nice!

dlrobertson (Oct 28 2019 at 00:28, on Zulip):

:/ need to do a rebase on upstream... will be a sec to rebuild and test

dlrobertson (Oct 28 2019 at 00:28, on Zulip):

if I don't push the changes up tonight, I'll push them tomorrow

dlrobertson (Oct 28 2019 at 02:45, on Zulip):

pushed the changes... its definitely still in the phase where it is largely a hack and less of a feature

dlrobertson (Oct 28 2019 at 02:45, on Zulip):

did quite a bit of testing and a lot seems to work out of the box

dlrobertson (Oct 28 2019 at 02:46, on Zulip):

largely due to the move from manually building the CFG -> using the Candidate

dlrobertson (Oct 28 2019 at 02:48, on Zulip):

but @ currently causes an ICE and unreachability checks trigger some false warnings

dlrobertson (Oct 28 2019 at 02:48, on Zulip):

I'm sure I'll find more as I go

centril (Oct 28 2019 at 02:50, on Zulip):

Hopefully @Matthew Jasper will pitch in at some point :slight_smile:

Matthew Jasper (Oct 28 2019 at 20:21, on Zulip):

I'll have a look at this at the weekend. I'll need to experiment a bit to work out what the final version of the code should look like.

Matthew Jasper (Nov 02 2019 at 22:46, on Zulip):

@dlrobertson I've pulled your branch and experimented with it locally a bit. There seem to be some ICEs and unexpected errors from match checking which I don't know how to fix, I guess @Nadrieril will be fixing them.

dlrobertson (Nov 02 2019 at 22:52, on Zulip):

Depends on what the ICE is I guess?

dlrobertson (Nov 02 2019 at 22:53, on Zulip):

I did hit ICEs when using @

dlrobertson (Nov 02 2019 at 22:53, on Zulip):

What were you hitting an ICE with?

Matthew Jasper (Nov 02 2019 at 22:54, on Zulip):

This assert is firing:

build/matches/mod.rs:1215 |    assert!(rows.iter().all(|r| r.len() == v.len()));

test code:

pub fn f() -> i32 {
    let x = ((1,),);

    match x {
        ((y,) | (y,),) => 2,
        _ => 4,
    }
}
Matthew Jasper (Nov 02 2019 at 22:56, on Zulip):

I've left a comment trying to explain what I was expecting the lowering code to look like.

Nadrieril (Nov 21 2019 at 17:26, on Zulip):

Hello ! I've been working on the exhaustiveness side, and the basic impl is reasonably straightforward. However the tests I write trigger ICEs :sad: . Example:

struct X<T>(T);
match X(0u8) {
    X(1 | 2) => {}
    _ => {}
    _ => {} //~ ERROR unreachable pattern
}

I made sure there would be an error so that we don't get into code generation, but rustc does try to go further and hits me with "simplifyable pattern found". Any idea how I could work around this ?

centril (Nov 21 2019 at 17:32, on Zulip):

@Nadrieril maybe insert something else below that would fail in match checking

centril (Nov 21 2019 at 17:32, on Zulip):
let 0: u8 = 0;
centril (Nov 21 2019 at 17:33, on Zulip):

hopefully this should allow you to write the tests exercising match checking but avoid reaching the borrow checker

centril (Nov 21 2019 at 17:34, on Zulip):

hmm; that doesn't seem to work... https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=ad7f9b5d67afd59c24aa395ceda01fbd

centril (Nov 21 2019 at 17:35, on Zulip):

maybe try inserting a fatal error in MIR building on encountering PatKind::Or(...) which isn't at the top level

centril (Nov 21 2019 at 17:35, on Zulip):

that way it won't ICE

centril (Nov 21 2019 at 17:35, on Zulip):

I believe you should also be able to have ICEs in tests if you prefer that strategy

Nadrieril (Nov 21 2019 at 18:39, on Zulip):

Oh indeed, I discovered should-ice, thanks !

centril (Nov 21 2019 at 18:41, on Zulip):

np; it was added pretty recently ^^

Nadrieril (Nov 21 2019 at 19:05, on Zulip):

Got https://github.com/rust-lang/rust/pull/66612 up ! It does almost everything I believe, there's just some more work to do to clean up the current ad-hocery used for top-level or-patterns

centril (Nov 21 2019 at 19:07, on Zulip):

that was quick :D

Nadrieril (Nov 22 2019 at 00:12, on Zulip):

I had been thinking about it for a while when writing my other PRs ^^

centril (Nov 22 2019 at 00:40, on Zulip):

hehe; left some review comments for you

centril (Nov 22 2019 at 00:40, on Zulip):

nice work

Nadrieril (Nov 26 2019 at 00:53, on Zulip):

@centril What is abort_if_errors that you mentioned in you PR comment ?

centril (Nov 26 2019 at 00:54, on Zulip):

https://doc.rust-lang.org/nightly/nightly-rustc/rustc/session/struct.Session.html#method.abort_if_errors

centril (Nov 26 2019 at 00:54, on Zulip):

it aborts compilation, basically

centril (Nov 26 2019 at 00:55, on Zulip):

could use https://doc.rust-lang.org/nightly/nightly-rustc/rustc_errors/struct.FatalError.html#method.raise also I think

centril (Nov 26 2019 at 00:56, on Zulip):

using https://doc.rust-lang.org/nightly/nightly-rustc/rustc/session/struct.Session.html#method.span_fatal is probably most apprioriate as it has -> !

Nadrieril (Nov 26 2019 at 01:09, on Zulip):

I see, thanks

Last update: Apr 06 2020 at 02:15UTC