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

Allow if and match in constants #2342

Merged
merged 3 commits into from
Mar 18, 2018
Merged

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Feb 20, 2018

Enable if and match during const evaluation and make them evaluate lazily.
In short, this will allow if x < y { y - x } else { x - y } even though the
else branch would emit an overflow error for unsigned types if x < y.

Rendered
Tracking issue

@Centril Centril added the T-lang Relevant to the language team, which will review and decide on the RFC. label Feb 20, 2018
@Centril
Copy link
Contributor

Centril commented Feb 20, 2018

The semantics of match in constants is not described at all, so it feels a bit thin atm. Could you add some details on this in the RFC?

@oli-obk
Copy link
Contributor Author

oli-obk commented Feb 20, 2018

@Centril I don't see match and if as separate commands semantically. The following can be converted

match enum_value {
    Variant0Pattern => {},
    Variant1Pattern => {},
    _ => {
        // something else
    },
}

to some very unsafe code:

if std::intrinsics::discriminant(enum_value) == 0 {
     let Pattern: EnumVariant0 = transmute(enum_value);
} else if std::intrinsics::discriminant(enum_value) == 1 {
     let Pattern: EnumVariant1 = transmute(enum_value);
} else {
     // something else
}

Which is almost how MIR handles matches.

@Centril
Copy link
Contributor

Centril commented Feb 20, 2018

@oli-obk Still... I feel lang RFC texts should be as unambiguous as possible... This makes it easier to review changes proposed and define the spec / reference of the language =)

@Havvy
Copy link
Contributor

Havvy commented Feb 21, 2018

Part of the issue with this is that if and recursion allows for looping in a very unidiomatic way, so I've been told that we've been waiting until we have a good const story for looping. Could be solved naively by disallowing recursion by making it a compiler error when it happens?

@oli-obk
Copy link
Contributor Author

oli-obk commented Feb 21, 2018

Could be solved naively by disallowing recursion by making it a compiler error when it happens?

The allowed recursion depth isn't very high. Also const fn is unstable. You won't be able to do many interesting loops this way.

@nox
Copy link
Contributor

nox commented Feb 25, 2018

@Centril I don't see match and if as separate commands semantically. The following can be converted

Then I would describe the semantics of match instead of if, and explain that if is just sugar for boolean matches. It seems weird to me to do the opposite.

@oli-obk
Copy link
Contributor Author

oli-obk commented Feb 25, 2018

The reason I did the opposite is that if examples are way simpler. I can do step between the explanations with "and since if is simply match on a boolean, ..."

@petrochenkov
Copy link
Contributor

@nox @oli-obk

and since if is simply match on a boolean, ...

Fun fact: there are actually differences in if cond { ... } else { ... } and match cond { true => ..., false => ... } with regards to how long cond lives! (At least without NLL.)
I tried to desugar all ifs into matches as part of #2260 (comment) and found some pretty real breakage from borrow checker during rustc bootstrap.

@withoutboats
Copy link
Contributor

@rfcbot fcp merge

@rfcbot
Copy link
Collaborator

rfcbot commented Feb 27, 2018

Team member @withoutboats has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added the proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. label Feb 27, 2018
@eddyb
Copy link
Member

eddyb commented Feb 27, 2018

@petrochenkov match { let cond = cond; cond } { ... } might work - or we could even make "terminating" scopes explicit in HIR instead of leaving them implicit.

@gnzlbg
Copy link
Contributor

gnzlbg commented Mar 5, 2018

This is required to implement const fn constructors for portable packed SIMD vector types in stdsimd, and is tangetially related to fixing rust-lang/stdarch#336 .

@hanna-kruppe
Copy link

@gnzlbg Can you elaborate how? I don't see why those functions (both for portable vector types and arch-specific ones) would have to branch or match, and indeed I can't see anything other than simple data movement when I look at the current implementations.

@gnzlbg
Copy link
Contributor

gnzlbg commented Mar 5, 2018

@rkruppe wrong link, I've edited the post, but this explains it in a nutshell:

Boolean vector constructors' take bools as arguments but use internally a signed integer type to store their values. The conversion is:

#[inline]
/* const */ fn bool_to_internal(x: bool) -> $elem_ty {  // $elem_ty is, e.g., i32
    if x  {
        !(0 as $elem_ty)
    } else {
        0 as $elem_ty
    }
}

The new and splat constructors need to perform these conversions:

#[inline]
pub /*const*/ fn new($($elem_name: bool),*) -> Self {
    $id($(Self::bool_to_internal($elem_name)),*)
}

and currently cannot be const fn. The constructors of the non-boolean SIMD vector types are, however, const fn.

@hanna-kruppe
Copy link

hanna-kruppe commented Mar 5, 2018

Ah, so this is only about the boolean consts? And probably only the platform specific types?

In any case note that this RFC just gives you lazy evaluation of the branches (and more natural syntax). In simple cases like selecting between 0 and !0, you can already use arrays and indexing today.

@gnzlbg
Copy link
Contributor

gnzlbg commented Mar 5, 2018

Ah, so this is only about the boolean consts? And probably only the platform specific types?

This is only about the portable boolean vector types (they work portably across platforms).

In simple cases like selecting between 0 and !0, you can already use arrays and indexing today.

Oh, I did not know about that! Thanks! That should work for stdsimd :)

@eddyb
Copy link
Member

eddyb commented Mar 6, 2018

In simple cases like selecting between 0 and !0, you can already use arrays and indexing today.

Why not just cast the boolean to an integer (giving you either 0 or 1) and then negate the integer (giving you either 0 or -1 aka !0)?

This is usually seen in branchless selects, where c ? a : b becomes (-c & a) | (~-c & b).

@rfcbot
Copy link
Collaborator

rfcbot commented Mar 8, 2018

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. and removed proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. labels Mar 8, 2018
@rfcbot
Copy link
Collaborator

rfcbot commented Mar 18, 2018

The final comment period is now complete.

@Centril Centril merged commit 3af4a21 into rust-lang:master Mar 18, 2018
@Centril
Copy link
Contributor

Centril commented Mar 18, 2018

Huzzah! The RFC has been accepted.

Tracking issue: rust-lang/rust#49146

y - x
}
}
const AB: u32 = foo(x, y);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't that be foo(X, Y)?

@oli-obk oli-obk deleted the const-control-flow branch April 16, 2018 08:05
@oli-obk oli-obk mentioned this pull request Apr 16, 2018
@Centril Centril added A-machine Proposals relating to Rust's abstract machine. A-control-flow Proposals relating to control flow. A-const-eval Proposals relating to compile time evaluation (CTFE). labels Nov 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-eval Proposals relating to compile time evaluation (CTFE). A-control-flow Proposals relating to control flow. A-machine Proposals relating to Rust's abstract machine. final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. T-lang Relevant to the language team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.