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

Backport compiler determinism to 2.12.x [ci: last-only] #7203

Open
wants to merge 15 commits into
base: 2.12.x
from

Conversation

Projects
None yet
6 participants
@retronym
Member

retronym commented Sep 12, 2018

Backports:

Which together resolve scala/scala-dev#405

A risk of the backport is that we create binary incompatibilities
with code compiled with -opt:inline that refers to fresh names
that are now changed. I had this problem with partest on 2.13.x:

scala/sbt-scala-module@7aee2b5#diff-b296be07ef3dd86a176475283d8583a4

Another risk is that there are serialization incompatibilities
that aren't covered by SerializationStabilityTest.

@scala-jenkins scala-jenkins added this to the 2.12.7 milestone Sep 12, 2018

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Sep 12, 2018

Member

Going through the reviews of the 2.13 versions. Some concerns (will edit as I make my way through):

  • should we avoid breaking macro annotations? 87be453
Member

adriaanm commented Sep 12, 2018

Going through the reviews of the 2.13 versions. Some concerns (will edit as I make my way through):

  • should we avoid breaking macro annotations? 87be453

@adriaanm adriaanm modified the milestones: 2.12.7, 2.12.8 Sep 12, 2018

@adriaanm adriaanm self-assigned this Oct 16, 2018

retronym and others added some commits Jan 24, 2018

Fix lookup of default getter in scope
By slightly modifying an existing test to force creation of
default getters for both `bar` methods _before_ typechecking the
application, I was able to show a latent bug in the way the default
getter is looked up in scope.

The bespoke `Context.lookup` method did not respect shadowing,
but rather considered the two, same-named default getters as
overloaded. Because the overloaded symbol had NoSymbol as its
owner, which didn't match the expected owner, neither default
was eligible.

This commit brings the code more into line with `Context.lookupSymbol`
and respects shadowing.

(cherry picked from commit 86f2028)
Refactor default getter synthesis
  - Factor out differences between constructors and regular methods
    into an virtual call to a helper class
  - Tease apart symbol creation/entry from synthesis of the default
    getter tree to prepare for a subsequent commit that will perform
    the first part eagerly.
  - Add a test to show the unstable order of the default getter
    symbols in the owner's scope.

(cherry picked from commit f44e1bf)
Refactor pickle phase
Small refactoring designed to make the subsequent
commit more reviewable.

Tightens up the definition of "companion"-s so that
we no longer add a type alias in a package object into
the pickle of a same-named module.

(cherry picked from commit b41e651)
Unify scope lookup for companions and default getters
In #5700, I fixed a bug in the companion lookup, which ensured
they were defined in the same scope.

The same approach applies well to the lookup of default getters.

You may ask, we can't just use:

```
context.lookupSymbol(name, _.owner == expectedOwner)
```

That doesn't individually lookup the entry in each enclosing
nested scopes, but rather relies on the outer scope delegation
in `Scope.lookupEntry` itself. This in turn relies on the way that
nested scopes share the `elems` table with the enclosing scope:

```
  final def newNestedScope(outer: Scope): Scope = {
    val nested = newScope
    nested.elems = outer.elems
    nested.nestinglevel = outer.nestinglevel + 1
    ...
  }
```

If the outer scope is later mutated, in our case by lazily adding
the default getter, the inner scope won't see the new elems.
Context.lookupSymbol will jump immediately jump to search of the
enclosing prefix.

Perhaps a better design would be for the inner scope to retain a
reference to the outer one, rather than just to the head of its
elems linked list at the time the nested scope was created.

(cherry picked from commit da14e9c)
scalap should not print class type param annots in decls
The annotation removed in this diff was actually from `R`!

(cherry picked from commit 623589a)
Eagerly enter default getters into scope
This stabilizes the order they appear in the owners scope. Previously,
their order was goverened by the order that the methods bearing
default parameters were type completed.

Make macro annotations compatible with these changes

(cherry picked from commit 87be453)
Preserve order of decls through pickle/unpickle
The pickle format does not explicitly enccode the order of
decls. Instead, symbols are entered into an index in the
order that they are found by the pickler, either as a definition
or as a reference. During unpickling, symbols are read and entered
into the owner's decls in that order.

This is a cause of unstable compiler output: a class that mixes
in members from some trait will have a different order if it is
compiled jointly with / separately from that trait.

This commit modifies the pickler with an initial pass that reserves
index entries for all declarations in the declaration order.

The pickle format and the unpickler are unchanged.

(cherry picked from commit 8cc7e56)
Update comment in test
(cherry picked from commit befc337)
Stable names for lambda lifted method and fresh names
Fresh names are created using a FreshNameCreator, which appends
an increasing number to the given prefix.

```
scala> val fresh = new scala.reflect.internal.util.FreshNameCreator()
fresh: scala.reflect.internal.util.FreshNameCreator = scala.reflect.internal.util.FreshNameCreator@42b84286

scala> List("foo$", "bar$", "foo$").map(fresh.newName(_))
res1: List[String] = List(foo$1, bar$1, foo$2)
```

