Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fix/overloaded access #46

Closed
wants to merge 6 commits into
from

Conversation

Projects
None yet
3 participants
Contributor

odersky commented Mar 6, 2014

Review by @DarkDimius @samuelgruetter

I apologize for the many commits. I have too much on my plate right now to spend time squashing them.

odersky added some commits Mar 5, 2014

Fix problem in TermRef.alternatives
Rewrap needs to produce alternatives with signatures. Otgerwise the new denotation will simply overwrite the old because both the overloaded TermRef and the alternative will hash to the same unique TermRef.
Fix problem comparing overloaded TermRefs
Overloaded TermRefs do not have an info, and consequently do not support =:=. Yet in Typer#checkNewOrShadowed we compared termrefs with =:=. This gives an exception if the termrefs are overloaded. The fix is to provide a new method isSameRef in TypeComparer which is called instead of =:= in Typer#checkNewOrShadowed.

@xeno-by xeno-by commented on the diff Mar 6, 2014

src/dotty/tools/dotc/typer/Typer.scala
@@ -139,8 +138,7 @@ class Typer extends Namer with Applications with Implicits {
}
val where = if (ctx.owner.exists) s" from ${ctx.owner.enclosingClass}" else ""
val whyNot = new StringBuffer
- val addendum =
- alts foreach (_.isAccessibleFrom(pre, superAccess, whyNot))
+ alts foreach (_.isAccessibleFrom(pre, superAccess, whyNot))
@xeno-by

xeno-by Mar 6, 2014

Owner

So this is executed just for side-effects?

Contributor

odersky commented Mar 6, 2014

Yes, it's just for side effects. The allocation of a ListBuffer in the previous line hints to that.

Contributor

odersky commented Mar 6, 2014

I repushed the branch after fixing merge errors.

@samuelgruetter samuelgruetter commented on the diff Mar 6, 2014

src/dotty/tools/dotc/typer/Typer.scala
@@ -245,7 +243,7 @@ class Typer extends Namer with Applications with Implicits {
* does properly shadow the new one from an outer context.
*/
@samuelgruetter

samuelgruetter Mar 6, 2014

Member

I have trouble understanding how checkNewOrShadowed works. It seems there are two types: a "previously found result from an inner context", and a "new one from an outer context". One is called found, and the other is called previous, I guess. But which one is which? Both ways do not (yet ;-)) make sense for me...

@odersky

odersky Mar 6, 2014

Contributor

found is new one.

Contributor

odersky commented Mar 6, 2014

Closing pull request and integrating in a new one.

@odersky odersky closed this Mar 6, 2014

Maybe this function could use a different name that would highlight it being side-effecting, e.g. checkAccessibleFrom?

I think isAccessibleFrom does not have enough side-effects to be renamed to checkAccessibleFrom ;-)
checkXxx methods call ctx.error on failure, but isAccessibleFrom doesn't, and I think it's good to have the is/check prefix as an indicator whether it calls ctx.error or not.

I guess Martin's naming policy might be something like this:

  • def checkXxx(foo: T): T does a check on foo and if success, it returns foo, maybe slightly modified, and if failure, it calls ctx.error and returns foo or something similar, such that we can continue typechecking despite the error
  • def isXxx(foo: T): Boolean has (almost) no side effects, and returns a boolean
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment