Stream: t-compiler

Topic: let_chain implementation


nikomatsakis (Dec 26 2018 at 16:29, on Zulip):

So @centril ...

nikomatsakis (Dec 26 2018 at 16:30, on Zulip):

... is the primary question you were asking me about how to modify the parser, or are you also wondering about the latter phases of the compiler?

centril (Dec 26 2018 at 16:31, on Zulip):

@nikomatsakis I've done some of that already but I might have questions about it... the bigger question is whether you find adding ast::ExprKind::Let(..) and removing ExprKind::{IfLet, WhileLet}

centril (Dec 26 2018 at 16:31, on Zulip):

...to be a good idea

centril (Dec 26 2018 at 16:32, on Zulip):

that's the cleanest way I could think of to implement the parser

nikomatsakis (Dec 26 2018 at 16:32, on Zulip):

it feels pretty reasonable to me

centril (Dec 26 2018 at 16:32, on Zulip):

and it will probably lead to better diagnostics / easier lowering / also more extensible if we want to improve lang design in the future

nikomatsakis (Dec 26 2018 at 16:33, on Zulip):

I guess the concern is that let isn't permitted in all places that expressions are permitted, obviously

nikomatsakis (Dec 26 2018 at 16:33, on Zulip):

but the same is true of (e.g.) impl Trait types

nikomatsakis (Dec 26 2018 at 16:33, on Zulip):

it seems particularly ok if this is confined to the AST, and not the HIR

nikomatsakis (Dec 26 2018 at 16:34, on Zulip):

@eddyb has often expressed the vision (which I agree with) that the AST ought to "naturally reflect" the things the user wrote, basically, whereas the HIR is more oriented around the compiler

nikomatsakis (Dec 26 2018 at 16:34, on Zulip):

I forget exactly how they put it but it seems to fit this scenario

centril (Dec 26 2018 at 16:34, on Zulip):

makes a lot of sense

centril (Dec 26 2018 at 16:36, on Zulip):

@nikomatsakis the other question is about the parser; In the RFC, specifically this subsection: https://github.com/rust-lang/rfcs/blob/master/text/2497-if-let-chains.md#a-few-more-examples, I specified some precedence changes

centril (Dec 26 2018 at 16:36, on Zulip):

I got the first one to work but not the other 3

nagisa (Dec 26 2018 at 16:36, on Zulip):

Consider having if and while take not an expression but pattern-expression?

nagisa (Dec 26 2018 at 16:36, on Zulip):

where pattern-expression = EXPR | LET PATTERN

nagisa (Dec 26 2018 at 16:38, on Zulip):

I’d rather not make let pat = expr an expression at AST level, that sounds like foot waiting to be shot into

centril (Dec 26 2018 at 16:39, on Zulip):

@nagisa my first thought was to do it like that, but if let a = b && c && let c = d { ... } is permitted and we might want to extend to || in the future; it also felt hard to get this right re. precedence since if a && b || c && d { .. } still needs to parse

centril (Dec 26 2018 at 16:41, on Zulip):

@nikomatsakis I wonder if I should follow the RFC to the letter re. the precedence changes of:

if let Range { start: _, end: _ } = true..true && false { ... }
if let PAT = break true && false { ... }
if let PAT = F..|| false { ... }
if let PAT = t..&&false { ... }

it seems more messy to do this change and honestly I cannot say that changing the parse is more intuitive

nikomatsakis (Dec 26 2018 at 16:41, on Zulip):

I got the first one to work but not the other 3

i.e., if let PAT = break true && false { ... } parsed incorrectly?

nikomatsakis (Dec 26 2018 at 16:41, on Zulip):

@centril do you have a branch somewhere?

nikomatsakis (Dec 26 2018 at 16:42, on Zulip):

re: following the RFC to the letter, I personally consider the RFC more of a "guideline", though of course we have to be clear about deviations and the reasons for them

nikomatsakis (Dec 26 2018 at 16:42, on Zulip):

but if we encounter complications or experience that makes us rethink what was decided, I think that's ok

nikomatsakis (Dec 26 2018 at 16:43, on Zulip):

as is implementing in stages :)

centril (Dec 26 2018 at 16:43, on Zulip):

;) in this case I also wrote the RFC :P

centril (Dec 26 2018 at 16:43, on Zulip):

@nikomatsakis yeah, if let PAT = break true && false { ... } parses incorrectly (it parses as it does today)

nikomatsakis (Dec 26 2018 at 16:43, on Zulip):

that said, I'm not clear on exactly what you are proposing

nikomatsakis (Dec 26 2018 at 16:43, on Zulip):

yes so the problem seems to be what restrictions were "intended" for

nikomatsakis (Dec 26 2018 at 16:44, on Zulip):

but I guess you are basically having problems with things open on the "right-hand side"

centril (Dec 26 2018 at 16:44, on Zulip):

