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

Port DomainGoal matching from rustc #216

Merged

Conversation

@detrumi
Copy link
Contributor

detrumi commented Apr 21, 2019

See the part on program_clauses_that_could_match in #214

Notes from comments that will probably be done outside of this PR:

@nikomatsakis nikomatsakis requested a review from scalexm Apr 22, 2019
@nikomatsakis nikomatsakis self-assigned this Apr 22, 2019
Copy link
Collaborator

nikomatsakis left a comment

Looks close -- not quite there. Left some comments.

src/db.rs Outdated
self.trait_datum(trait_ref.trait_id)
.to_program_clauses(self, &mut clauses);

// TODO sized, unsize_trait, builtin impls?

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Apr 23, 2019

Collaborator

Hmm, I had forgotten that we were handling these in this callback! Interesting.

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Apr 23, 2019

Collaborator

We'll have to think about how to structure said callbacks etc. I'm not wild about building that stuff into chalk-solve, but actually maybe it makes sense.

src/db.rs Outdated Show resolved Hide resolved
src/db.rs Outdated Show resolved Hide resolved
detrumi added 2 commits Apr 23, 2019
Copy link
Collaborator

nikomatsakis left a comment

I didn't look closely at the new program_clauses_for_XXX logic, but this looks very close! Exciting.

