Skip to content

Fix #2948: Use symbol's info when mapping inherited denotations #2970

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 14 commits into from
Aug 16, 2017

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Aug 10, 2017

The test case shows that it is inadmissible to combine the infos of inherited denotations
into a new denotation. The problem here is that a type parameter CC was instantiated in
an inherited denotation to Traversable, yet the parameter was afterwards instantiated to
ListBuffer. This shows that infos from inherited denotations are useless; instead we have
to go back to the inherited symbol's infos and map them with an asSeenFrom.

This fix also shows that one of the reasons for abandoning #2947 was wrong. We cannot form
denotations from parent denotations in any case, so instantiating early should be fine with
the change in this commit.

Based on #2945

odersky added 14 commits August 9, 2017 10:23
Approximating type maps now work with bounds instead of NoTypes. This
gives more precision (in particular for type applications), and also
makes it possible to combine approximation with other mappings.

As a side-effect we provide the hooks for preventing constructing
illegal types C#A where C is non-singleton and A is abstract by
setting variance to 0 for the prefix of an abstract type selection.

There's one test case that fails: One of the types in
dependent-exractors.scala does not check out anymore. This has
likely to do with the loss of precision incurred by the maps.
Exact cause remains to be tracked down.
A Range, if it survives, will always lead to a situation
where the upper bound appears in a covariant position
in the result, and the lower bound appears in a contravariant
position. Hence, when we apply a type map to the
argument in a range we should take this into account.
Fixes a previous failure in t2435.scala and dependent-extractors.scala.
Supersedes old scheme of dealing with unstable prefixes in non-variant positions.
It's no longer needed.
 - Use range instead of Range in AsSeenFromMap#apply. We needed Range
   before because we did an incorrect variance computation for NamedTypes.

 - Refine derivedSelect
This puts avoid on firmer ground.
 - Add documentation
 - Drop debug code
 - Tweak what we do in the TypeVar case to align
   with interpolation.
In a commonly used derived... operation, deal with the common
case that the types have not chanegd first.
The test case shows that it is inadmissible to combine the infos of inherited denotations
into a new denotation. The problem here is that a type parameter CC was instantiated in
an inherited denotation to Traversable, yet the parameter was afterwards instantiated to
ListBuffer. This shows that infos from inheroted denotations are useless; instead we have
to go back to the inherited symbol's infos and map them with an asSeenFrom.

This fix also shows that one of the reasons for abandoning scala#2947 was wrong. We cannot form
denotations from parent denotations in any case, so instantiating early should be fine with
the change in this commit.
@odersky
Copy link
Contributor Author

odersky commented Aug 11, 2017

@felixmulder CI seems to hang here. How can we unblock it?

@smarter
Copy link
Member

smarter commented Aug 11, 2017

@odersky I restarted drone and the tests for your PR.

@felixmulder
Copy link
Contributor

Looks like it's only running one job concurrently. Will try to fix it now.

@felixmulder
Copy link
Contributor

Alright, should be unblocked now. Guillaume and I both jumped on this at the same time, so we ended up killing and starting docker images left and right 😂

@odersky odersky requested a review from nicolasstucki August 16, 2017 12:23
@odersky
Copy link
Contributor Author

odersky commented Aug 16, 2017

Only last two commits need to be reviewed.

@nicolasstucki
Copy link
Contributor

Last two commits LGTM

@odersky odersky merged commit 9c4b88d into scala:master Aug 16, 2017
@allanrenucci allanrenucci deleted the fix-#2948 branch December 14, 2017 19:18
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.

4 participants