@nikomatsakis (I'm suggesting) to let them parse as they do today

nikomatsakis (Dec 26 2018 at 16:44, on Zulip):

what about other examples, e.g. if let PAT = x && foo.is_true()

nikomatsakis (Dec 26 2018 at 16:44, on Zulip):

I feel ok about certain things -- like break -- yielding parse errors

nikomatsakis (Dec 26 2018 at 16:45, on Zulip):

I'm not keen about && being part of the matched expression sometimes but not other times

centril (Dec 26 2018 at 16:45, on Zulip):

hah; I haven't tested fields / methods yet; good point

nikomatsakis (Dec 26 2018 at 16:45, on Zulip):

(and successfully parsing, I mean)

nikomatsakis (Dec 26 2018 at 16:45, on Zulip):

anyway, to figure out why it's not working, and how best to make it work, I'd have to look at the code I guess

nikomatsakis (Dec 26 2018 at 16:45, on Zulip):

though I can sort of imagine how it's going wrong

centril (Dec 26 2018 at 16:46, on Zulip):

yeah; the code changes are very prototype-y atm

centril (Dec 26 2018 at 16:46, on Zulip):

ill throw up a temporary branch

nikomatsakis (Dec 26 2018 at 16:46, on Zulip):

I am not very keen on the "restrictions" impl, I feel like what we would rather have is that the "parse expression" fn is kind of a template, that takes as a parameter the "kind of expression" for the cases where it recurs

nikomatsakis (Dec 26 2018 at 16:46, on Zulip):

but anyway

nikomatsakis (Dec 26 2018 at 16:47, on Zulip):

yeah; the code changes are very prototype-y atm

seems fine

centril (Dec 26 2018 at 16:48, on Zulip):

@nikomatsakis https://github.com/Centril/rust/tree/let-chains-2-hack

centril (Dec 26 2018 at 16:50, on Zulip):

(ignore the various changes to Debug and such... they were just convenient hacks to see precedence changes...)

centril (Dec 26 2018 at 16:53, on Zulip):

(method calls and field projections seem to work right)

nikomatsakis (Dec 26 2018 at 16:54, on Zulip):

ok

centril (Dec 26 2018 at 16:57, on Zulip):

I feel like what we would rather have is that the "parse expression" fn is kind of a template, that takes as a parameter the "kind of expression" for the cases where it recurs

that seems like a good idea as a bigger refactoring; I think parser.rs is pretty stateful spaghetti atm so that'd help

centril (Dec 26 2018 at 17:06, on Zulip):

@nikomatsakis so when you say "I'm not keen about && being part of the matched expression sometimes but not other times" you mean that from a lang design POV you'd prefer to make the changes in the "A few more examples"?

nikomatsakis (Dec 26 2018 at 17:13, on Zulip):

maybe..

nikomatsakis (Dec 26 2018 at 17:14, on Zulip):

I mostly mean that if you have:

if let foo = bar && baz.is_true() { .. }

and it parses like let (foo = bar), then I think that every other use of && should either not parse or be treated as part of the if, not the expression being matched

nikomatsakis (Dec 26 2018 at 17:14, on Zulip):

in other words, maybe those examples with break and stuff should just error

nikomatsakis (Dec 26 2018 at 17:15, on Zulip):

I think it would be confusing if the && sometimes parses as part of the expression and sometimes as part of the if

nikomatsakis (Dec 26 2018 at 17:15, on Zulip):

but I've not given this a lot of thought

nikomatsakis (Dec 26 2018 at 17:15, on Zulip):

I confess :)

centril (Dec 26 2018 at 17:30, on Zulip):

@nikomatsakis so if we write { a && b..&&c }; it currently associates as { a && (b..(&&c)) }; (it won't typeck...) -- idk if we can change this tho due to macros; and it seems unlikely that someone will get bitten by this?

centril (Dec 26 2018 at 17:31, on Zulip):

the type error you get is pretty clear about the problem

centril (Dec 26 2018 at 17:54, on Zulip):

@nikomatsakis So what do you think about this?: To make progress, I'll implement this first without changing the 4 examples since that is the simplest and most minimal change, it is also forward compatible with changing the precedence later...

centril (Dec 28 2018 at 10:20, on Zulip):

@Vadim Petrochenkov thoughts on the above discussion? wdyt about ExprKind::Let ?

Vadim Petrochenkov (Dec 28 2018 at 10:35, on Zulip):

I think I'd prefer the nagisa's variant for now.

centril (Dec 28 2018 at 10:36, on Zulip):

@Vadim Petrochenkov so this would be a Vec<P<ExprOrLet>> in ::If and ::While ?

centril (Dec 28 2018 at 10:36, on Zulip):

(the Vec is for the &&s)

centril (Dec 28 2018 at 10:38, on Zulip):

@Vadim Petrochenkov also, what are your thoughts on the 4 ambiguities (https://github.com/rust-lang/rfcs/blob/master/text/2497-if-let-chains.md#a-few-more-examples) I've referenced above?

Vadim Petrochenkov (Dec 28 2018 at 11:41, on Zulip):

Vec<P<ExprOrLet>>

Yes, something like that, maybe even Vec<ExprOrLet> without P.

Vadim Petrochenkov (Dec 28 2018 at 11:51, on Zulip):

Precedence specified in the RFC looks ok to me.

Vadim Petrochenkov (Dec 28 2018 at 11:52, on Zulip):

We are basically parsing the expression after let PAT = as an arbitrary expression with restriction "no && and no ||".

centril (Dec 28 2018 at 11:54, on Zulip):

@Vadim Petrochenkov ah, my other thought was to just have ExprKind::Let and have |this| this.parse_assoc_expr_with(4, ...) in parse_let_expr

centril (Dec 28 2018 at 11:55, on Zulip):

well, substitute 4 for the precedence just above && so that it doesn't parse &&

centril (Dec 28 2018 at 11:55, on Zulip):

LAnd => 6, :slight_smile:

centril (Dec 28 2018 at 11:55, on Zulip):

I'll try your approach

Vadim Petrochenkov (Dec 28 2018 at 11:55, on Zulip):

We had restrictions like this before, but they were removed.
See e.g. RESTRICTION_NO_BAR_OP removed in https://github.com/rust-lang/rust/pull/23930

Last update: Nov 16 2019 at 01:00UTC