这是indexloc提供的服务,不要输入任何密码
Skip to content

Improve occurrence typing on Listof types #15

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

Closed
wants to merge 5 commits into from

Conversation

takikawa
Copy link
Contributor

This PR improves occurrence typing to use car/cdr information for Listof types. It also simplifies the types of operations like car and improves type inference a little bit.

Code review concerns: was there any reason this wasn't done before? (unsound maybe?) The new type for car seems simpler (only one case instead of two) but is it harder to understand for users?

Also, I haven't actually adjusted the types for all of the car variants yet (the changes are straightforward). I'll do that if code review checks out.

Review on Reviewable

These don't seem to be necessary

Fixup commit
@takikawa
Copy link
Contributor Author

It turns out the special case that I added initially for this PR was unnecessary. Changing the base environment types is enough.

Unfortunately, the tests don't pass yet because the change to the type of first causes this type error:

unit-tests/typecheck-tests.rkt:3081:22: Type Checker: Polymorphic function `first' could not be applied to arguments:
Argument 1:
  Expected: (U '() (Pairof a (Listof b)))
  Given:    (List g4803 ... B)

which boils down to this inference test failure (a new test that I added):

--------------------
All inference tests > Tests for infer > ((U '() (Pairof X (Listof Y)))) ((List A ... B))
((U '() (Pairof X (Listof Y)))) ((List A ... B))
FAILURE
location:   <pkgs>/typed-racket-test/unit-tests/infer-tests.rkt:274:4
Could not infer a substitution
--------------------

It seems like this case should succeed though, so I'm not sure what's going on. I'll look into it, but if anyone else has any ideas let me know.

@samth
Copy link
Member

samth commented Feb 6, 2015

@takikawa what's the status here?

@takikawa
Copy link
Contributor Author

takikawa commented Feb 6, 2015

@samth the status is that I'm stuck on the inference problem above. I'll update this PR when I fix it.

@endobson
Copy link
Contributor

Note that this is a backwards incompatible change, because instantiating the functions with A Any ... no longer extracts an A in many cases. If we are going to break backwards compatibility in this tiny case we should at least make the functions a lot simpler by making the types like cadr : (All (a) (Pair Any (Pair a Any))) with the appropriate Nulls.

@samth
Copy link
Member

samth commented Dec 6, 2017

Is this worth keeping open at this point?

@pnwamk
Copy link
Member

pnwamk commented May 20, 2018

This PR and the associated discussion may be useful to reference in the future, but leaving it as "open" seems incorrect at this point.

@pnwamk pnwamk closed this May 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants