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

When truncating a goal, don't truncate the environment #294

Merged
merged 2 commits into from Dec 9, 2019

Conversation

@jackh726
Copy link
Contributor

jackh726 commented Nov 22, 2019

Fixes #280
Fixes #289

Not sure if this is the correct fix. It does solve the issue and, conceptually, I don't see a particular reason why we need to truncate the environment of a goal. However, there may be other cases we could theoretically still enter an infinite loop and this doesn't necessarily protect against that.

@matklad

This comment has been minimized.

Copy link
Member

matklad commented Nov 24, 2019

I've tried to compile rust-analyzer with this PR, and it indeed fixes the issue

λ cargo run -p ra_cli --release -- analysis-stats --with-deps ../chalk/
    Finished release [optimized] target(s) in 0.08s
     Running `target/release/ra_cli analysis-stats --with-deps ../chalk/`
Database loaded, 143 roots, 181.260377ms
Crates in this dir: 174
Total modules found: 1896
Total declarations: 144924
Total functions: 38502
Item Collection: 11.188857545s, 0b allocated 0b resident
Total expressions: 781907
Expressions of unknown type: 83740 (10%)
Expressions of partially unknown type: 47814 (6%)
Type mismatches: 28907
Inference: 211.9265556s, 0b allocated 0b resident
Total: 223.115420171s, 0b allocated 0b resident
@matklad

This comment has been minimized.

Copy link
Member

matklad commented Nov 26, 2019

NOTE: I've switched rust-analyzer to use this commit

@95th

This comment has been minimized.

Copy link

95th commented Nov 29, 2019

@Speedy37

This comment has been minimized.

Copy link

Speedy37 commented Dec 4, 2019

@matklad Same for rust-analyzer/rust-analyzer#2313, no more infinite loop for me.

@nikomatsakis

This comment has been minimized.

Copy link
Collaborator

nikomatsakis commented Dec 7, 2019

OK, so, the solution I had in mind was a bit different. We still permitted truncating the environment, but we also permitted unification of unnormalized projection types with inference variables.

This solution makes a lot of sense, but it also has some drawbacks. For example, if you have a Chalk goal like

if(C) { ... }

then the clause C moves into the environment. This clause could even have inference variables etc in it. It seems like it may be possible to trigger infinite recursion then by "sneaking" a clause into the environment with an inference variable that looks small but gets unified to something very large.

Let me open an alternative PR. We should definitely land something (also, rust-analyzer should probably increase the size of its max-types limit, 4 is absurdly low.)

@nikomatsakis

This comment has been minimized.

Copy link
Collaborator

nikomatsakis commented Dec 7, 2019

Alternative PR: #305 but I have concerns about that approach, too =)

@nikomatsakis

This comment has been minimized.

Copy link
Collaborator

nikomatsakis commented Dec 9, 2019

We decided to go with this PR -- for now I created #306 to track my concern for the future.

This test dies with an overflow. With CHALK_DEBUG=1 this death is
quick, without it, it is slow and painful. The problem is that some of
the types in the environment are "truncated" and replaced with
inference variables, but those types are unnormalized
projections. When we then go to unify the environment found in the
answer, we end up needing to unify inference variables with
unnormalized projections -- but the current code creates subgoals
instead, which leads to an infinite recursion.
@nikomatsakis nikomatsakis merged commit 151949d into rust-lang:master Dec 9, 2019
1 check passed
1 check passed
Format
Details
@jackh726

This comment has been minimized.

Copy link
Contributor Author

jackh726 commented Dec 9, 2019

@nikomatsakis did you mean to duplicate the test?

@nikomatsakis

This comment has been minimized.

Copy link
Collaborator

nikomatsakis commented Dec 10, 2019

@jackh726 I didn't because I couldn't quite tell if you were testing for something different. But probably it makes sense to do so.

@jackh726

This comment has been minimized.

Copy link
Contributor Author

jackh726 commented Dec 11, 2019

@nikomatsakis they actually are the same test. I think we probably want to move the tests in tests/slg into other files eventually anyways. The test macros are so similar, it almost makes things more complicated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.