Each compilation unit had its own fresh name creator, which is used
in the regular compiler. Macros and quasiquotes make use of a global
creator (at least, as of #3401).

Both of these are too broadly scoped to help achieve deterministic
fresh names: if sources are recompiled in a different order or separately
recompiled, the fresh name counters can be different. Methods in a given
compilation unit are not necessarily typechecked in a linear fashion;
they might be typechecked ahead of time to provide an inferred type
to a caller.

This commit:

  - Changes all known fresh name creations within the typer phase (in which
    out-of-order typechecking is a factor) to use a fineer grained fresh
    name creator. How fine grained? A fresh name generated as some position
    `p` shares the fresh name generator scoped at the closest method or
    class that encloses that the outermost enclosing tree at the same
    position. This definition is designed to give a shared fresh name
    creator for all fresh names generated in `macro1(macro2())`, even if
    the fresh names are requiested from with a Typer in the macro enclosed
    by a synthetic method.
  - Changes macro fresh names to use the same fresh naming scheme as the regular
    typechecker. An opt-out compiler option allows the old behaviour, but I'm
    interested to find real-world cases where the new scheme actually causes
    a problem

In addition, a small change is made to lambda lift to lift local methods in the
order that they are encountered during traversal, rather than sorting them
based on `Symbol.isLess` (which include `Symbol.id`, an order-of-typechecking
dependent value).

(cherry picked from commit 69d60cb)
Don't copy the OVERRIDE flag in refinement decls
typedRefinement defers the setting of this flag until the end
of the compilation unit, which means that inferred types that are
derived from the written refinement type can be unstable depending
on whether they were computed before or after the flag was set.

An alternative fix might be to just remove the setting of OVERRIDE
in typedRefinement.unitToCheck.

(cherry picked from commit f6ca3dd)
Report an error on problematic use of unit.fresh
I've used this to flush out the corner cases fixed in the previous commit.

(cherry picked from commit dfaefa0)
Tone error down to a dev warning
(cherry picked from commit c5cc71f)
Defer parameter alias computation until the end of typechecking
Previously, it was done while typechecking super calls, and would fail
to see the fact that a yet-to-be-typechecked super constructor itself
had a parameter aliased by a grand-parent class.

(cherry picked from commit 3ae11c1)
Make quasiquote pattern matching deterministic
As per the discussion at 7184fe0#r29651930,
all we want to avoid is name collision among the holes in a single
pattern. For that, `c.freshName(<a constant>)` itself is sufficient
and the randomness is not needed. This is what scala.meta does,
and works just as well.

This fixes scala/bug#11008

(cherry picked from commit 0336c14)
@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm
Member

adriaanm commented Oct 16, 2018

@mkeskells

This comment has been minimized.

Show comment
Hide comment
@mkeskells

mkeskells Oct 16, 2018

Contributor

Great to see this being backported.
This will open up new possibilities with our CI plant both for reductions in compile and for test

Contributor

mkeskells commented Oct 16, 2018

Great to see this being backported.
This will open up new possibilities with our CI plant both for reductions in compile and for test

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Oct 17, 2018

Member

I added a community build for 2.12.8-bin-e764204-SNAPSHOT to the queue (https://scala-ci.typesafe.com/job/scala-2.12.x-integrate-community-build/3650). It's using a custom config (adriaanm/community-builds@bddc889) that uses macro-paradise at https://github.com/scalamacros/paradise/tree/2.12.8 (I incorporated the community build fixes)

/cc @SethTisue FYI on queuing the build

Member

adriaanm commented Oct 17, 2018

I added a community build for 2.12.8-bin-e764204-SNAPSHOT to the queue (https://scala-ci.typesafe.com/job/scala-2.12.x-integrate-community-build/3650). It's using a custom config (adriaanm/community-builds@bddc889) that uses macro-paradise at https://github.com/scalamacros/paradise/tree/2.12.8 (I incorporated the community build fixes)

/cc @SethTisue FYI on queuing the build

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Oct 17, 2018

Member

After a green community build, this is ready to merge!

Member

adriaanm commented Oct 17, 2018

After a green community build, this is ready to merge!

@SethTisue SethTisue self-assigned this Oct 17, 2018

@SethTisue

This comment has been minimized.

Show comment
Hide comment
@SethTisue

SethTisue Oct 18, 2018

Member

there were some failures in run 3650. the enumeratum one, at least, looks spurious, but let's see how a rebuild does:
https://scala-ci.typesafe.com/view/scala-2.12.x/job/scala-2.12.x-integrate-community-build/3652/

Member

SethTisue commented Oct 18, 2018

there were some failures in run 3650. the enumeratum one, at least, looks spurious, but let's see how a rebuild does:
https://scala-ci.typesafe.com/view/scala-2.12.x/job/scala-2.12.x-integrate-community-build/3652/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment