Skip to content

Fix #4247: Do not crash if self is of Array type #4251

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

Closed
wants to merge 2 commits into from
Closed

Fix #4247: Do not crash if self is of Array type #4251

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8d9e220
Fix #4247: Do not crash if self is of Array type
nicolasstucki Apr 5, 2018
0e64a0f
Check arrays uptate and length
nicolasstucki Apr 5, 2018
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
10 changes: 7 additions & 3 deletions compiler/src/dotty/tools/dotc/typer/TypeAssigner.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,14 +271,18 @@ trait TypeAssigner {
def assignType(tree: untpd.Select, qual: Tree)(implicit ctx: Context): Select = {
def qualType = qual.tpe.widen
def arrayElemType = {
val JavaArrayType(elemtp) = qualType
elemtp
qualType match {
case JavaArrayType(elemtp) => elemtp
case _ =>
ctx.error("Array type conflict with " + qual.symbol.name, tree.pos)
defn.NothingType
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Scalac allows both the following:

class Foo[U] { self : Array[U] => self(0) }

class Foo[U] { self : Array[U] with Nothing =>
  self.length
}

We need to justify why Dotty should deviate in such cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We will never be able to instantiate a Foo because Array is final. The code is nonsensical as no subclass of Foo can mixin Array.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm thinking it's the same case as def f(x: Nothing): Unit = 3. The code is valid, though the function can not be called. If we have checks for realization of self types during instantiation, can we just reuse that mechanism?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we could. But this patch should still be there to ensure there are no compiler crashes on arrays in case that checker fails to detect it early.

Also, It is possible that we may create some other kind of unrealisable array with something that is not the self type.

Copy link
Contributor

Choose a reason for hiding this comment

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

I tend to agree with @liufengyun. We should fix https://github.com/lampepfl/dotty/blob/4c8a2bd319437f7ab22303a162f5922bc000134d/compiler/src/dotty/tools/dotc/typer/Checking.scala#L148
Otherwise we will reject class Foo[U] { self : Array[U] & Nothing => self(0) } but accept class Foo[U] { self : Array[U] & Nothing => }.

Also, It is possible that we may create some other kind of unrealisable array with something that is not the self type.

Wouldn't that be a bug? Then crashing is the right think to do

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was a different issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The & Nothing is unrelated tho this particular issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

That was a different issue

Sorry, my point was that we should also reject test[U](x: Array[U] & Nothing) = x(0), if we reject self: Array[U] & Nothing => self(0)

Copy link
Contributor

Choose a reason for hiding this comment

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

If we have checks for realization of self types during instantiation, can we just reuse that mechanism?

BTW, those checks on instantiation are spec-mandated (#4252), so I think I agree with @allanrenucci and @liufengyun.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The check on instantiation are done and work as expected. The issue is that for this particular case we do have a problem before instantiation. This is a particular exception due to the way we erase Arrays into java arrays.

Additionally the self type Array is completely valid as far as this class is conserened and could be realizable. Though Array would be the only class that could extend it.

}
val p = nme.primitive
val tp = tree.name match {
case p.arrayApply => MethodType(defn.IntType :: Nil, arrayElemType)
case p.arrayUpdate => MethodType(defn.IntType :: arrayElemType :: Nil, defn.UnitType)
case p.arrayLength => MethodType(Nil, defn.IntType)
case p.arrayLength => arrayElemType; MethodType(Nil, defn.IntType)

// Note that we do not need to handle calls to Array[T]#clone() specially:
// The JLS section 10.7 says "The return type of the clone method of an array type
Expand Down
3 changes: 3 additions & 0 deletions tests/neg/i4247.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
class Foo[U] { self : Array[U] & Nothing =>
val s = self(0) // error: Array type conflict with Foo
}
3 changes: 3 additions & 0 deletions tests/neg/i4247b.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
class Foo[U] { self : Array[U] & Nothing =>
self(0) = ??? // error: Array type conflict with Foo
}
3 changes: 3 additions & 0 deletions tests/neg/i4247c.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
class Foo[U] { self : Array[U] & Nothing =>
self.length // error: Array type conflict with Foo
}