Skip to content

Fix #9751: Fix Inline purity checks #9753

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 1 commit into from
Sep 10, 2020
Merged

Fix #9751: Fix Inline purity checks #9753

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 3 additions & 1 deletion compiler/src/dotty/tools/dotc/ast/TreeInfo.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -454,12 +454,14 @@ trait TypedTreeInfo extends TreeInfo[Type] { self: Trees.Instance[Type] =>
def refPurity(tree: Tree)(using Context): PurityLevel = {
val sym = tree.symbol
if (!tree.hasType) Impure
else if (!tree.tpe.widen.isParameterless || sym.isEffectivelyErased) PurePath
else if !tree.tpe.widen.isParameterless then PurePath
else if sym.is(Erased) then PurePath
else if tree.tpe.isInstanceOf[ConstantType] then PurePath
else if (!sym.isStableMember) Impure
else if (sym.is(Module))
if (sym.moduleClass.isNoInitsClass) PurePath else IdempotentPath
else if (sym.is(Lazy)) IdempotentPath
else if sym.isAllOf(Inline | Param) then Impure
else PurePath
}

Expand Down
2 changes: 1 addition & 1 deletion compiler/src/dotty/tools/dotc/transform/TransformByNameApply.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ abstract class TransformByNameApply extends MiniPhase { thisPhase: DenotTransfor
ref(defn.cbnArg).appliedToType(argType).appliedTo(arg).withSpan(arg.span)
arg match {
case Apply(Select(qual, nme.apply), Nil)
if qual.tpe.derivesFrom(defn.FunctionClass(0)) && isPureExpr(qual) =>
if qual.tpe.derivesFrom(defn.FunctionClass(0)) && (isPureExpr(qual) || qual.symbol.isAllOf(Inline | Param)) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it matter? If I were correct, the body of inlined methods is never executed, thus only the TASTY matters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In theory that seems to be the case. But I caught one that came for the DottyPredef.assert when compiling dotty itself. It might be a re-bootstrapping issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. It could be some invariants after the typer -- and it's always good to do so.

wrap(qual)
case _ =>
if (isByNameRef(arg) || arg.symbol == defn.cbnArg) arg
Expand Down
2 changes: 1 addition & 1 deletion compiler/src/dotty/tools/dotc/typer/Inliner.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -521,7 +521,7 @@ class Inliner(call: tpd.Tree, rhsToInline: tpd.Tree)(using Context) {
| Literal(_) =>
true
case Ident(_) =>
isPureRef(tree)
isPureRef(tree) || tree.symbol.isAllOf(Inline | Param)
case Select(qual, _) =>
if (tree.symbol.is(Erased)) true
else isPureRef(tree) && apply(qual)
Expand Down
8 changes: 6 additions & 2 deletions compiler/src/dotty/tools/dotc/typer/Typer.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3629,8 +3629,12 @@ class Typer extends Namer
}

private def checkStatementPurity(tree: tpd.Tree)(original: untpd.Tree, exprOwner: Symbol)(using Context): Unit =
if (!tree.tpe.isErroneous && !ctx.isAfterTyper && isPureExpr(tree) &&
!tree.tpe.isRef(defn.UnitClass) && !isSelfOrSuperConstrCall(tree))
if !tree.tpe.isErroneous
&& !ctx.isAfterTyper
&& !tree.isInstanceOf[Inlined]
&& isPureExpr(tree)
&& !isSelfOrSuperConstrCall(tree)
Copy link
Contributor

Choose a reason for hiding this comment

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

tree.isInstanceOf[Inlined] : can an inlined tree be pure? What about handle inlined trees in isPureExpr?

Copy link
Contributor Author

@nicolasstucki nicolasstucki Sep 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

The Inlined trees can be pure which is correctly identified in isPureExpr. Here we special case a
Inlined calls that inserted a pure expression in a statement position as these are only pure due to other constraints that may change. For example, the assert can take a reference to an Inlined true value which converts it in a no-op and we do not want to warn that this is a pure expression in statement position.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation. It might be good to add a short comment after the line.

then
report.warning(PureExpressionInStatementPosition(original, exprOwner), original.srcPos)

/** Types the body Scala 2 macro declaration `def f = macro <body>` */
Expand Down
9 changes: 9 additions & 0 deletions tests/neg-custom-args/fatal-warnings/i9751.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
def f(): Unit = {
() // error
()
}

inline def g(): Unit = {
() // error
()
}
11 changes: 11 additions & 0 deletions tests/pos-special/fatal-warnings/i9751.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
object Test {
extension (x: Int)
inline def times(inline op: Unit): Unit = {
var count = 0
while count < x do
op
count += 1
}

10.times { println("hello") }
}
13 changes: 13 additions & 0 deletions tests/pos-special/fatal-warnings/i9751b.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
object Test {
inline def f(inline x: Boolean): Unit =
inline if x then println()

f(true)
f(false)

inline def g(inline x: => Boolean): Unit =
inline if x then println()

g(true)
g(false)
}