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

should let _: T = RHS be synonymous with let _ = RHS: T? #56715

Closed
pnkfelix opened this issue Dec 11, 2018 · 8 comments
Closed

should let _: T = RHS be synonymous with let _ = RHS: T? #56715

pnkfelix opened this issue Dec 11, 2018 · 8 comments
Labels
A-NLL Area: Non-lexical lifetimes (NLL) NLL-sound Working towards the "invalid code does not compile" goal T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@pnkfelix
Copy link
Member

pnkfelix commented Dec 11, 2018

Spawned off of #55748 (comment)

The Rust test suite currently has the following test of how subtyping and "covariance" (*) works with respect to regions.

struct Covariant<'a> {
f: extern "Rust" fn(&'a isize)
}
fn use_<'a>(c: Covariant<'a>) {
// OK Because Covariant<'a> <: Covariant<'static> iff 'a <= 'static
let _: Covariant<'static> = c;
}

The problem is that there are small variations on this test that exercise very different parts of the compiler, and potentially expose incoherence in the semantics.

Here is some code that explores the different behaviors one might witness (play):

#![feature(type_ascription)]
#![allow(dead_code)]

// SUMMARY OF BEHAVIOR
//
// EDITION        WILD      VAR       EXPR      ARG
// -----------    ------    ------    ------    ------
// 2015/AST       accept    accept    reject    accept
// 2018/NLL       accept    accept    reject    accept
// PATCHED        reject    accept    reject    accept

#[derive(Copy, Clone)]
struct Covariant<'a> {
    f: extern "Rust" fn(&'a isize)
}

fn ascribe_wild<'a>(co: Covariant<'a>) {
    let _: Covariant<'static> = co;  // (ascribe on) WILD(card)
}

fn ascribe_var<'a>(co: Covariant<'a>) {
    let _c: Covariant<'static> = co; // (ascribe on) VAR
}

fn ascribe_expr<'a>(co: Covariant<'a>) {
    let _ = co: Covariant<'static>;  // (ascribe on) EXPR
}

fn co_arg(_co_s: Covariant<'static>) { }

fn pass_actual_arg<'a>(co: Covariant<'a>) {
    co_arg(co);                      // (actual )ARG(ument)
}

pub fn main() {}

The entry in the summary that says "PATCHED" is for a 2018 compiler with the following patch applied to the following line of code:

ty::Variance::Covariant,
box ascription.user_ty.clone().user_ty(),

diff --git a/src/librustc_mir/build/matches/mod.rs b/src/librustc_mir/build/matches/mod.rs
index 7e7c0b1555..89f95368ad 100644
--- a/src/librustc_mir/build/matches/mod.rs
+++ b/src/librustc_mir/build/matches/mod.rs
@@ -1336,7 +1336,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
                     source_info,
                     kind: StatementKind::AscribeUserType(
                         ascription.source.clone(),
-                        ty::Variance::Covariant,
+                        ty::Variance::Invariant,
                         box ascription.user_ty.clone().user_ty(),
                     ),
                 },

The original reasoning behind that patch is that every other place that we use AscribeUserType, we use an Invariant relation, not a Covariant one.

Note that the PATCHED version continues to handle the VAR and ARG cases in a manner consistent with the previous editions of Rust. In other words, in this instance, it just changes how let _: T = RHS; behaves to act more like let _ = RHS: T;


(*): I put "covariance" in quote because I think Rust's definition of co/contravariance on regions is tied to a mental model that is not ideal for practical usage, see also rust-lang/rfcs#391

@pnkfelix
Copy link
Member Author

(Also, the aforementioned patch would also fix the test from #55748 (play) where NLL is causing us to accept code that we previously would have rejected. So could be used as an attempt to argue that the aforemented patch is "good"; but its entirely possible that a different path exists for a fix to #55748, so I won't further attempt to argue for the change on that basis. Instead I think the decision should be made based on the presentation of the various cases given above in this issue #56715 )

@matthewjasper matthewjasper added I-nominated A-NLL Area: Non-lexical lifetimes (NLL) NLL-sound Working towards the "invalid code does not compile" goal labels Dec 11, 2018
@pnkfelix
Copy link
Member Author

I think this is a T-lang issue as well as an NLL one. The change I am suggesting seems like it might only impact NLL (and will be downgraded to a warning when it errors under migration mode), but it still is something I want the T-lang team to look at. So I'll add that label here too.

@pnkfelix pnkfelix added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Dec 12, 2018
@nikomatsakis
Copy link
Contributor

We discussed this a bit in the @rust-lang/lang meeting and decided that we needed to have further discussion.

@pnkfelix -- one thing I would note though is that, as presently implemented, expr: T performs a type equality rule, but that was considered a temporary hack. I think that is the source of the discrepancy you are talking about?

@pnkfelix
Copy link
Member Author

expr: T performs a type equality rule, but that was considered a temporary hack. I think that is the source of the discrepancy you are talking about?

This seems likely.

I wasn't aware it was meant to be a temporary hack. I'll go see if I can find record of discussion of that matter.

@Centril
Copy link
Contributor

Centril commented Dec 15, 2018

@pnkfelix You might want to take a look here: rust-lang/rfcs#2522.

@pnkfelix
Copy link
Member Author

@Centril hmm while I do see that RFC discusses lifetimes, I'm not sure if it goes so far as to discuss whether PAT: T will use subtyping or type-equality (when considering lifetimes within types) when it checks the ascription...? Maybe an unresolved question? Or am I overlooking something?

@Centril
Copy link
Contributor

Centril commented Dec 17, 2018

@pnkfelix See judgements in the section on static semantics. The first and second subsections notes that given types τ and σ and some term x (or pattern), then ImplicitlyCoercible(τ, σ), x : τ => (x : σ) : σ. This is broader than subtyping; implicit coercions are OK. For patterns, default match bindings are also taken into account.

@pnkfelix
Copy link
Member Author

pnkfelix commented Dec 17, 2018

During a discussion on zulip, @nikomatsakis pointed out to me that given let PAT: T_LHS = RHS, it would be weird to perform coercions of the RHS based on T_LHS (which we definitely allow today, as in let _: &[u32] = y; // where y: &[u32; 3]) and yet not allow subtyping between T_LHS and whatever the type of RHS is.

I found that to be a pretty compelling argument against what I am proposing here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-NLL Area: Non-lexical lifetimes (NLL) NLL-sound Working towards the "invalid code does not compile" goal T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants