Skip to content

Fix #12344: tune splicing of default getters #12353

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.

Sign up for GitHub

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 3 commits into from
May 21, 2021
Merged

Fix #12344: tune splicing of default getters #12353

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fdbf838
Fix #12344: tune splicing of default getters
liufengyun May 6, 2021
10d75e9
Performance tweak
liufengyun May 6, 2021
c8b823f
Add test
liufengyun May 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion compiler/src/dotty/tools/dotc/typer/Applications.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,7 +261,20 @@ object Applications {
/** Splice new method reference `meth` into existing application `app` */
private def spliceMeth(meth: Tree, app: Tree)(using Context): Tree = app match {
case Apply(fn, args) =>
spliceMeth(meth, fn).appliedToArgs(args)
// Constructors always have one leading non-implicit parameter list.
// Empty list is inserted for constructors where the first parameter list is implicit.
//
// Therefore, we need to ignore the first empty argument list.
// This is needed for the test tests/neg/i12344.scala
//
// see NamerOps.normalizeIfConstructor
//
if args == Nil
&& !fn.isInstanceOf[Apply]
&& app.tpe.isImplicitMethod
&& fn.symbol.isConstructor
then meth
else spliceMeth(meth, fn).appliedToArgs(args)
case TypeApply(fn, targs) =>
// Note: It is important that the type arguments `targs` are passed in new trees
// instead of being spliced in literally. Otherwise, a type argument to a default
Expand Down
26 changes: 26 additions & 0 deletions tests/neg/i12344.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
import scala.quoted.*

class C(using q: Quotes)(i: Int = 1, f: q.reflect.Flags = q.reflect.Flags.EmptyFlags)

def test1a(using q: Quotes) = new C() // error
def test2a(using q: Quotes) = new C(1) // error
def test3a(using q: Quotes) = new C(1, q.reflect.Flags.Lazy) // error
def test4a(using q: Quotes) = new C(f = q.reflect.Flags.Lazy) // error

def test1b(using q: Quotes) = C() // error
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is there an error with this one?

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like it should compile.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As you mentioned in #12344 (comment), for the default parameters to apply, we do need double ():

def test1e(using q: Quotes) = new C()()
def test2e(using q: Quotes) = C()()

Copy link
Contributor

Choose a reason for hiding this comment

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

That goes against the design philosophy of using clauses though doesn't it? My understanding is that if using isn't specified at call-site then it shouldn't map to the using param clause. Having the first () map to the using q clause breaks that rule.

Copy link
Contributor

Choose a reason for hiding this comment

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

@japgolly I think there is no such mapping. It seems the ctor gets an initial empty param list that is not written, see the linked comment. The first explicit params in the call correspond to that one. This is perhaps more natural in Scala 2 where you expect auto-application and only one implicit at the end, so new C makes sense as new C()(implicits). I see it must still be explicit in Scala 3 for case class: "A case class must have at least one non-implicit parameter list".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given a definition like the following:

class C(using Int)
object C

Without prepending the constructor with(), the identifier C would be ambiguous (Scala 3 generailzes synthetic apply to all classes to avoid new).

For the example reported in #12344, the implicit parameter list comes after the synthetic empty parameter list (), and finally follows the default parameter list.

def test2b(using q: Quotes) = C(1) // error
def test3b(using q: Quotes) = C(1, q.reflect.Flags.Lazy) // error
def test4b(using q: Quotes) = C(f = q.reflect.Flags.Lazy) // error

def test1c(using q: Quotes) = new C(using q)()
def test2c(using q: Quotes) = new C(using q)(1)
def test3c(using q: Quotes) = new C(using q)(1, q.reflect.Flags.Lazy)
def test4c(using q: Quotes) = new C(using q)(f = q.reflect.Flags.Lazy)

def test1d(using q: Quotes) = C(using q)()
def test2d(using q: Quotes) = C(using q)(1)
def test3d(using q: Quotes) = C(using q)(1, q.reflect.Flags.Lazy)
def test4d(using q: Quotes) = C(using q)(f = q.reflect.Flags.Lazy)

def test1e(using q: Quotes) = new C()()
def test2e(using q: Quotes) = C()()
Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly, I understand that these work now but that's part of the bug - these 2 lines shouldn't work. There's only one non-using param clause, it shouldn't be possible to specific two non-using clauses at call-site.