-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix #2631: Add custom error message #3802
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
Conversation
|
@biboudis do not forget this one |
| } | ||
| else if (name.toTermName == nme.ERROR) | ||
| UnspecifiedErrorType | ||
| else if (ctx.owner.isConstructor && ctx.owner.owner.unforcedDecls.toList.exists(_.name == tree.name)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would factor out the second test into a def. It is not clear what it does. Maybe:
def isNotAccesibleFromConstructor(name: Name) = {
val classMembers = ctx.owner.owner.unforcedDecls
classMembers.lookup.(name).exists
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the test is factored out it is not sufficient to ensure that it is not accessible from the constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will use the .lookup.(name).exists though.
7c486df to
d83d09d
Compare
| else if (ctx.owner.isConstructor && ctx.owner.owner.unforcedDecls.lookup(tree.name).exists) { | ||
| // If the field existed but was not found we assume that | ||
| // we where in the context of an argument of the super constructor | ||
| errorType(ex"$tree is not accessible from super constructor arguments", tree.pos) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@smarter do you think this condition is enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't you check ctx.mode.is(InSuperCall) ? (The name is misleading, it's set for both this- and super-call, see the method superOrThisCallContext)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, don't say "super constructor", a this call is not a super call, it's just a constructor call
| else if (name.toTermName == nme.ERROR) | ||
| UnspecifiedErrorType | ||
| else | ||
| else if (ctx.owner.isConstructor && ctx.mode.is(Mode.InSuperCall) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't ctx.mode.is(Mode.InSuperCall) implies ctx.owner.isConstructor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be a method super call with super.xys.
No description provided.