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

Replace calls to has_local_value with needs_infer. #70285

Closed
eddyb opened this issue Mar 22, 2020 · 6 comments · Fixed by #70860
Closed

Replace calls to has_local_value with needs_infer. #70285

eddyb opened this issue Mar 22, 2020 · 6 comments · Fixed by #70860
Assignees
Labels
A-type-system Area: Type system C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@eddyb
Copy link
Member

eddyb commented Mar 22, 2020

The only difference between the two is that "fresh" variables (a predecessor of the "canonical query" system) are considered needs_infer, despite not really taking part in inference.

I think that even without changing/replacing the "freshening" system at all, we can make "fresh" variables not needs_infer, removing the need for TypeFlags::KEEP_IN_LOCAL_TCX and has_local_value (which only existed for the 'gcx vs 'tcx split anyway).

If we want to be more explicit, we could rename needs_infer to is_local_to_inference_context or something similar, but either way I think it makes more sense than "local value".

cc @nikomatsakis @matthewjasper @Zoxc

This issue has been assigned to @lcnr via this comment.

@eddyb eddyb added A-type-system Area: Type system C-cleanup Category: PRs that clean code up or issues documenting cleanup. labels Mar 22, 2020
@jonas-schievink jonas-schievink added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Mar 22, 2020
@lcnr
Copy link
Contributor

lcnr commented Mar 31, 2020

@rustbot claim

I want to look into this, may take a few days though.

@rustbot rustbot self-assigned this Mar 31, 2020
@nikomatsakis
Copy link
Contributor

This sounds right. I was wondering whether that flag still needed to exist really, given that the gcx/tcx split was removed.

@eddyb
Copy link
Member Author

eddyb commented Apr 1, 2020

@nikomatsakis What would you prefer we do to "fresh variables"? Does anything rely on needs_infer returning true for them?

I guess there's at least one place where we want to check if any types or consts could be later replaced ("is further specializable" type checks), but @davidtwco is adding a special flag for that.
I think right now the check omits ty::Bound and ty::Placeholder (because it checks needs_infer() || needs_substs()), which seems wrong.

I'll have to do an audit of all uses of needs_infer, I think. @lcnr let's coordinate on Zulip.

@nikomatsakis
Copy link
Contributor

I'd like to get rid of fresh variables altogether, but in the meantime, I don't think anything should be relying on that -- they really shouldn't leak outside of the select caching in the trait system, I don't think.

@eddyb
Copy link
Member Author

eddyb commented Apr 1, 2020

Okay I've looked at all needs_infer uses, these are the interesting ones:

  1. this one is super weird, I think it refers to something that doesn't exist anymore?

    if !t.needs_infer() && !ty::keep_local(&t) {
    t // micro-optimize -- if there is nothing in this type that this fold affects...
    // ^ we need to have the `keep_local` check to un-default
    // defaulted tuples.
    } else {
    let t = self.infcx.shallow_resolve(t);
    match t.kind {
    ty::Infer(ty::TyVar(vid)) => {
    self.err = Some(FixupError::UnresolvedTy(vid));
    self.tcx().types.err
    }
    ty::Infer(ty::IntVar(vid)) => {
    self.err = Some(FixupError::UnresolvedIntTy(vid));
    self.tcx().types.err
    }
    ty::Infer(ty::FloatVar(vid)) => {
    self.err = Some(FixupError::UnresolvedFloatTy(vid));
    self.tcx().types.err
    }
    ty::Infer(_) => {
    bug!("Unexpected type in full type resolver: {:?}", t);
    }
    _ => t.super_fold_with(self),
    }
    }
    }
    fn fold_region(&mut self, r: ty::Region<'tcx>) -> ty::Region<'tcx> {
    match *r {
    ty::ReVar(rid) => self
    .infcx
    .lexical_region_resolutions
    .borrow()
    .as_ref()
    .expect("region resolution not performed")
    .resolve_var(rid),
    _ => r,
    }
    }
    fn fold_const(&mut self, c: &'tcx ty::Const<'tcx>) -> &'tcx ty::Const<'tcx> {
    if !c.needs_infer() && !ty::keep_local(&c) {
    c // micro-optimize -- if there is nothing in this const that this fold affects...
    // ^ we need to have the `keep_local` check to un-default
    // defaulted tuples.

  2. the specialization check I mentioned, which @davidtwco will open a PR to replace:

    // NOTE(eddyb) inference variables can resolve to parameters, so
    // assume `poly_trait_ref` isn't monomorphic, if it contains any.
    let poly_trait_ref =
    selcx.infcx().resolve_vars_if_possible(&poly_trait_ref);
    !poly_trait_ref.needs_infer() && !poly_trait_ref.needs_subst()

  3. select, which I believe likely wants to know about fresh variables too?

    let needs_infer = stack.obligation.predicate.needs_infer();
    // If there are STILL multiple candidates, we can further
    // reduce the list by dropping duplicates -- including
    // resolving specializations.
    if candidates.len() > 1 {
    let mut i = 0;
    while i < candidates.len() {
    let is_dup = (0..candidates.len()).filter(|&j| i != j).any(|j| {
    self.candidate_should_be_dropped_in_favor_of(
    &candidates[i],
    &candidates[j],
    needs_infer,

    used here:
    // Subtle: If the predicate we are evaluating has inference
    // variables, do *not* allow discarding candidates due to
    // marker trait impls.
    //
    // Without this restriction, we could end up accidentally
    // constrainting inference variables based on an arbitrarily
    // chosen trait impl.
    //
    // Imagine we have the following code:
    //
    // ```rust
    // #[marker] trait MyTrait {}
    // impl MyTrait for u8 {}
    // impl MyTrait for bool {}
    // ```
    //
    // And we are evaluating the predicate `<_#0t as MyTrait>`.
    //
    // During selection, we will end up with one candidate for each
    // impl of `MyTrait`. If we were to discard one impl in favor
    // of the other, we would be left with one candidate, causing
    // us to "successfully" select the predicate, unifying
    // _#0t with (for example) `u8`.
    //
    // However, we have no reason to believe that this unification
    // is correct - we've essentially just picked an arbitrary
    // *possibility* for _#0t, and required that this be the *only*
    // possibility.
    //
    // Eventually, we will either:
    // 1) Unify all inference variables in the predicate through
    // some other means (e.g. type-checking of a function). We will
    // then be in a position to drop marker trait candidates
    // without constraining inference variables (since there are
    // none left to constrin)
    // 2) Be left with some unconstrained inference variables. We
    // will then correctly report an inference error, since the
    // existence of multiple marker trait impls tells us nothing
    // about which one should actually apply.
    !needs_infer

@davidtwco
Copy link
Member

Submitted #70658 to address the second case from eddy's comment.

Centril added a commit to Centril/rust that referenced this issue Apr 2, 2020
…-specializable, r=eddyb

add `STILL_FURTHER_SPECIALIZABLE` flag

Contributes to rust-lang#70285.

This PR adds a `STILL_FURTHER_SPECIALIZABLE` flag to `TypeFlags`
which replaces `needs_infer` and `needs_subst` in `Instance::resolve`
and `assemble_candidates_from_impls`.

r? @eddyb
Centril added a commit to Centril/rust that referenced this issue Apr 2, 2020
…-specializable, r=eddyb

add `STILL_FURTHER_SPECIALIZABLE` flag

Contributes to rust-lang#70285.

This PR adds a `STILL_FURTHER_SPECIALIZABLE` flag to `TypeFlags`
which replaces `needs_infer` and `needs_subst` in `Instance::resolve`
and `assemble_candidates_from_impls`.

r? @eddyb
@bors bors closed this as completed in 11f6096 Apr 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-type-system Area: Type system C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants