Skip to content
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

Give to @odersky. #2439

Closed
wants to merge 66 commits into from
Closed

Give to @odersky. #2439

wants to merge 66 commits into from

Conversation

DarkDimius
Copy link
Contributor

Ping @odersky.
Reproduction of inference problem:

sbt "run -optimise -Ycheck:tailrec,resolveSuper,mixin,restoreScopes,labelDef,simplify tests/pos/t7126.scala"

Currently it infers a Nothing as a CC[T] to be Nothing[T], and isn't able to figure out that

Nothing[Any] =:= Nothing

We've had a discussion that it should isntead infer CC[T] to be type lambda T => Nothing to be kind-correct.

Applies need to have same receiver :-)
They may have side-effects, unlike reading other arguments.
Disabled it because it makes trees not similar
DarkDimius and others added 21 commits May 10, 2017 14:18
@DarkDimius this is what I remember from our conversation yesterday. Could you
check that I got the valify/devalify/varify right? It's mostly a brain dump
from what you explained, I didn't spent too much time trying to reverse
engineer the code.

I also took the liberty to reformat a few things (such as { (ctx0: Context) =>
{ implicit val ctx: Context = ctx0 <body> } }), I hope these are improvement.
The bug happened for `new Tuple2[Int, Int](1, 2)._1`, if the last
`._1` selection is done by symbol. This will create a
uniqueRefDenotation with underlying type Int.

This denotation has to be erased in conformance with underlying type,
not with the observed type before erasure, as otherwise you'd think
that `_1` on topple returns Int.
This one has no checking and does not assume anything about how labels
work. But if labels are generated wrong, it does not try to fix it or
warn about it.

In case labels are wrong - now you'll get a crash in backend.

--

Manually cherry picked from dotty-linker@11e7808
LabelDefs phase now may move labels into other labels if this is
the right location to emmig the go-to. This may break scoping rules.

Possible solution was to disable Ycheck, but as there are other tricky
phases now in the same phase block, I preferred to teach Ycheck to lift
more restrictions on LabelDefs.
Ycheck already knew something about LabelDefs, as it used to break
ownership links for go-tos, but not it also breaks scoping rules.

Demonstrated by tests/run/t1333.scala

Thanks @OlivierBlanvillain for nice minimization.
The test suite fails on master with -optimise without this change
The situation is a follows:

Without this commit `testOnly dotty.tools.dotc.CompilationTests -- *testPickling` fail with more precise types after pickling.

Reverting tpd.scala to master breaks the "tree referencial equiality" condition used in Simplify to detect a fix point. For example `vulpix t7336.scala` (this one also requires the Skolem#toString change).

This hack is sufficent for Simplify to reach its fix points without breaking pickling...
3 failing tests left:

tests/run/t8933c.scala
tests/pos/t348plus.scala
dotty1 from idempotency1
This test:

sbt "run -optimise -Ycheck:tailrec,resolveSuper,mixin,restoreScopes,labelDef,simplify tests/pos/t7126.scala"

Breaks with the tpd changes alone. By that I mean even when disabling all optimisation (this commit), t7126 still fails.
This also seems to be a reason why Ycheck:front fails.

typeAssigner is REMOVING partial evaluation that typer did
when you copy a tree. It means that types would be different if
you test pickling.

Temporary(permanent) solution: don't print constant types in pickling.
Copy link
Member

@dottybot dottybot left a comment

Choose a reason for hiding this comment

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

Hello, and thank you for opening this PR! 🎉

All contributors have signed the CLA, thank you! ❤️

Commit Messages

We want to keep history, but for that to actually be useful we have
some rules on how to format our commit messages (relevant xkcd).

Please stick to these guidelines for commit messages:

  1. Separate subject from body with a blank line
  2. When fixing an issue, start your commit message with Fix #<ISSUE-NBR>:
  3. Limit the subject line to 72 characters
  4. Capitalize the subject line
  5. Do not end the subject line with a period
  6. Use the imperative mood in the subject line ("Added" instead of "Add")
  7. Wrap the body at 80 characters
  8. Use the body to explain what and why vs. how

adapted from https://chris.beams.io/posts/git-commit

Have an awesome day! ☀️

@odersky
Copy link
Contributor

odersky commented May 16, 2017

@DarkDimius @OlivierBlanvillain I verified that the original problem is fixed with the latest commit.

@odersky
Copy link
Contributor

odersky commented May 16, 2017

I was trying first to avoid a kind-polymorphic Nothing but ran into several other problems. See 9b022ab in branch wip-hk-bottom on staging. So for now this is a quick fix of the problem you were seeing.

@DarkDimius
Copy link
Contributor Author

@odersky, thanks for the fix!

@DarkDimius DarkDimius closed this May 17, 2017
OlivierBlanvillain added a commit to dotty-staging/dotty that referenced this pull request May 23, 2017
This is the issue discussed on scala#2439. The fix implemented there (6f3aa3c) breaks a bunch of other tests; to be further investigated.
OlivierBlanvillain added a commit to dotty-staging/dotty that referenced this pull request May 29, 2017
This is the issue discussed on scala#2439. The fix implemented there (6f3aa3c) breaks a bunch of other tests; to be further investigated.
DarkDimius pushed a commit to dotty-staging/dotty that referenced this pull request May 29, 2017
This is the issue discussed on scala#2439. The fix implemented there (6f3aa3c) breaks a bunch of other tests; to be further investigated.
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