fix: javalib posix fileKeys are now unique

[LeeTibbert]

Fix: #3905

javalib java.nio.file.attributes.PosixFileAttributeViewImpl fileKeys now follow the JVM description
of a fileKey as uniquely identifying a file.

Windows appears to always return null for dos fileKey, so they are not unique (or useable) on that OS.

[LeeTibbert]

Reviewers,

Could you take a few minutes to look at the implementation of equals() and hashCode() in
the new PosixFileKey class? I think I got both reasonably right but am not sure if equals() is idiomatic.

I was experiencing failures using the equals() method inherited from Object. I know know why: that method
uses .eq, reference equality.

See also Issue #3909. fileKeys always being NULL on Windows is within the spec, but unexpected.

Thank you.

[LeeTibbert]

I am marking this as draft whilst I chase Windows failures.

Files.getAttribute(d0, "fileKey")

Seems to be returning null on Windows.

There is code in javalib/src/main/scala/scala/scalanative/nio/fs/windows/WindowsDosFileAttributeView.scala
which look like it is returning a non-null file key. I will have to study that code more closely by the light of day.

[ekrich]

I was reviewing and realize I really can't help. I found this and I got even less useful 😢 https://users.scala-lang.org/t/help-writing-equals-and-a-compatible-hashcode/5721

[LeeTibbert]

Eric,

Thank you for looking & for your time.

I just re-worked the description. Seems like
./nativelib/src/main/scala/scala/scalanative/runtime/Object.scala
always does a reference check (.eq). On thinking about it, there is not
much else it can do because it has lost (or never had) type info.

 @inline def __equals(that: _Object): scala.Boolean =
    this eq that

The kicker, for me, is that fileKey gets converted to Object by the method
which returns it, so Object.scala#equals is invoked.

I got lost in the maze (and probably still am).

[LeeTibbert]

I believe this PR is now ready for review. Thank you.

1. I fixed and then tracked down and understood why early version were using reference equality
   rather than the required content equality.

2. It appears that Windows always returns null fileKeys. Go figure. This PR no longer runs
   fileKey tests on Windows.

   Now that I know about always null fileKeys on Windows, I will adjust my SystemFileSystemLoopException
   detection design sketches.

[WojciechMazur]

I note about equals method, otherwise looks good to me

[WojciechMazur]

We should check for equality of each of the fields.

Suggested change

	override def equals(that: Any): Boolean = that match {
	case n if that == null => false
	case that: Object => this.hashCode() == that.hashCode()
	case _ => false
	}
	override def equals(that: Any): Boolean = that match {
	case that: PosixFileKey => this.deviceId == that.deviceId && this.inodeNumber == that.inodeNumber
	case _ => false
	}

[LeeTibbert]

Wojciech,

Thank you for the improvements.

1. Doesn't your suggestion have a problem with that being null (NPE)? Now I am not a big fan of
   nulls being being passed around but it it part of the definition of BasicFileAttributes#fileKey that
   it can return null.

2. re: case that: PosixFileKey PosixFileKey is a Scala Native implementation-only definition.
   Java BasicFileAttributes#fileKey returns Object. If IIUC, that will always be an Object
   and never activate the case that: PosixFileKey.

   If the code under discussion is executing, then this must know it is a PosixFileKey extension
   of Object. Is there an idiomatic way to say "case that if classOf[that] == classOf[this]"?

   I know that hasCode() matches are inexact and much prefer the == suggested (to fast longword
   machine comparisons) if I can convince myself and the runtime that that is a PosixFileKey.

Sorry to take time on this apparent molehill.

I hope to use the fileKey for doing File System Loop detection in javalib Files.walk and Files.walkFileTree,
at least on non-Windows. Debugging those is hard enough. It will be neigh on impossible to get right
if I mess up this primitive. I have to work with set-in-stone Java definitions here and would use less difficult
definitions.

[WojciechMazur]

if the that is null it would not be matched in case that: PosixFileKey - isInstanceOf on null is safe it always returns false, so the pattern would never be matched.

that will always be an Object and never activate the case that: PosixFileKey.

Object is a compile-time type, that's the most specific type we get from the signatures. In the equals we check for runtime type and it would typically be PosixFileKey

part of the definition of BasicFileAttributes#fileKey that it can return null.

