Skip to content

Change : freeze bounds #805

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

Merged
merged 18 commits into from
Oct 6, 2015
Merged

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Sep 25, 2015

There was a big hole in the type system so far in that lubs and glbs (and some other ops as well) used isSubType instead of isSubTypeWhenFrozen. Consequently, taking the lub of, say Set[A] and Set[B]
would have unified A and B! Keeps the constraints nice and tidy but is wrong. Fixing this uncovered several other problems which needed to be fixed in turn.

Review by @DarkDimius

@odersky
Copy link
Contributor Author

odersky commented Sep 25, 2015

Note that this PR is based on #802.

odersky added 15 commits October 1, 2015 19:33
Can assume P#T <: Q#T if P <: Q. This follows from
the rules how we expand # to existentials.
Some subtype tests should not instantiate type variables, in particular
those having to do with & and |.
withMode sets the whole mode, nit an individual bits. This was used
wrongly in several places. Make this less of a trap by renaming
withMode -> withModeBits.
Logging while printing messes up the recursion counts.
make successive underlying values an iterator (so that we
do not run into a stackoverflow in case of cycles).
Avoid redundant computations when already in printing mode.
We had problems printing constraints which are ill-formed, because
the basic operations & , | cause exceptions themselves. toString
serves as a fallback if show does not work.
Cases like these (in fact one of the operads was a type variable
the other its underlying polyparam) arose in pos/overloads.scala and caused
deep subtype recursions.
Checking whether two alternatives are the same should not unify
them by instantiating type variables.
Overall goal: Push backtracking deeper into the tree.
Iter2.scala fails with 6 errors, but succeeds once lubs and glbs do not try to
unify under invariant type constructors.
There is a diff, but a minor one. Instead of

    (T? >: Int <: Int)

we get

    (T? = Int) after pickling.
@odersky odersky force-pushed the change-freeze-bounds branch from aae7783 to 6497d02 Compare October 1, 2015 17:34
@odersky
Copy link
Contributor Author

odersky commented Oct 1, 2015

Rebased to master

@odersky
Copy link
Contributor Author

odersky commented Oct 2, 2015

If there are no more reviewing comments I am going to merge this later today.

@odersky
Copy link
Contributor Author

odersky commented Oct 2, 2015

@DarkDimius I added some explanations to the And/Or rewritings. I did see some code fail which got fixed by the rewriting, but currently all tests pass with or without the rewriting. So it would still be good to get a test which pinpoints the difference at some point. But since the PR is important for many other things it should not be held up by this, I think.

@odersky odersky force-pushed the change-freeze-bounds branch from 23e8437 to cd17e26 Compare October 2, 2015 17:42
@odersky
Copy link
Contributor Author

odersky commented Oct 5, 2015

@smarter

Hmm actually I don't think that will work, we can replace P by Q in P#T only if T is a member of Q, so we have to set variance = 0 to be conservative.

I did not understand that remark. Can you elaborate?

@DarkDimius DarkDimius mentioned this pull request Oct 5, 2015
According to the new subtyping rules,

    T <: U  ==>  T#X <: U#X and T#x <: U#x

Therefore type maps should treat the prefix of a named type
covariantly.
@odersky odersky force-pushed the change-freeze-bounds branch from 10c9a1e to 0a48b2a Compare October 6, 2015 11:52
@odersky
Copy link
Contributor Author

odersky commented Oct 6, 2015

I dropped the strawman from this PR, in order to resubmit this as a separate PR.

@odersky
Copy link
Contributor Author

odersky commented Oct 6, 2015

I now understood @smarter's argument why 0a48b2a is unsafe, so the last commit is reverted.

@odersky
Copy link
Contributor Author

odersky commented Oct 6, 2015

Going to merge now, I think we got all the reviews we could expect.

odersky added a commit that referenced this pull request Oct 6, 2015
@odersky odersky merged commit adc4a54 into scala:master Oct 6, 2015
@allanrenucci allanrenucci deleted the change-freeze-bounds branch December 14, 2017 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants