Skip to content
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

New (?) Scala Native 3.2.1 warnings #2952

Closed
LeeTibbert opened this issue Oct 28, 2022 · 7 comments · Fixed by #2966
Closed

New (?) Scala Native 3.2.1 warnings #2952

LeeTibbert opened this issue Oct 28, 2022 · 7 comments · Fixed by #2966

Comments

@LeeTibbert
Copy link
Contributor

LeeTibbert commented Oct 28, 2022

Scala 3.2.1, SN 0.5.0-SNAPSHOT approx 2022-10-28T02:00

I have been trying to cut some of the build warnings out of the SN 3.2.0
(now 3.2.1) build so that I could see any build defects I might be
introducing.

When I reduced the shrubbery, this popped up. Does not look like mine
and it looks new.

javalib/src/main/scala/java/util/Hashtable.scala:102:13 
[warn] 102 |        case o: UnboxedEntry => boxedEntry.equals(o.boxedEntry)
[warn]     |             ^
[warn]     |           the type test for UnboxedEntry cannot be checked at runtime

Question before the house: Is the test broken and should be fixed or removed or did something in the compiler change?

@LeeTibbert
Copy link
Contributor Author

Removed more shrubbery and found:

unit-tests/shared/src/test/scala/javalib/util/ArraysTest.scala:995:13 
[warn] 995 |        case that: A => this.x == that.x
[warn]     |             ^
[warn]     |             the type test for A cannot be checked at runtime

@LeeTibbert LeeTibbert changed the title New (?) Scala Native 3.2.1 warning New (?) Scala Native 3.2.1 warnings Oct 28, 2022
@WojciechMazur
Copy link
Contributor

Probably it's a regression in the compiler. It's only present for classes defined in local scope like methods. I'll try to ask the compiler team is it actual regression or insight from the compiler about actual problem. However, I think that's just a false negative warning.

@WojciechMazur
Copy link
Contributor

Created an issue in dotty scala/scala3#16259

@WojciechMazur
Copy link
Contributor

Seems it's an actual issue - the local class leads to unsoundness - it's a situation when even though something might compile and have correct types it might not be actually checked in the runtime. I'm not sure if it only applies to JVM, but probably we should move the locally defined classes to the outer scope.

@LeeTibbert
Copy link
Contributor Author

Thank you for digging into this. I will look at those two files later today or tomorrow.

Not my or our biggest problems, but it is nice to work with clean decks.

@LeeTibbert
Copy link
Contributor Author

LeeTibbert commented Oct 28, 2022

Wojciech,

I do not want to place you in the middle.

@Test def equals_AnyRefs(): Unit = {
    // scalastyle:off equals.hash.code                                          
    class A(private val x: Int) {
      override def equals(that: Any): Boolean = that match {
        case that: A => this.x == that.x
	case _       => false
      }
    }
    // scalastyle:on equals.hash.code      

I am only a provincial, but this looks like, smell like, and
quacks like a compiler bug.

I agree that True unsoundness is to be avoided, as it is
equals True Sin. Was there any explanation of the unsoundness
here?

The code which triggers this new(?) warning looks like the above.
With only the name changed to equals_AnyRefs, this is code from
and still active in Scala.js. Almost always, if there is something
wonky in Scala code, Scala.js will not accept the code into
their repository in the first place.

The other site in SN has the same equals idiom.

Sorry to be slow, but why can't the type check happen at runtime
if equals is ever called?

I use local classes all over the place, usually to limit scope.
This issue shakes my belief in those classes as being reliable.
I guess the compiler will tell me the cases which are suspect.

Mechanically, it appears easy enough to move the class just outside
its enclosing definition. That leads to oddness that I would like
to be able to explain in a one/two line comment.

Others reading the code will, or strongly should, surely ask about the weirdness.

Thanks,

Lee

@WojciechMazur
Copy link
Contributor

In the case of the equals_AnyRef our use case seems to be safe. I ain't the compiler expert and it seems quite strange that type check cannot happen at runtime. However, the danger is real (scala/scala3#4812).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants