Skip to content

fix #22833: allow unroll annotation in methods of final class #22926

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
Apr 14, 2025
+18 −12
Merged

fix #22833: allow unroll annotation in methods of final class #22926

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
8 changes: 4 additions & 4 deletions compiler/src/dotty/tools/dotc/reporting/messages.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3430,12 +3430,12 @@ extends DeclarationMsg(IllegalUnrollPlacementID):
val isCtor = method.isConstructor
def what = if isCtor then i"a ${if method.owner.is(Trait) then "trait" else "class"} constructor" else i"method ${method.name}"
val prefix = s"Cannot unroll parameters of $what"
if method.is(Deferred) then
i"$prefix: it must not be abstract"
if method.isLocal then
i"$prefix because it is a local method"
else if !method.isEffectivelyFinal then
i"$prefix because it can be overridden"
else if isCtor && method.owner.is(Trait) then
i"implementation restriction: $prefix"
else if !(isCtor || method.is(Final) || method.owner.is(ModuleClass)) then
i"$prefix: it is not final"
else if method.owner.companionClass.is(CaseClass) then
i"$prefix of a case class companion object: please annotate the class constructor instead"
else
Expand Down
4 changes: 2 additions & 2 deletions compiler/src/dotty/tools/dotc/transform/PostTyper.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,9 +132,9 @@ class PostTyper extends MacroTransform with InfoTransformer { thisPhase =>
then
false // not an error, but not an expandable unrolled method
else if
method.is(Deferred)
method.isLocal
|| !method.isEffectivelyFinal
Comment on lines +135 to +136
Copy link
Member

Choose a reason for hiding this comment

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

@lihaoyi confirmed that abstract methods shouldn't be allowed to have @unroll.

Suggested change
method.isLocal
|| !method.isEffectivelyFinal
method.is(Deferred)
|| method.isLocal
|| !method.isEffectivelyFinal

Copy link
Member

Choose a reason for hiding this comment

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

I'm surprised that no tests were affected by this change!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

isEffectivelyFinal already checks that the method is not Deferred, at least when the owner is not extensible.

I think the only place where the extra check for Deferred would be needed is for a method that is both abstract and final. In that case, the desired semantics is not clear: should @unroll be allowed because the method is final, or should @unroll be prohibited because the method is also abstract?

final abstract is rare but it does happen.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've never seen final abstract, I didn't know that was even allowed?

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 believe the only place it's used is for the primitive classes in the standard library like scala.Int.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah ... that's really just primitives. A final abstract class cannot possibly be instantiated nor subclassed. So who cares what happens inside. It's basically an uninhabited type. ;)

Copy link
Member

@hamzaremmal hamzaremmal Apr 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I'm surprised that no tests were affected by this change!

@olhotak You're right, I was context switching when I made the comment. I've rechecked the diff and everything seems correct to me.

|| isCtor && method.owner.is(Trait)
|| !(isCtor || method.is(Final) || method.owner.is(ModuleClass))
|| method.owner.companionClass.is(CaseClass)
&& (method.name == nme.apply || method.name == nme.fromProduct)
|| method.owner.is(CaseClass) && method.name == nme.copy
Expand Down
3 changes: 2 additions & 1 deletion compiler/src/dotty/tools/dotc/transform/UnrollDefinitions.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,8 @@ class UnrollDefinitions extends MacroTransform, IdentityDenotTransformer {
else Some((paramClauseIndex, annotationIndices))
if indices.nonEmpty then
// pre-validation should have occurred in posttyper
assert(annotated.is(Final, butNot = Deferred) || annotated.isConstructor || annotated.owner.is(ModuleClass) || annotated.name.is(DefaultGetterName),
assert(!annotated.isLocal, i"$annotated is local")
assert(annotated.isEffectivelyFinal || annotated.name.is(DefaultGetterName),
i"$annotated is not final&concrete, or a constructor")
indices
})
Expand Down
4 changes: 2 additions & 2 deletions tests/neg/unroll-abstractMethod.check
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
-- [E207] Declaration Error: tests/neg/unroll-abstractMethod.scala:6:41 ------------------------------------------------
6 | def foo(s: String, n: Int = 1, @unroll b: Boolean = true): String // error
| ^
| Cannot unroll parameters of method foo: it must not be abstract
| Cannot unroll parameters of method foo because it can be overridden
-- [E207] Declaration Error: tests/neg/unroll-abstractMethod.scala:10:41 -----------------------------------------------
10 | def foo(s: String, n: Int = 1, @unroll b: Boolean = true): String // error
| ^
| Cannot unroll parameters of method foo: it must not be abstract
| Cannot unroll parameters of method foo because it can be overridden
6 changes: 3 additions & 3 deletions tests/neg/unroll-illegal3.check
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
-- [E207] Declaration Error: tests/neg/unroll-illegal3.scala:7:31 ------------------------------------------------------
7 | def foo(s: String, @unroll y: Boolean) = s + y // error
| ^
| Cannot unroll parameters of method foo: it is not final
| Cannot unroll parameters of method foo because it is a local method
-- [E207] Declaration Error: tests/neg/unroll-illegal3.scala:12:29 -----------------------------------------------------
12 | def foo(s: String, @unroll y: Boolean) = s + y // error
| ^
| Cannot unroll parameters of method foo: it is not final
| Cannot unroll parameters of method foo because it can be overridden
-- [E207] Declaration Error: tests/neg/unroll-illegal3.scala:16:29 -----------------------------------------------------
16 | def foo(s: String, @unroll y: Boolean): String // error
| ^
| Cannot unroll parameters of method foo: it must not be abstract
| Cannot unroll parameters of method foo because it can be overridden
5 changes: 5 additions & 0 deletions tests/pos/i22833-unroll-final-class.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import scala.annotation.{experimental,unroll}

@experimental final class Foo {
def bar(@unroll x: Int = 0) = x + 1
}
Loading