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

Fix #4060 by marking ghost symbols as unstable #4071

Merged
merged 2 commits into from
Mar 9, 2018

Conversation

Blaisorblade
Copy link
Contributor

Closes #4060.

@@ -600,7 +600,7 @@ object SymDenotations {

/** Is this a denotation of a stable term (or an arbitrary type)? */
final def isStable(implicit ctx: Context) =
isType || is(Stable) || !(is(UnstableValue) || info.isInstanceOf[ExprType])
!is(Ghost) && (isType || is(Stable) || !(is(UnstableValue) || info.isInstanceOf[ExprType]))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Testing is(Ghost) first is probably overkill, I'll try isType || !is(Ghost) && (is(Stable) || !(is(UnstableValue) || info.isInstanceOf[ExprType])) after the build finishes.

@Blaisorblade
Copy link
Contributor Author

test performance please

@dottybot
Copy link
Member

dottybot commented Mar 4, 2018

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

@dottybot
Copy link
Member

dottybot commented Mar 5, 2018

Performance test finished successfully:

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

Benchmarks is based on merging with master (4cfffbe)

fun3(new Bar)

def fun1[F >: Bar <: Foo](ghost f: F): f.X = null.asInstanceOf[f.X]
def fun2[F >: Bar <: Foo](ghost f: F)(ghost bar: f.B): f.B = null.asInstanceOf[f.B]
Copy link
Contributor

Choose a reason for hiding this comment

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

Make them neg tests. It is always good to make sure we do not have a regression.

@@ -600,7 +600,7 @@ object SymDenotations {

/** Is this a denotation of a stable term (or an arbitrary type)? */
final def isStable(implicit ctx: Context) =
isType || is(Stable) || !(is(UnstableValue) || info.isInstanceOf[ExprType])
isType || !is(Ghost) && (is(Stable) || !(is(UnstableValue) || info.isInstanceOf[ExprType]))
Copy link
Contributor

Choose a reason for hiding this comment

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

We should avoid marking the symbols as stable if they are ghost. We could assert that a symbol must at most one of those flags.

Copy link
Contributor

Choose a reason for hiding this comment

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

Because in some places we might ask if sym.is(Stable) which would say true for a symbol with Stable and Ghost flags.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually don't do this change

@nicolasstucki nicolasstucki dismissed their stale review March 5, 2018 11:19

Dismissed requested change

Copy link
Contributor

@nicolasstucki nicolasstucki left a comment

Choose a reason for hiding this comment

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

Move old tests to neg tests

@Blaisorblade
Copy link
Contributor Author

@nicolasstucki Good for you?

@nicolasstucki nicolasstucki merged commit 09bcded into scala:master Mar 9, 2018
@Blaisorblade Blaisorblade deleted the fix-#4060 branch March 9, 2018 22:05
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