Skip to content

Rework type inference #4080

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 19 commits into from
Mar 7, 2018
Merged

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Mar 6, 2018

This is an attempt to put type inference on a more solid basis. The main two commits are

Drop Ephemeral 90a661b

Changing the interpolation scheme uncovered several cache invalidation problems with

  • the asSeenFrom cache in Denotations
  • the superType cache in AppliedType
  • the lastDenotation cache in NamedType

The new interpolation scheme performed essentially the same operations as the old one, but sometimes in a different order. I am still not quite sure how the differences made the cache invalidations fail. On the other hand, it is quite plausible (obvious even, in retrospect) that the previous invalidation schemes are incomplete. So this commit replaces them with a common algorithm that does not rely on the previous global state represented by ephemeral.

New interpolation scheme 417f0c8

Variable interpolation "improves" types by instantiating type variables that do not appear in the result of a type derivation, or that appear only co- or only contra-variantly. This is convenient because it keeps the constraints small. It is also necessary since some operations have more or better solutions once type variables are instantiated. For instance, the members of a type
variable

X >: T <: U

are the members of U. But assuming X is covariant once it is instantiated to T we get more members. Similarly for implicit searches.

The major changes from the previous interpolation scheme are the following:

  • We explicitly keep track in typer of which variables should and
    which should not be interpolated. This replaces searching trees
    for embedded variable definitions, which is fragile e.g. in the
    presence of eta expansion.
  • We compute variances starting with all variables found in the type,
    not just the qualifying ones. The previous scheme caused some
    variance information to be missed, which caused some variables
    to be mis-classified as non-occurring. i4032.scala is a test case.
    Unfortunately, fixing this caused several other tricky inference
    failures which were previously hidden because some variables
    were already instantiated prematurely. Examples were hmap.scala,
    hmap-covariant.scala, and i2300.scala. In all these cases there was another
    problem which was masked by the fact that some type variables had
    already been instantiated where they should not have been.
  • We interpolate at the end of typedUnadapted instead of at the
    beginning of adapt. Tracking instantiatable variables turned out
    to be easier this way.

