Skip to content

Only allow sound inline parameters in overrides #8785

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 4 commits into from
Apr 24, 2020
Merged

Only allow sound inline parameters in overrides #8785

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
89672bf
Only allow sound inline parameters in overrides
nicolasstucki Apr 23, 2020
9b78ef9
Update compiler/src/dotty/tools/dotc/typer/Checking.scala
nicolasstucki Apr 24, 2020
b5a968e
Apply suggestions from code review
nicolasstucki Apr 24, 2020
e9c1e7e
Disallow non-inline parametter to override inline parameter
nicolasstucki Apr 24, 2020
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
2 changes: 1 addition & 1 deletion compiler/src/dotty/tools/dotc/config/Printers.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ object Printers {
}

object noPrinter extends Printer {
inline override def println(inline msg: => String): Unit = ()
inline override def println(msg: => String): Unit = ()
}

val default = new Printer
Expand Down
15 changes: 14 additions & 1 deletion compiler/src/dotty/tools/dotc/typer/Checking.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -451,7 +451,8 @@ object Checking {
|| sym.is(TermParam) && !sym.owner.isInlineMethod
))
fail(ParamsNoInline(sym.owner))

if sym.isInlineMethod && !sym.is(Deferred) && sym.allOverriddenSymbols.nonEmpty then
checkInlineOverrideParameters(sym)
if (sym.isOneOf(GivenOrImplicit)) {
if (sym.owner.is(Package))
fail(TopLevelCantBeImplicit(sym))
Expand Down Expand Up @@ -646,6 +647,18 @@ object Checking {
val enumCls = enumCase.owner.linkedClass
if !cls.info.parents.exists(_.typeSymbol == enumCls) then
ctx.error(i"enum case does not extend its enum $enumCls", enumCase.sourcePos)

/** Check the inline override methods only use inline parameteres if they override an inline parameter. */
def checkInlineOverrideParameters(sym: Symbol)(using Context): Unit =
val params = sym.paramSymss.flatten
if params.exists(_.is(Inline)) then
for
sym2 <- sym.allOverriddenSymbols
(p1, p2) <- params.lazyZip(sym2.paramSymss.flatten)
if p1.is(Inline) && !p2.is(Inline)
Copy link
Member

Choose a reason for hiding this comment

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

What about the reverse? Is it ok for a non-inline parameter to override an inline parameter?

Copy link
Member

Choose a reason for hiding this comment

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

It's debatable whether it's OK or desirable. But at least it is sound, because, as @nicolasstucki observed, you can always emulate the semantics of a non-inline param with an inline param that you immediately assign to a val in the body of the method.

However, personally I think we should not allow it either. In Scala, typically overrides must have exactly the same parameters. I think we should also apply that to the quality of being inline. And if someone really needs that capability, well they can always do the emulation themselves.

Copy link
Member

Choose a reason for hiding this comment

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

Well, actually I think they're unsound. If we have a method with two parameters, the first one being either inline+val or by-value, we can end up with different evaluation order for the actual arguments:

def foo(inline x: Int, y: Int): Int = {
  val x2 = x
  x2 * x2 + y
}

foo({ println(1); 3 }, { println(2); 4 })
// prints 2
// prints 1
// returns 13

but

def foo(x: Int, y: Int): Int = {
  x * x + y
}

foo({ println(1); 3 }, { println(2); 4 })
// prints 1
// prints 2
// returns 13

So overriding an inline parameter with a by-value parameter is unsound.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I already added a commit disallowing them.

do
ctx.error("Cannot override non-inline parameter with and inline parameter", p1.sourcePos)

}

trait Checking {
Expand Down
35 changes: 35 additions & 0 deletions tests/neg/inline-parameter-override.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@


abstract class Logger {
def log1(msg: String): Unit
inline def log2(msg: String): Unit
inline def log3(inline msg: String): Unit
}

class Logger1 extends Logger {
inline def log1(msg: String): Unit = ()
inline def log2(msg: String): Unit = ()
inline def log3(msg: String): Unit = ()
}

class Logger2 extends Logger {
inline def log1(inline msg: String): Unit = () // error: Cannot override non-inline parameter with and inline parameter
inline def log2(inline msg: String): Unit = () // error: Cannot override non-inline parameter with and inline parameter
inline def log3(inline msg: String): Unit = ()
}

trait A {
inline def f(inline a: Int): Int
}

trait B {
def f(a: Int): Int
}

class C extends A, B {
inline def f(inline a: Int): Int = 3 // error: Cannot override non-inline parameter with and inline parameter
}

class D extends B, A {
inline def f(inline a: Int): Int = 3 // error: Cannot override non-inline parameter with and inline parameter
}
2 changes: 1 addition & 1 deletion tests/run/inline-override-num.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ trait Num[T] {

object Num {
class IntNum extends Num[Int] {
inline def plus(inline x: Int, inline y: Int): Int = x + y
inline def plus(x: Int, y: Int): Int = x + y
}
given IntNum

Expand Down
4 changes: 2 additions & 2 deletions tests/run/typeclass-derivation2b.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ object TypeLevel {

abstract class GenericSum[S] extends Generic[S] {
def ordinal(x: S): Int
inline def alternative(n: Int): GenericProduct[_ <: S]
inline def alternative(inline n: Int): GenericProduct[_ <: S]
}

abstract class GenericProduct[P] extends Generic[P] {
Expand All @@ -49,7 +49,7 @@ object Lst {
case x: Cons[_] => 0
case Nil => 1
}
inline override def alternative(inline n: Int) <: GenericProduct[_ <: Lst[T]] =
inline def alternative(inline n: Int) <: GenericProduct[_ <: Lst[T]] =
inline n match {
case 0 => Cons.GenericCons[T]
case 1 => Nil.GenericNil
Expand Down