-
Notifications
You must be signed in to change notification settings - Fork 361
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.
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
fix: javalib posix fileKeys are now unique #3906
fix: javalib posix fileKeys are now unique #3906
Conversation
Reviewers, Could you take a few minutes to look at the implementation of I was experiencing failures using the See also Issue #3909. Thank you. |
I am marking this as draft whilst I chase Windows failures.
Seems to be returning null on Windows. There is code in |
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 |
Eric, Thank you for looking & for your time. I just re-worked the description. Seems like
The kicker, for me, is that I got lost in the maze (and probably still am). |
I believe this PR is now ready for review. Thank you.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I note about equals method, otherwise looks good to me
override def equals(that: Any): Boolean = that match { | ||
case n if that == null => false | ||
case that: Object => this.hashCode() == that.hashCode() | ||
case _ => false | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should check for equality of each of the fields.
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 | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wojciech,
Thank you for the improvements.
-
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 ofBasicFileAttributes#fileKey
that
it can return null. -
re:
case that: PosixFileKey
PosixFileKey is a Scala Native implementation-only definition.
JavaBasicFileAttributes#fileKey
returnsObject
. If IIUC,that
will always be anObject
and never activate thecase that: PosixFileKey
.If the code under discussion is executing, then
this
must know it is aPosixFileKey
extension
ofObject
. 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 thatthat
is aPosixFileKey
.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
Thank you for the patient and informative explanation.
With 0.99+ probability, The following code fails (Scala 3.4.1, 2.12.19, 2.13.14) its Test in
It appears that The code above is adapted from that in
The following code passes various Scala versions using the same test &
The first case seems totally unnecessary. Suggestions on how to proceed? Thank you. |
Hi Lee, it seems that Not sure that is the issue but ... |
Indeed, one would think the I am modeling on Since that does not override The instances I will be dealing with will almost never be reference equal, so I need content equality.
That makes me think that the argument type should be I am following Step 3 there defines the argument as The information in that article appears on I may not understand fundamentals or continue to understand them beyond the top of the parabola, but |
Wow, I thought for sure it should match Object not Any but that shows how much I understand about Scala. |
Fix: #3905
javalib
java.nio.file.attributes.PosixFileAttributeViewImpl
fileKey
s now follow the JVM descriptionof 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.