x || t.mightBeProvisional && {
t.mightBeProvisional = t match {
case t: TypeVar =>
!t.inst.exists
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An instantiated TypeVar could have its underlying type refer to another uninstantiated TypeVar and therefore still be provisional I think.

@odersky
Copy link
Contributor Author

odersky commented Mar 6, 2018

test performance please

@dottybot
Copy link
Member

dottybot commented Mar 6, 2018

performance test scheduled: 1 job(s) in queue, 0 running.

@dottybot
Copy link
Member

dottybot commented Mar 7, 2018

performance test failed:

Error line number: 24

[check /data/workspace/bench/logs/pull-4080-03-07-00.16.out for more information]

@odersky
Copy link
Contributor Author

odersky commented Mar 7, 2018

test performance please

@dottybot
Copy link
Member

dottybot commented Mar 7, 2018

performance test scheduled: 1 job(s) in queue, 0 running.

@dottybot
Copy link
Member

dottybot commented Mar 7, 2018

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/4080/ to see the changes.

Benchmarks is based on merging with master (4be2e76)

@odersky
Copy link
Contributor Author

odersky commented Mar 7, 2018

test performance please

@dottybot
Copy link
Member

dottybot commented Mar 7, 2018

performance test scheduled: 1 job(s) in queue, 0 running.

odersky added 15 commits March 7, 2018 15:31
Changing the interpolation scheme uncovered several cache invalidation problems with

 - the asSeenFrom cache in Denotations
 - the superType cache in AppliedType
 - the lastDenotation cache in NamedType

The new denotation scheme performed essentially same operations as the old one, but sometimes
in a different order. I am still not quite sure how the differences made the cache invalidations
fail. On the other hand, it is quite plausible (obvious even, in retrospect) that the previous
invalidation schemes are incomplete. So this commit replaces them with a common algorithm that
does not rely on the previous global state represented by `ephemeral`.
Allow to define what gets shown as a result on a backtrace
The previous scheme created a `val showOp = <some closure>` value for each trace
operation. It was unused if tracing was disabled. Still might be better to
avoid its creation in the first place.
-Yshow-no-inline suppresses "inlined from" parts when printing trees. This is
useful when one has deeply inlined structures, as is the case when looking at `trace`ed code.
Used for small, linked sets. Normal immutable sets
are about as fast for 0 - 4 elements, but are not linked
for larger sizes.
Identify them by number. Helps in the same way other fixed numbering schemes
help understand debug output.
 - Fix isProvisional condition for TypeVars
 - Force recomputation via memberDenot in NamedType if previous prefix was provisional
Need to follow up later on what caused it to fail.
Avoids needlessly complicated inferred types such as

    C[_ >: 1.type <: Singleton]

by detecting that that this is equivalent to `C[1.type]`.
Hashes ruin diffability; replace them with the serial `id` numbers.
We missed some cases before.
Major changes from previous one:

 - We explicitly keep track in typer of which variables should and
   which should not be interpolated. This replaces searching trees
   for embedded variable definitions, which is fragile e.g. in the
   presence of eta expansion.
 - We compute variances starting with all variables found in the type,
   not just teh qualifying ones. The previous scheme caused some
   variance information to be missed, which caused some variables
   to be mis-classified as non-occurring. i4032.scala is a test case.
   Unfortunately, fixing this caused several other tricky inference
   failures because which were previously hidden because some variables
   were already instantiated prematurely. Examples were hamp.scala,
   hmap-covariant.scala, and i2300.scala.
 - We interpolate at the end of typedUnadapted instead of at the
   beginning of `adapt`. Managing instantiatable variables turned out
   easier this way.
@odersky odersky force-pushed the change-interpolation-3 branch from 1060d12 to 04d1588 Compare March 7, 2018 14:37
@odersky
Copy link
Contributor Author

odersky commented Mar 7, 2018

Rebased

@odersky odersky changed the title Drop ephemeral Rework type inference Mar 7, 2018
@dottybot
Copy link
Member

dottybot commented Mar 7, 2018

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/4080/ to see the changes.

Benchmarks is based on merging with master (bdfe740)

Call simplify and interpolateTypeVars in the same situations.
@odersky odersky force-pushed the change-interpolation-3 branch from 04d1588 to a6e1d19 Compare March 7, 2018 14:59
@odersky
Copy link
Contributor Author

odersky commented Mar 7, 2018

test performance please

@dottybot
Copy link
Member

dottybot commented Mar 7, 2018

performance test scheduled: 1 job(s) in queue, 1 running.

Copy link
Member

@smarter smarter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it!

@@ -76,6 +76,11 @@ class TyperState(previous: TyperState /* | Null */) {
/** The uninstantiated variables */
def uninstVars = constraint.uninstVars

/** The set of uninstantiated type varibles which have this state as their owning state */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: varibles -> variables

trace(i"adapting ${tree.showSummary}: ${tree.tpe} to $pt", show = true) {
assert(ctx.phase == ctx.erasurePhase.next, ctx.phase)
assert(ctx.phase == ctx.erasurePhase || ctx.phase == ctx.erasurePhase.next, ctx.phase)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why. Some transform ran at phase erasure and used the erasure typer to do it. Seems legit. Not sure why it did not run before.

* Y <: X
*
* Then `Y` also occurs co-variantly in `T` because it needs to be minimized in order to constrain
* `T` teh least. See `variances` for more detail.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: teh -> the

@dottybot
Copy link
Member

dottybot commented Mar 7, 2018

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/4080/ to see the changes.

Benchmarks is based on merging with master (bdfe740)

@odersky odersky merged commit 14fb071 into scala:master Mar 7, 2018
@allanrenucci allanrenucci deleted the change-interpolation-3 branch March 8, 2018 08:23
smarter added a commit to dotty-staging/dotty that referenced this pull request Mar 8, 2018
Type variable instantiation should only occur in Typer, TreeChecker and
during exhaustiveness checking. This was not enforced until now, and wa
not always true before scala#4080.
smarter added a commit to dotty-staging/dotty that referenced this pull request Mar 8, 2018
Type variable instantiation should only occur in Typer, TreeChecker and
during exhaustiveness checking, this means that no uninstantiated type
variable should exist outside of these phases. This was not enforced
until now, and was not always true before scala#4080.
smarter added a commit to dotty-staging/dotty that referenced this pull request Mar 8, 2018
Type variable instantiation should only occur in Typer, TreeChecker and
during exhaustiveness checking, this means that no uninstantiated type
variable should exist outside of these phases. This was not enforced
until now, and was not always true before scala#4080.
smarter added a commit to dotty-staging/dotty that referenced this pull request Mar 8, 2018
Type variable instantiation should only occur in Typer, TreeChecker and
during exhaustiveness checking, this means that no uninstantiated type
variable should exist outside of these phases. This was not enforced
until now, and was not always true before scala#4080.
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