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

[NLL] check other sorts of rvalues beyond aggregates #45959

Closed
17 tasks done
nikomatsakis opened this issue Nov 13, 2017 · 4 comments
Closed
17 tasks done

[NLL] check other sorts of rvalues beyond aggregates #45959

nikomatsakis opened this issue Nov 13, 2017 · 4 comments
Labels
T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Nov 13, 2017

@Nashenas88 implemented aggregate type-checking in nikomatsakis#13, but we still need to handle other sorts of rvalues.

This is a meta-issue to cover the various cases. As PRs come in, I can check things off. Here is a list. In practice, all of the issues boil down to needing to verify various trait requirements, so the main step here will be building a subroutine for checking that a given trait reference is satisfied at a particular location (somewhat similar to normalize -- instructions to come shortly).

  • Rvalue::Use(Operand) -- let y = x
    • Nothing to do here, I think. added by @arielb1 verify that the operand is Sized? Do that for every operand?
  • Rvalue::Repeat(Operand, ConstUsize) -- let y = [x; N]
    • We must check that the operand is Copy.
  • Rvalue::Ref(Region, BorrowKind, Lvalue) -- let y = &x
    • The only special rules here are region-specific -- e.g., reborrowing constraints -- so we'll let the NLL region inference handle them.
  • Rvalue::Len(X)
    • No special rules.
  • Rvalue::Cast(CastKind, Operand, Ty), let's break this down by CastKind:
    • Misc -- some form of bitcast I think, no action needed
    • ReifyFnPointer - added by @arielb1 the signatures must match
    • ClosureFnPointer - added by @arielb1 the signatures must match
    • UnsafeFnPointer - added by @arielb1 the signatures must match
    • Unsize - I think we should verify a trait obligation here, something like T: Unsize<U> where T is the operand's type and U` is the target type. Have to double check this.
    • all these cases are handled in Check other rvalues nikomatsakis/rust#19
  • Rvalue::BinaryOp(..) -- built-in binary operators. Probably we should check that e.g. the LHS and RHS are of the same type, but I'm not sure if that must be true and it doesn't really affect NLL anyhow. Leave it for now.
  • Rvalue::CheckedBinaryOp(..) -- as above.
  • Rvalue::UnaryOp(..) -- as above.
  • Rvalue::Discriminant(..) -- no special rule.
  • Rvalue::NullaryOp(..) --
    • sizeof should require that the type is sized
    • box the same
@nikomatsakis nikomatsakis added E-needs-mentor T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 13, 2017
@nikomatsakis nikomatsakis changed the title [NLL] check other sorts of rvalues beyond [NLL] check other sorts of rvalues beyond aggregates Nov 13, 2017
@nikomatsakis
Copy link
Contributor Author

Mentoring instructions

As noted above, the main thing we need is some kind of function to check that a given trait reference is satisfied at a particular location. Let's call it prove. I imagine it would have a signature like this:

fn prove(&mut self, trait_ref: TraitRef<'tcx>, location: Location)

Inernally, like normalize, it should use fully_perform_op and look a little something like this:

self.fully_perform_op(location.at_self(), |this| {
    let mut selcx = traits::SelectionContext::new(this.infcx);
    let cause = traits::ObligationCause::misc(this.last_span, ast::CRATE_NODE_ID);
    let obligation = traits::Obligation::new(cause, this.param_env, trait_ref.to_poly_trait_ref());
    if let Some(vtable) = selcx.select(&obligation)? {
        Ok(InferOk { value: (), obligations: vtable.nested_obligations() })
    } else {
        mir_span_bug!(...) // typeck should have ensured that this will succeed
    }
}).unwrap()

This basically invokes the trait checker (selcx.select) and gets back a (hopefully) successful resolution (vtable) which may carry with it some more work to do, which we then propagate upwards via vtable.nested_obligations().

@spastorino
Copy link
Member

I'm taking this one, I've already talk with @nikomatsakis about it :).

bors added a commit that referenced this issue Nov 23, 2017
…ielb1

typeck aggregate rvalues in MIR type checker

This branch is an attempt to land content by @spastorino and @Nashenas88 that was initially landed on nll-master while we waited for #45825 to land.

The biggest change it contains is that it extends the MIR type-checker to also type-check MIR aggregate rvalues (at least partially). Specifically, it checks that the operands provided for each field have the right type.

It does not yet check that their well-formedness predicates are met. That is #45827. It also does not check other kinds of rvalues (that is #45959). @spastorino is working on those issues now.

r? @arielb1
@nikomatsakis
Copy link
Contributor Author

OK, so, some notes.

ReifyFnPointer. This coercion casts from the TyFnDef type (a zero-sized type given to each function) to a TyFnPtr type (the "reified" form, a function pointer whose origin we do not statically know). We basically want to check that the "target type" of the cast is equal to a TyFnPtr with the signature that we can derive from the source type ty (which is a TyFnDef).

To compute the signature, we do ty.fn_sig(tcx) -- this gives a PolyFnSig. This can be then converted into a TyFnPtr via tcx.mk_fn_ptr(ty.fn_sig(tcx)). We can then equate that with the target type of the cast.

@nikomatsakis
Copy link
Contributor Author

Done (in nll-master)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

2 participants