Add in 061_TopLevelCantBeImplicit #9
Conversation
|
Thanks so much for the contribution @tanishiking! You're the first one to contribute, so if you have any feedback about the
This is sort of what I expected, thanks for confirming!
So the id should actually never be removed in Dotty (it actually can't since it will screw up all the numbering). Just for documentation purposes we should keep the IDs, but the |
| import scala.language.implicitConversions | ||
| case class C(str: String) | ||
| implicit def toC(x: String): C = C(x) | ||
| implicit val defaultC: C = C("default") |
There was a problem hiding this comment.
I think we might be even able to just put the most simple example possible to prove this. I'm assuming that just a top level
implicit val foo: String = "foo"should also provide the same example right? If so, let's just keep it as simple as possible.
Overall, everything is very clear, and I could understand the workflow, great job 👍 !
Gotcha, that makes sense :)
That's a good idea, now Anyway, I'll tackle the review comments, and rebase on main :) |
Yup, exactly. Either is fine. |
tanishiking
force-pushed
the
061-toplevelcantbe-implicit
branch
from
8151e96 to
1b147cd
Compare
Adding `// LEGACY` to 061. It seems like TopLevelCantBeImplicit is no longer the case thanks to scala/scala3#5754 This is actually confirmed in https://github.com/lampepfl/dotty/blob/93fc41fcb624df73cc12d52b79d518a30a778a7c/tests/run/toplevel-implicits/a.b.scala#L19-L21
tanishiking
force-pushed
the
061-toplevelcantbe-implicit
branch
from
1b147cd to
7dcfe78
Compare
| import scala.language.implicitConversions | ||
| implicit def intToBoolean(x: Int): Boolean = x != 0 |
There was a problem hiding this comment.
Sorry if I wasn't clear in my message. I mean everything except the
implicit val foo: String = "foo"
can be removed so then this would just be
@main def TopLevelCantBeImplicitID = ()
implicit val foo: String = "foo"Since that's the most minimal to illustrate the top level implicit right?
There was a problem hiding this comment.
Ah, right! I'll remove the implicit def part 👍
There was a problem hiding this comment.
Even the import isn't needed though right? Since it's not doing anything?
There was a problem hiding this comment.
Oh, yeah, it's now not needed!
tanishiking
force-pushed
the
061-toplevelcantbe-implicit
branch
2 times, most recently
from
94fe263 to
e16a95d
Compare
tanishiking
force-pushed
the
061-toplevelcantbe-implicit
branch
from
e16a95d to
49c6749
Compare
There was a problem hiding this comment.
Awesome, thanks @tanishiking! If all work after this is merged the index should be updated as well. So we'll see!
|
Looks like the CI works fine and the index is now updated 424de7d 🎉 cool! |
Just to tie these together I ended up doing this in scala/scala3#14965 |
Adding
// LEGACYto 061.It seems like TopLevelCantBeImplicit is no longer the case thanks to scala/scala3#5754
This is actually confirmed in https://github.com/lampepfl/dotty/blob/93fc41fcb624df73cc12d52b79d518a30a778a7c/tests/run/toplevel-implicits/a.b.scala#L19-L21
I'm planning to refactor the dotty repo to delete the
TopLevelCantBeImpliciterror message-id, and the check logic (as it's a dead code now). In that case, should we keepTopLevelCantBeImplicitindotty-error-indexrepo?Thank you very much for kicking off https://contributors.scala-lang.org/t/revisiting-dotty-diagnostics-for-tooling/5649/15 @ckipp01 ! This is definitely a good way to improve the devtools experience, the user experience of Scala (especially for newcomers).