Refactor ProgramClauseData to remove Implies variant

[declanvk]

Closes #511

As far as I can tell, I didn't change the logic of anything. However, when I checked the test suite there were still some failing tests. I've listed them here just in case they are relevant.

Initially failing tests

test test::coherence::downstream_impl_of_fundamental_43355 ... FAILED
test test::coherence::fundamental_traits ... FAILED
test test::coherence::local_negative_reasoning_in_coherence ... FAILED
test test::coherence::nonoverlapping_assoc_types ... FAILED
test test::coherence::overlapping_assoc_types ... FAILED
test test::coherence::overlapping_assoc_types_error ... FAILED
test test::coherence::two_blanket_impls ... FAILED
test test::misc::flounder_ambiguous ... FAILED
test test::projection::normalize_into_iterator ... FAILED
test test::projection::projection_from_env_a ... FAILED
test test::projection::projection_from_env_slow ... FAILED
test test::wf_goals::struct_wf ... FAILED
test test::wf_lowering::copy_constraints ... FAILED
test test::wf_lowering::drop_constraints ... FAILED
test test::wf_lowering::implied_bounds ... FAILED

Looking into the reason now.

[jackh726]

Just quickly looking over this, the changes look correct. So there's something a bit more subtle here that I'll have to look more deeply at.

[declanvk]

My best guess for the error is that by introducing an empty Binders it still changes the meaning, especially with respect to folding, via:

chalk/chalk-ir/src/fold/binder_impls.rs

Lines 38 to 56 in a6400eb

	fn fold_with<'i>(
	&self,
	folder: &mut dyn Folder<'i, I, TI>,
	outer_binder: DebruijnIndex,
	) -> Fallible<Self::Result>
	where
	I: 'i,
	TI: 'i,
	{
	let Binders {
	binders: self_binders,
	value: self_value,
	} = self;
	let value = self_value.fold_with(folder, outer_binder.shifted_in())?;
	let binders = VariableKinds {
	interned: TI::transfer_variable_kinds(self_binders.interned().clone()),
	};
	Ok(Binders::new(binders, value))
	}

I managed to reproduce the same set of errors (the ones in the list above) with this commit alone: https://github.com/declanvk/chalk/commit/e96058569721eb71587ea294b82ee9c527ebcbf3

[jackh726]

Your example made me realize what the problem probably is. Where we used to construct an Implies, but now what essentially was a ForAll, if there are bound vars in the ProgramClauseImplication, then we have to shift them in.

[declanvk]

That fixed it! I had to export the Shift folder from chalk-ir, let me know if I should refactor this at all.

[nikomatsakis]

I'm not 100% sure if I think we should change this here or in follow-up PRs, but I do feel like probably the "cast" impls shouldn't be doing shifting.

[nikomatsakis]

I wonder if we should make a helper for this -- i.e., for inserting a binders into something with no captures.

But also, this strikes me as a bit dubious. I wonder if in the Cast routines we could, instead of shifting, assert that !implication.needs_shift(interner).

In particular, I feel like we don't normally embed ProgramClauses within other binders, so I wouldn't expect any references to bound variables in outer scopes -- and if there are any, it feels like the "intent" from a cast operation is sort of ambiguous to me.

So I'd be happier if we don't use cast in these cases.

[nikomatsakis]

That said, it seems like needs_shift needs to be rewritten from a hard-coded helper routine to a method that is available on anything that implements the Visit trait.

[jackh726]

Yeah, that was going to be my next comment: if we got rid of the Cast impl for one that does the shift, can we find the root "cause" there

[nikomatsakis]

I wonder if those cast impls are even used. Probably.

[jackh726]

@nikomatsakis nope, neither Cast impl is used :/

[nikomatsakis]

Well then let's just remove them and avoid the problem =)

[declanvk]

I removed the Cast impls and fixed the locations that failed by wrapping empty Binders and unconditionally applying shifted_in. I was contemplating some sort of Binders helper that looks like:

impl<I, T> Binders<T>
where
    T: Visit<I> + Fold<I, I, Result = T> + HasInterner<Interner = I>,
    I: Interner,
{
    pub fn empty_shift_if_required(interner: &T::Interner, value: T) -> Self {
        let value = if value.has_free_vars(interner) {
            // If this type contains "bound" types/lifetimes, then it
            // needs to be shifted across binders.
            value.shifted_in(interner)
        } else {
            value
        };

        Self::empty(interner, value)
    }
}

instead of having to apply shifted_in every time Binders::empty is used.

[nikomatsakis]

OK, I know I'm changing my mind, but seeing the diff makes me want to keep the impls -- or at least the one that goes from DomainGoal to ProgramClause.

It's hard to figure out the best API for dealing with debruijn indices, but I do think it's nice to have a way to say "here is some fact that contains types etc that are in scope, let me construct a program clause out of it" and not have to worry about the forall binder that would otherwise get added there.

[nikomatsakis]

@declanvk

I was contemplating some sort of Binders helper that looks like

I would perhaps instead just make the shifted_in helper check if there are no free variables and return a clone if so -- although there is a bit of a complication in that we might need to account for the possibility of T::Result not being equal to T.

[nikomatsakis]

But in any case that's just an optimization, so let's address it separately. It actually fits in with another theme of work I think we should pursue, which is propagating "flags" and the like, similar to what rustc has, which can enable us to create fast-paths quite efficiently.

[declanvk]

I removed the unused Cast impls and put back the CastTo<ProgramClause<I>> for T one with a shifted_in inside of it.

Should I rebase/squash to address conflicts?

[jackh726]

@declanvk Squashing doesn't matter. Rebase yes :)

[jackh726]

Seems like rebase problems with recursive.rs

[jackh726]

LGTM. I'll merge when CI is working again.

Just saw, guess CI is working again.