Fix #1784: allow to omit types for local implicit vals #1785
Conversation
|
To exercise this in bootstrap, you could remove all the |
Drop explicit types for local implicit vals of type Context and Position. Exercises the functionality and shortens the code.
@sjrd Done. I saw there were quite a few of those. |
| /** Map this function over given type */ | ||
| def mapOver(tp: Type): Type = { | ||
| implicit val ctx: Context = this.ctx // Dotty deviation: implicits need explicit type | ||
| implicit val ctx = this.ctx // Dotty deviation: implicits need explicit type |
There was a problem hiding this comment.
Looks like the comment is obsolete :P
| } | ||
| else { | ||
| implicit val ctx: Context = (new ContextBase).initialCtx // Dotty deviation: implicits need explicit type | ||
| implicit val ctx = (new ContextBase).initialCtx // Dotty deviation: implicits need explicit type |
Since we now allow to drop the explicit type of a local implicit val it can happen that this causes a cyclic reference, namely when the typechecking of the right-hand side involves an implicit search. It's unpractical and fragile to avoid this. Instead we give now a nice error message explaining the problem and how to fix it in source code.
Needed an // error annotation
| errorMsg(ex.show, ctx) | ||
|
|
||
| if (cycleSym.is(Implicit, butNot = Method) && cycleSym.owner.isTerm) | ||
| em"""cyclic reference involving implicit $cycleSym |
There was a problem hiding this comment.
@felixmulder Line 2 and 3 of this message might be moved to an explanation. But we can also leave it like this, I think.
|
@sjrd @odersky: note that the The sources for the Scala.JS IR were added as a source dependency (by sbt) and compiled by sbt with dotty compiler. The resulting class files were previously erroneously put on the classpath during bootstrap. Therefore the Scala.JS backend is no longer compiled with the tests. Ask me today and I'll give a more thorough explanation in person. |
|
LGTM. Added a proper error message as per @odersky's recommendation. |
|
@felixmulder I decided to revert the last commit. I think moving the explanation away into a separate step is counter-productive here because the error message alone is too opaque to tell you anything. So I think in this case it's actually better to give the explanation as part of the error message. |
|
@odersky - that's fine by me 👍 |
|
Ping @olafurpg since this affect scalafix |
|
Thanks for the ping @smarter. I will update the ExplicitImplicit rewrite to allow optional type annotations for local implicit vals. To be sure I understand correctly, a val is considered local if and only if it's not defined inside a template? class/trait/object A {
// not OK
implicit val x =???
def/val/var foo = {
// OK
implicit val x = ???
}
} |
|
On Thu, Dec 15, 2016 at 4:48 PM, Ólafur Páll Geirsson < ***@***.***> wrote:
Thanks for the ping @smarter <https://github.com/smarter>. I will update
the ExplicitImplicit rewrite to allow optional type annotations for local
implicit vals. To be sure I understand correctly, a val is considered local
if and only if it's not defined inside a template?
class/trait/object A {
// not OK
implicit val x =???
def/val/var foo = {
// OK
implicit val x = ???
}
}
Yes, that's right. Some local values still need to be given types, namely
if their right-hand side requires an implicit search. But that can be
handled locally by dotty, I think.
—
… You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1785 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAwlVrROPlGPLcacEzbrCoTKR-NSFRoKks5rIWFhgaJpZM4LKTSv>
.
{"api_version":"1.0","publisher":{"api_key":"
05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":
{"external_key":"github/lampepfl/dotty","title":"
lampepfl/dotty","subtitle":"GitHub repository","main_image_url":"
https://cloud.githubusercontent.com/assets/143418/17495839/a5054eac-5d88-
11e6-95fc-7290892c7bb5.png","avatar_image_url":"https://
cloud.githubusercontent.com/assets/143418/15842166/
7c72db34-2c0b-11e6-9aed-b52498112777.png","action":{"name":"Open in
GitHub","url":"https://github.com/lampepfl/dotty"}},"
***@***.*** in #1785:
Thanks for the ping @smarter. I will update the ExplicitImplicit rewrite to
allow optional type annotations for local implicit vals. To be sure I
understand correctly, a val is considered local if and only if it's not
defined inside a template?\r\n\r\n```scala\r\nclass/trait/object A {\r\n
// not OK\r\n implicit val x =???\r\n def/val/var foo = {\r\n // OK\r\n
implicit val x = ???\r\n }\r\n}\r\n```"}],"action":{"name":"View Pull
Request","url":"#1785 (comment)
267361468"}}}
|
6 participants
Review by @felixmulder ?