Skip to content

Fix/erasure 2 #128

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 36 commits into from
May 9, 2014
Merged

Fix/erasure 2 #128

merged 36 commits into from
May 9, 2014

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Apr 29, 2014

Not a complete fix for erasure yet. But we can now do -Ycheck:frontend and ensure
that typechecked trees typecheck.

Before attacking erasure, we should merge this pull request and then check all other phases one by one with -Ycheck to make sure their output is type-correct.

Question: Should we enable -Ycheck:all in the checkin tests? The only downside is that it could slow down tests too much. Maybe enable it only for some tests?

@odersky
Copy link
Contributor Author

odersky commented Apr 29, 2014

Review by @DarkDimius and @sjrd please. @DarkDimius, can you shepard this please? And once it is in start -Ychecking the initial transformation phases? Thanks!

@retronym
Copy link
Member

Question: Should we enable -Ycheck:all in the checkin tests? The only downside is that it could slow down tests too much. Maybe enable it only for some tests?

Would it be feasible to parallelize -Ycheck:_ with the regular compiler phase, given that it is read-only?

@xeno-by
Copy link

xeno-by commented Apr 29, 2014

The checker is probably read-only, but are regular compiler phases read-only?

@odersky
Copy link
Contributor Author

odersky commented Apr 29, 2014

In fact, I think can get away with using -Ycheck for only two phases. Once just before erasure and once just before the backend. I believe it's relatively unlikely that one of the other transformations would heal a type error introduced by a previous transformation. If one of the two checks fails, we can always drill down with -Ycheck:all to identify the culprit.

@odersky
Copy link
Contributor Author

odersky commented May 3, 2014

I noticed that -Ycheck indeed does flag type-incorrect code produced by the next phases (both LazyVals and TailCalls). So, I think it's important to get this enabled to make progress.

@DarkDimius
Copy link
Contributor

I'm on it.

@DarkDimius
Copy link
Contributor

Most errors in LazyVals come from fact that if one creates
tpd.ModuleDef(moduleSymbol, List(EmptyTree))
with a single parent class for moduleSymol - defn.ObjectClass.typeRef
The result will be incorrect twice:

  1. trees won't have any position set which fails in Typer.scala:955;
  2. constructor created for module val is incorrect: it has trees with unassigned types(parent class constructor invocation);

@odersky
Copy link
Contributor Author

odersky commented May 3, 2014

Not clear whether we should automatically assign a position in ModuleDef. Does the symbol have a position? If not, there's no way anyway to do this and we need to set positions manually.

If we do decide to have tpd manage some positions automatically, what's the rule that decides which positions are assigned that way?

I am not sure what the constructor problem is.

@DarkDimius
Copy link
Contributor

Regarding moduleDef - maybe add additional position argument to tpd.ModuleDef?

Constructor problem(and failed test) - https://gist.github.com/DarkDimius/120fe85bfea08c47a8c9
To make it run until this problem and not fail before fixes from #124 are required.

@odersky
Copy link
Contributor Author

odersky commented May 3, 2014

I don't want to add pos parameters to tpd.ModuleDef. It's more regular to just write

tpd.ModuleDef(...) withPos pos

The only option we have is whether we could maybe retrieve the position from the symbol, if it has one.

I'll have a look at the constructor problem.

@DarkDimius
Copy link
Contributor

tpd.ModuleDef(...) withPos pos without withPos seems to just be invalid, and existence of method with constructs invalid tree(and even kind-of checks it during type assignment) wich fails in later stage feels arguable and misleading for people joining development.
withPos seems to be a method that should be rarely used, but with ModuleDef it should be used in most cases to create valid trees, so, why not pos to signature?

@odersky
Copy link
Contributor Author

odersky commented May 3, 2014

On Sat, May 3, 2014 at 1:11 PM, Dmitry Petrashko
[email protected]:

tpd.ModuleDef(...) withPos pos without withPos seems to just be invalid,
and existence of method with constructs invalid tree(and even kind-of
checks it during type assignment) wich fails in later stage feels arguable
and misleading for people joining development.
withPos seems to be a method that should be rarely used, but with
ModuleDef it should be used in most cases to create valid trees, so, why
not pos to signature?

Because you'd need pos parameters everywhere and it's redundant. In most
cases you can just do a withPos at the root of the tree. It will fill in
the positions of child trees automatically. We absolutely need to strive
for "one way to do it" instead of convenience here. "Many ways to do it"
inevitably leads to a mess. The issue is less convenience but how do we
force people who are not yet fully educated in compiler internals on the
right path? Test a lot and fail early is an important ingredient of that.

Reply to this email directly or view it on GitHubhttps://github.com//pull/128#issuecomment-42102400
.

Martin Odersky

EPFL

JOIN US. REGISTER TODAY! http://www.scaladays.org/
Scala http://www.scaladays.org/
Days http://www.scaladays.org/
June 16th-18th, http://www.scaladays.org/
Berlin http://www.scaladays.org/

@odersky
Copy link
Contributor Author

odersky commented May 5, 2014

I found the constructor problem. It's produced by Ycheck itself (in Typer#classDef). Fix coming shortly.

@gzm0
Copy link
Contributor

gzm0 commented May 6, 2014

This branch doesn't compile.

> test:compile
[info] Updating {file:/Users/tschlatt/Documents/dotty/}dotty...
[info] Resolving jline#jline;2.11 ...
[info] Done updating.
[info] Compiling 142 Scala sources to /Users/tschlatt/Documents/dotty/target/scala-2.11.0-RC3/classes...
[warn] there were 1 unchecked warning(s); re-run with -unchecked for details
[warn] one warning found
[info] Compiling 24 Scala sources to /Users/tschlatt/Documents/dotty/target/scala-2.11.0-RC3/test-classes...
[error] /Users/tschlatt/Documents/dotty/test/dotc/SigTest.scala:24: could not find implicit value for parameter defaultOptions: List[String]
[error]   @Test def pos_Sig = compileFile(posDir, "ScalaSignature")
[error]                                  ^
[warn] /Users/tschlatt/Documents/dotty/test/test/showClass.scala:4: imported `Symbols' is permanently hidden by definition of trait Symbols in package test
[warn] import dotty.tools.dotc.core.Symbols
[warn]                              ^
[warn] one warning found
[error] one error found
[error] (test:compile) Compilation failed
[error] Total time: 48 s, completed May 6, 2014 9:19:04 AM
> 

@odersky
Copy link
Contributor Author

odersky commented May 6, 2014

Can you check again? This looks like a merge error on checkout. Travis says
it compiles.

On Tue, May 6, 2014 at 9:19 AM, Tobias Schlatter
[email protected]:

This branch doesn't compile.

test:compile
[info] Updating {file:/Users/tschlatt/Documents/dotty/}dotty...
[info] Resolving jline#jline;2.11 ...
[info] Done updating.
[info] Compiling 142 Scala sources to /Users/tschlatt/Documents/dotty/target/scala-2.11.0-RC3/classes...
[warn] there were 1 unchecked warning(s); re-run with -unchecked for details
[warn] one warning found
[info] Compiling 24 Scala sources to /Users/tschlatt/Documents/dotty/target/scala-2.11.0-RC3/test-classes...
[error] /Users/tschlatt/Documents/dotty/test/dotc/SigTest.scala:24: could not find implicit value for parameter defaultOptions: List[String]
[error] @test def pos_Sig = compileFile(posDir, "ScalaSignature")
[error] ^
[warn] /Users/tschlatt/Documents/dotty/test/test/showClass.scala:4: imported `Symbols' is permanently hidden by definition of trait Symbols in package test
[warn] import dotty.tools.dotc.core.Symbols
[warn] ^
[warn] one warning found
[error] one error found
error Compilation failed
[error] Total time: 48 s, completed May 6, 2014 9:19:04 AM


Reply to this email directly or view it on GitHubhttps://github.com//pull/128#issuecomment-42273691
.

Martin Odersky

EPFL

JOIN US. REGISTER TODAY! http://www.scaladays.org/
Scala http://www.scaladays.org/
Days http://www.scaladays.org/
June 16th-18th, http://www.scaladays.org/
Berlin http://www.scaladays.org/

@gzm0
Copy link
Contributor

gzm0 commented May 6, 2014

Whoups. Indeed. Lingering file in the repo. Sorry for the noise.

@DarkDimius
Copy link
Contributor

fixed regressions in LazyVals tests.

@DarkDimius
Copy link
Contributor

@odersky have we agreed on adding documentation for synthesizing Object and Any?

@odersky
Copy link
Contributor Author

odersky commented May 7, 2014

yes, i'll add something shortly.

odersky added 10 commits May 8, 2014 21:47
Need to allow for possibility that term ref does not have a symbol (e.g. termrefs with union prefixes do not
always have symbols).
There was a mismatch between symbol and signature before.
We cannot have same named methods defined in Object and Any because after erasure
the Any references get remapped to the Object methods which would result in a double binding
assertion failure.

Instead we do the following:

 - Have some methods exist only in Any, and remap them with the Erasure denotation
   transformer to be owned by Object.
 - Have other methods exist only in Object.

To achieve this, we synthesize all Any and Object methods; Objetc methods no longer get
loaded from a classfile.

There's a complication with getClass. We need to reconsider what the best treatment of getClass is.
Right now there's too much magic going on for my taste. It might be better to leave getClass on Object only as it
is in Java, forget about the special treatement of its type, and have another getClass like method in
an decorator on class Any. That could produce the right types and could also work for primitive types.
We need to take into account that symbols can can have non-class owners yet still be valid.
Previously such symbols were treated as invalid. Now they are valid if their owners are valid.

The commit supersedes

29976d7

which got reverted.
Used for debugging purposes for now. Might be used for replaying IDE interactions later.
Add test whether we are after typer. Use it to assert that
eta expansion and implicit search do not happen after typer.
Will now print the full chain of instances using ? for uninstantiated and ' for
instantiated.
The check assumes that inferred TypeTrees do not exist yet, but after
the typer they do exist.
odersky and others added 23 commits May 8, 2014 21:48
If a type variable TV1 is instantiated to a poly param, and the poly param
is itself accomanied by a type variable TV2, then the instance of TV1 should
be TV2 and not the original poly param. Otherwise we lose instantiations. This
is demonstrated by running dotc on the .scala files in the dotc directory itself
with option -Ycheck.
In dotc, AnyVal is a synthetic toplevel class. We need to make sure it is not loaded by
the classfile parser from scala.AnyVal. To do this, the companion module class needs to be
marked as absent (by setting its info to NoType).

Also, added Repeated annotation which will be used in the next commit.
Previously, repeated parameters were typed as `<repeated>[T]`. The
method `underlyingWithRepeated` converts `<repeated>[T]` to `Seq[T]`.
This method was called in typedIdent, but the call was ineffective
because the type of a repeated parameter ident is a TermRef. This led
to a retyping error in Decorators.scala under -Ycheck:front.

We now distinguish between the type of the internal parameter ValDef
and the type of the parameter in the MethodType. The former has the type
`Seq[T] @dotty.annotation.internal.repeated`, the latter has the type
`<repeated>[T]`. The translation with `underlyingWithRepeated` thus
becomes unneccessary.
Literals need to be promoted as is, because constant folding might
have changed the type of the literal to the expected type. E.g. in

    val x: Byte = 2

The literal 2 will have type `Byte(2)` but its underlying constant will still be `Int(2)`.
So re-typing would give the wrong type.
null is not a subtype of a singleton type in Dotty. This slipped by the type checker before.
With the tightened rules in the next commits, it will become illegal.
Satisfiability was too loose before. It is noww tightened. We check that the lower bounds
of all constrained parameters represent a solution to the constraint. To make the check pass
we have to first propagate the constraint by re-verifying all bounds.
This is an opimization to save on unncessessary typed implicits.
1) PolyParams are now approximated with a bounded wildcardtype representing
the bounds in the current constraint, rather than the bounds in the parameter's
declaration. That makes them consistent with the handling of TypeVars.

2) Wildcard types are taken into consideration when result types are constrained.
defaultOptions is now an implicit parameter, which means it can be overridden
on a call-by-call basis.

Added -Ycheck:front to verify that typed trees typecheck again with same types.

The option is disabled for one of the structural tests.
t0786 works again after type inference fixes. Re-enabling.
Instead of replacing all constrained poly params by their lower bounds before checking satsfiability,
we now do this on the fly in the subtype tests.
Test case in TreeTransformers.scala. We have there

    type Mutator[T] = (TreeTransform, T, Context) => TreeTransform

It turns out that then Mutator[X] for some type X did not typecheck because
the typer got confused what were the type parameters of the Function3 type on
Mutator's RHS. The fix adds a case to handle RefinedTypes that bind type
parameters.
Fixes to make the files in `transform` compile in Dotty.
If we intersect a higher-kinded type C with an instance C[T],
we should expect C[T].

Conversely, taking the union of a higher-kinded type C and an
instance C[T] should give C.

Previously, the higher-kinded place-holder $hkN was merged with
&/| with the type T which led to type errors.
1) We now demand that all implicit defs have an implicit type, not
just class members. If we admitted implicit term members without
explicit types, the rules and algorithms for dteremining eligible
implicits would be greatly complicated (because there's always the
danger that inferring the type by typechecking the rhs causes a cyclic
reference).

2) We check for violations of this rule earlier, during type completion, in order
to avoid cyclic references happening before we do the check.
With the previous fixes, we can now compile dotc/transform without errors.
The previous scheme checked all constraint bounds twice everytime
the bounds for a parameter in a constraint were changed. The new scheme,
which can be disabled by unsetting `Config.trackContrDeps`, only
checks those cbounds that directly or indirectly mention the changed
parameter.
The superclass comnstructor of a ClassDef is supposed to be a constructor call. The fix
ensures this is the case when creating classes with tpd.ClassDef.
@DarkDimius
Copy link
Contributor

ping @odersky
Please have a look.

@DarkDimius
Copy link
Contributor

Continuing discussion regarding positions of ModuleDef ->
tpd.ModuleDef(...) withPos pos doesn't work, as ModuleDef returns a Thicket and positions aren't set in underlying trees.
tpd.ModuleDef(...).toList.map(_.withPos(pos)) works, but is alot more verbose

@odersky
Copy link
Contributor Author

odersky commented May 9, 2014

The current PR LGTM.

Regarding withPos in Thickets, we should make that work.

@DarkDimius
Copy link
Contributor

Ok, I'll make a PR with withPos for Thickets.
The other question that arrised for me is about constructing a WhileDo&friends, so that it will desugar into typedTrees. Do we have a convenient method for this?

DarkDimius added a commit that referenced this pull request May 9, 2014
@DarkDimius DarkDimius merged commit 8a4186f into scala:master May 9, 2014
DarkDimius added a commit to DarkDimius/dotty that referenced this pull request May 9, 2014
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.

5 participants