Skip to content

Fix caching of TermRef #5808

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 1 commit into from
Jan 31, 2019
Merged

Conversation

liufengyun
Copy link
Contributor

This commit fixes a bug found in implementing ScalaTest macros, where findMember("===") returns 3
difference single denotations, but alternatives.map(_.termRef) returns 3
types of which the last 2 are the same. This causes an assertion failure in overloading resolution.

Unfortunately we are unable to create a minimized test.

@@ -752,15 +752,15 @@ object Denotations {

/** The TypeRef representing this type denotation at its original location. */
def typeRef(implicit ctx: Context): TypeRef =
TypeRef(symbol.owner.thisType, symbol.name.asTypeName, this)
TypeRef(symbol.owner.thisType, symbol)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@odersky what about TermRef(symbol.owner.thisType, symbol).withDenot(this)?

This commit fixes a bug found in implementing ScalaTest macros, where `findMember("===")` returns 3
difference single denotations, but `alternatives.map(_.termRef)` returns 3
types of which the last 2 are the same. This causes an assertion failure in overloading resolution.

Unfortunately we are unable to create a minimized test.
@liufengyun liufengyun requested a review from odersky January 31, 2019 11:43
@liufengyun liufengyun assigned odersky and unassigned liufengyun Jan 31, 2019
Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

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

This looks like the right fix to me.

@odersky odersky merged commit 6bb20b3 into scala:master Jan 31, 2019
@liufengyun liufengyun deleted the fix-cache-termref branch January 31, 2019 16:44
@smarter
Copy link
Member

smarter commented Feb 2, 2019

This looks like the right fix to me.

I'm not sure so sure, withDenot is going to mutate the current NamedType, since NamedTypes are cached this could affect arbitrary code later on, right ?

@liufengyun
Copy link
Contributor Author

This looks like the right fix to me.

I'm not sure so sure, withDenot is going to mutate the current NamedType, since NamedTypes are cached this could affect arbitrary code later on, right ?

If we assume .termRef and .typeRef are only called on SymDenotation (which is the most common usage), will the code be safe? I cannot see any reason to call .termRef or .typeRef on non-SymDenotation.

liufengyun added a commit to dotty-staging/dotty that referenced this pull request Feb 5, 2019
The fix restricts that we only call `.termRef` and `.typeRef` on
`SymDenotation`.
odersky added a commit that referenced this pull request Feb 6, 2019
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