3.6.4-RC - false positive unused import

[OndrejSpanel]

Another report, to save my damaged reputation after #22690 fiasco. 😘

Compiler version

3.6.4-RC1, 3.6.4-RC2

Minimized code

import javax.swing.*
import javax.swing.event as swingEvent

type b = AbstractButton
type t = swingEvent.AncestorListener

Output

C:\Dev\Sandbox\src\main\scala\Main.scala:2:26
unused import
import javax.swing.event as swingEvent

Expectation

The import is not unused, the code does not compile without it.

See also this Scastie: https://scastie.scala-lang.org/OndrejSpanel/bHJcv9AhSS2DODsdcQFvAw/17

Note

Replacing wildcard import on the first line makes the error go away, this compiles fine:

import javax.swing.AbstractButton
import javax.swing.event as swingEvent

type b = AbstractButton
type t = swingEvent.AncestorListener

[som-snytt]

To remind myself, one change landed in 3.6.4

#22314

[OndrejSpanel]

one change landed in 3.6.4

This hints about another way to prevent the warning, which is:

import javax.swing.event as swingEvent
import javax.swing.*

or

import javax.swing.{event as swingEvent, *}

[Gedochao]

Last good release (pre-introducing the issue): 3.6.4-RC1-bin-20250112-ae980a7-NIGHTLY
First bad release: 3.6.4-RC1-bin-20250113-c10def4-NIGHTLY

The problem was introduced in af655c9 (cc @KacperFKorban)

Last bad release (post-introducing the issue): 3.7.0-RC1-bin-20250127-89c20f8-NIGHTLY
First good release: 3.7.0-RC1-bin-20250128-9cb97ec-NIGHTLY

The bisect didn't return clear results, but it seems the problem was fixed somewhere in #20894 (possibly 152a8cd)
cc @som-snytt

Since the issue won't appear in 3.7.0, I'm not adding the regression label, since we could technically say it's already been fixed.
The only question is whether we are too late to try to port it to 3.6.4-RC3.
cc @WojciechMazur

[WojciechMazur]

Backporting The Great CheckUnused refactor would be an overkill, both due to complexity/scope and due to amount of follow-up regressions it caused, but we might try to revert the change that introduced the regression while fixing regression introduced in 3.5.0.
If there would be no other issues we can release RC3 tomorrow. In such case the final 3.6.4 release would happen at the day of 3.7.0 cutoff - so it's a last moment do to it

[Gedochao]

@WojciechMazur sounds like it might be worth a shot. Your call.

[OndrejSpanel]

While I am honored to have fix for my issue to be considered for inclusion at last possible moment, and I had been often guilty of last-minutes fixes in my products myself (or maybe right because of this), I would argue against it. I do not think the issue is serious enough, the workarounds are easy and unless the revert is absolutely trivial, the risks of breaking something else can easily outweigh possible gains.

Practically speaking, my project will require much more maintenance to deal with #22690 than with this. While unused imports for constructor parameters appear about 30x in my project, this clash of renamed import with wildcard import appears just once there.

[som-snytt]

It's "just a warning" and therefore benign. I don't think it's worth expending energy on the old, brittle code.

[KacperFKorban]

Hmmm, IMHO this isn't really fixed. I stand by the fact that my fix was correct, it just exposed a different limitation of the unused checker. I think that this is one of the cases that simply cannot be precisely checked. The "fix" #20894 simply trades some false positives for some negatives e.g.

//> using scala 3.nightly
//> using options -Xprint:typer -Wunused:imports

import javax.swing.*
import javax.swing.event // warn with my fix and no warn now
                                                                                                       
type b = AbstractButton                                  
type t = event.AncestorListener

The underlying issue was already mentioned in multiple issues. (e.g. #17315)
The problem is that the output after typer is the following:

[[syntax trees at end of                     typer]] // wiadro/i22692.scala
package <empty> {
  import javax.swing.*
  import javax.swing.{event as swingEvent}
  final lazy module val i22692$package: i22692$package = new i22692$package()
  final module class i22692$package() extends Object() {
    this: i22692$package.type =>
    type b = javax.swing.AbstractButton
    type t = javax.swing.event.AncestorListener
  }
}

[som-snytt]

@KacperFKorban A specific import has higher precedence than a wildcard import. event is imported by swing.event.

The unused check is not trying to minimize the imports (which would be a useful feature for scalafix or scalafmt).

Seb added an attachment to track renames, which is still in use.

Please don't reference my fixes in "scare quotes".

The fundamental limitation of the previous unused check was that it did not model contexts precisely. The purpose of the refactor was to rely on the familiar Context, with the limitation that nesting level is not represented.

[KacperFKorban]

@som-snytt Sorry if it came out mean, wasn't my intention (though to be fair you were first to call my fix "old, brittle code").
I just meant that my fix is IMHO not a regression and simply just a trade-off.

Also, my understanding of an unused import is ~"It can be removed and the code still compiles and has the same semantics", so unless there is a spec for unused, your definition isn't better than mine.

[som-snytt]

@KacperFKorban sorry, I meant to say the old unused check turned out brittle. I made many efforts last summer to add fixes, lost to squashing, but "superconstructor context" finally broke the camel's back. I had known for some time that it would need a refactor.

I agree with your opinion about the commit in question.

"It can be removed and the code has different semantics" would be a less useful definition, indeed.

[som-snytt]

Not sure what the disposition of the ticket is, or whether the PR is against the correct branch, but I posted it in case it is just a one-line tweak that is simple.

The 3.7 code, which is similar, also lacks this check. If it works anyway, that is because it takes the highest precedence binding, since we know the code typechecks.

@OndrejSpanel thanks again, I also made the tweak on 3.7; that may be an aesthetic choice.

[WojciechMazur]

We've just discussed this topic internally and decided we'd release the 3.6.4-RC2 as 3.6.4 (final) without this change or revert of the PR that caused the regression in the RC3 release.
As the discussions proved the issue is non trivial to solve, each approach has it's own pros and cons

Thank you for investigating the issue and creating a PR!