chalk-solve/src/solve/slg.rs Outdated Show resolved Hide resolved
chalk-solve/src/solve/slg/clause_visitor.rs Outdated Show resolved Hide resolved
src/db.rs Show resolved Hide resolved
detrumi added 2 commits Apr 25, 2019
chalk-solve/src/solve/slg.rs Outdated Show resolved Hide resolved
chalk-solve/src/clauses.rs Outdated Show resolved Hide resolved
DomainGoal::Holds(WhereClause::Implemented(trait_ref)) => {
program
.trait_datum(trait_ref.trait_id)
.to_program_clauses(program, &mut clauses);

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Apr 25, 2019

Collaborator

Something seems fishy here -- I'm surprised this never has to lower the impls of the given trait

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Apr 25, 2019

Collaborator

Indeed, in the rust source code, we do seem to lower the rules from the impl, although in a mildly confusing way.

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Apr 25, 2019

Collaborator

So I think we need something like

for impl_id in program.impls_for_trait(trait_ref.trait_id) {
    program.impl_datum(impl_id).to_program_clauses(program, &mut clauses);
}

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Apr 25, 2019

Collaborator

Actually i'm a bit confused by the rustc source, maybe @scalexm can explain. The comment says:

                // These come from:
                // * implementations of the trait itself (rule `Implemented-From-Impl`)
                // * the trait decl (rule `Implemented-From-Env`)

but the source seems to (a) lower the associated items from an impl and (b) not the trait declaration itself. I haven't looked more deeply though and I'm sure it all works out.

chalk-solve/src/clauses.rs Outdated Show resolved Hide resolved
chalk-solve/src/clauses.rs Outdated Show resolved Hide resolved
@nikomatsakis nikomatsakis marked this pull request as ready for review Apr 26, 2019
@nikomatsakis

This comment has been minimized.

Copy link
Collaborator

nikomatsakis commented Apr 26, 2019

@detrumi I removed the "draft status" because i was hoping that would trigger travis -- but I still don't see travis checks, so i'm not sure what's up with that

Copy link
Collaborator

nikomatsakis left a comment

Looking good, a few style nits -- do the tests pass?

chalk-solve/src/clauses/clause_visitor.rs Outdated Show resolved Hide resolved
@detrumi

This comment has been minimized.

Copy link
Contributor Author

detrumi commented Apr 26, 2019

do the tests pass?

Not yet, there's still a lot (42) of failing tests. Some "does not meet well-formedness requirements" errors, and a lot of diffs where no solutions were found.

There's also tests that fail around LocalImplAllowed, do we need to handle DomainGoal::IsLocal and such specially while matching? I wouldn't expect it, since that's not included in the rustc code either, but the tests are still failing...

failures:
    test::auto_trait_with_impls
    test::auto_trait_without_impls
    test::coherence::downstream_impl_of_fundamental_43355
    test::coherence::fundamental_traits
    test::coherence::overlapping_assoc_types_error
    test::coherence::two_blanket_impls_open_ended
    test::coinductive_semantics
    test::forall_projection
    test::forall_projection_gat
    test::fundamental_types
    test::gat_implied_bounds
    test::is_fully_visible
    test::local_and_upstream_types
    test::local_impl_allowed_for_traits
    test::mixed_indices_normalize_application
    test::mixed_indices_normalize_gat_application
    test::normalize_basic
    test::normalize_gat1
    test::normalize_gat2
    test::normalize_gat_with_higher_ranked_trait_bound
    test::normalize_gat_with_where_clause
    test::normalize_gat_with_where_clause2
    test::normalize_rev_infer
    test::normalize_rev_infer_gat
    test::normalize_under_binder
    test::projection_from_env
    test::projection_from_env_slow
    test::slg::cached_answers_1
    test::slg::cached_answers_2
    test::slg::cached_answers_3
    test::slg::contradiction
    test::slg::example_2_1_EWFS
    test::slg::example_2_2_EWFS
    test::slg::example_2_3_EWFS
    test::slg::example_3_3_EWFS
    test::slg::negative_answer_delayed_literal
    test::slg::negative_loop
    test::unselected_projection
    test::unselected_projection_with_gat
    test::unselected_projection_with_parametric_trait
    test::wf::assoc_type_recursive_bound
    test::wf::cyclic_wf_requirements

(edit: pushing that last commit finally triggered a build)

@nikomatsakis

This comment has been minimized.

Copy link
Collaborator

nikomatsakis commented Apr 26, 2019

@detrumi ok, cool -- yeah I guess there's nothing for it but to start digging in closer into the code, I guess we're just missing the right rules. Have you looked at the Lowering Rules rustc-guide chapter? Although, I suppose, it's probably not complete (sigh) -- I don't think we ever added the IsLocal and al lthat stuff to it.

This logic appeared to be adding a kind of "open-ended" program clause
for cases of `!1: Foo` -- it's not clear why we would do that.
@nikomatsakis nikomatsakis mentioned this pull request Apr 29, 2019
7 of 10 tasks complete
@nikomatsakis

This comment has been minimized.

Copy link
Collaborator

nikomatsakis commented Apr 30, 2019

This now passes tests on stable (fails on nightly due to Debug changes)

@scalexm
scalexm approved these changes May 2, 2019

// If we know that `T: Iterator`, then we also know
// things about `<T as Iterator>::Item`, so push those
// implied bounds too:

This comment has been minimized.

Copy link
@scalexm

scalexm May 2, 2019

Member

Right, this isn’t done in rustc right now because bounds on associated items are converted in where clauses like in where <Self as Trait>::Item: Foo, but we’ll need this once we have GATs and cannot make this conversion.

match domain_goal {
DomainGoal::FromEnv(from_env) => self.visit_from_env(from_env),
DomainGoal::Holds(WhereClause::ProjectionEq(ProjectionEq { projection, ty: _ })) => {
self.visit_projection_ty(projection)

This comment has been minimized.

Copy link
@scalexm

scalexm May 2, 2019

Member

I’m not sure this is useful; you would not have any useful implied bounds if you just have e.g. where ProjectionEq(<T as Iterator>::Item = HashSet<K>)

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis May 3, 2019

Collaborator

Maybe it's not needed. I'm experimenting with removing it.

@nikomatsakis nikomatsakis merged commit 25daf75 into rust-lang:master May 3, 2019
1 check failed
1 check failed
Travis CI - Pull Request Build Failed
Details
@detrumi detrumi deleted the detrumi:program_clauses_that_could_match branch May 4, 2019
bors bot added a commit to rust-analyzer/rust-analyzer that referenced this pull request May 4, 2019
Merge #1216
1216: Basic Chalk integration r=matklad a=flodiebold

This replaces the ad-hoc `implements` check by Chalk. It doesn't yet any new functionality (e.g. where clauses aren't passed to Chalk yet). The tests that exist actually work, but it needs some refactoring, currently crashes when running analysis on the RA repo, and depends on rust-lang/chalk#216 which isn't merged yet 😄 

The main work here is converting stuff back and forth and providing Chalk with the information it needs, and the canonicalization logic. Since canonicalization depends a lot on the inference table, I don't think we can currently reuse the logic from Chalk, so we need to implement it ourselves; it's not actually that complicated anyway ;) I realized that we need a `Ty::Bound` variant separate from `Ty::Param` -- these are two different things, and I think type parameters inside a function actually need to be represented in Chalk as `Placeholder` types.

~~Currently this crashes in the 'real' world because we don't yet do canonicalization when filtering method candidates. Proper canonicalization needs the inference table (to collapse different inference variables that have already been unified), but we need to be able to call the method candidate selection from the completion code... So I'm currently thinking how to best handle that 😄~~

Co-authored-by: Florian Diebold <flodiebold@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.