If that is null it's handled by the wildcard case which returns false. If we get to the point when we need to check if it's a null then this is already non-null - we cannot call method on null, so the result of equals should return false.

Is there an idiomatic way to say "case that if classOf[that] == classOf[this]"?

It could be case that if that.getClass == this.getClass, but in case of PosixFileKey is literally just case PosixFileKey => - there is no other case we're intrested about. The class only needs to check if some other object can equal this object. There is no universal equality here, and a.equals(b) might not always be the same as b.equals(a) depending on implementation of equals in A and B.

Note: there is one more case the equals method should check first - it should start with this eq that to make a quick check for referential equality, before starting the value equality.

[LeeTibbert]

Thank you for the patient and informative explanation.

I will implement your suggestion later today. Having a believable equals() method
helps assure readers of the probably quality of the rest of the code.

[ekrich]

+1

Thank you for the patient and informative explanation.

[LeeTibbert]

and it would typically be PosixFileKey

With 0.99+ probability, PosixFileKey#equals will be called with an argument of type Object, not PosixFileKey.
This is because the BasicFileAttributes.scala: def fileKey(): Object method which is the only way to retrieve
a fileKey returns Object.

The following code fails (Scala 3.4.1, 2.12.19, 2.13.14) its Test in FilesTest. In that test, the argument
is an Object.

   override def equals(that: Any): Boolean =
      that match {
        case that: PosixFileKey =>
          (this.deviceId == that.deviceId) &&
            (this.inodeNumber == that.inodeNumber)

        case _ => false
      }

It appears that case that: PosixFileKey => never executes.

The code above is adapted from that in WindowsPath.scala. Might as well crib from the best.

 override def equals(obj: Any): Boolean =
    obj match {
      case other: WindowsPath =>
        this.fs == other.fs && this.path == other.path
      case _ => false
    }

The following code passes various Scala versions using the same test & Object argument.

   override def equals(that: Any): Boolean =
      that match {
        case that: PosixFileKey =>
          (this.deviceId == that.deviceId) &&
            (this.inodeNumber == that.inodeNumber)

        case that: Object if that.isInstanceOf[PosixFileKey] =>
          val pfk = that.asInstanceOf[PosixFileKey]
          (this.deviceId == pfk.deviceId) &&
            (this.inodeNumber == pfk.inodeNumber)

        case _ => false
      }

The first case seems totally unnecessary.

Suggestions on how to proceed? Thank you.

[ekrich]

Hi Lee, it seems that equals should take AnyRef/Object not Any to match the Java signature. See https://docs.scala-lang.org/scala3/book/first-look-at-types.html

Not sure that is the issue but ...

[LeeTibbert]

Indeed, one would think the that argument would be an Object a.k.a AnyRef . That would make life easier.

I am modeling on scala-native/javalib/src/main/scala/scala/scalanative/nio/fs/windows/WindowsDosFileAttributeView.scala
class DosFileKey(volumeId: DWord, fileIndex: ULargeInteger)

Since that does not override equals, I believe that uses reference equality (which may be a bug).

The instances I will be dealing with will almost never be reference equal, so I need content equality.

When I try `Object` or `AnyRef` as argument type, I get
[error] 91 |  private  final class PosixFileKey(
[error]    |                 ^
[error]    |Name clash between defined and inherited member:
[error]    |def equals(x$0: Any): Boolean in class Any and
[error]    |override def equals(that: AnyRef): Boolean in class PosixFileKey at line 98
[error]    |have the same type after erasure.

That makes me think that the argument type should be Any.

I am following https://alvinalexander.com/scala/how-to-define-equals-hashcode-methods-in-scala-object-equality/
That article summarizes (and makes accessible) two of the foundational books on Scala. One of which was by
Odersky et al.

Step 3 there defines the argument as Any. Based on the info there, I made my class final so that I
do not need to define canEqual(), steps 1 & 2.

The information in that article appears on scala-lang.org. Alvin is one of the authors of that page, so the
use of Any as the argument type is also recommended there.

I may not understand fundamentals or continue to understand them beyond the top of the parabola, but
I can copy pretty well. Can you say "Cargo cult"? Hope I am not mistaken here.

[ekrich]

Wow, I thought for sure it should match Object not Any but that shows how much I understand about Scala.