Fix the non-miniphase tree traverser

[PaulCoral]

Fixes #16678. The issue is related to the second tree traverser (needed as miniphase traverser do not cover every cases).

Symbols are added to a set during the traversal of their own definition to avoid recording recursive. In the second tree traverser this symbol is never removed, so the following usage are never cached, resulting in false positive.

@szymon-rd

[szymon-rd]

Could you fix the conflicts? @PaulCoral

[smarter]

I'm concerned that this phase uses both the miniphase traversing logic and a TreeTraverser, aren't you traversing the same trees multiple times?

[szymon-rd]

@smarter Paul responded to this concern in his initial PR:
#16157 (comment)

[PaulCoral]

Could you fix the conflicts? @PaulCoral

Yes, this seems OK to me.

(also I resolve conflict with main and it passes the test locally)

[smarter]

Paul responded to this concern in his initial PR:

I'm still somewhat confused:

• you can pattern match on the tree in prepareOthers if you need to do different things on different trees, this doesn't require doing a traversal
• if you really need to do a full traversal, then maybe the phase should not be a mini phase and instead just do the traversal

[szymon-rd]

I agree that this is a bit confusing. But IMO it should land in another issue and PR.

[PaulCoral]

Paul responded to this concern in his initial PR:

I'm still somewhat confused:

• you can pattern match on the tree in prepareOthers if you need to do different things on different trees, this doesn't require doing a traversal
• if you really need to do a full traversal, then maybe the phase should not be a mini phase and instead just do the traversal

The current design of the traversal may appear to be quite confusing and has been a subject of some debate. Here, the full traversal is needed as the miniphase does not traverse many type-related parts of the tree, and thus, the symbols are not registered, resulting in many false positives (e.g. an import used only as a type argument). The first design in #16157 was using only a Traversers. It was also decided that it could be useful to convert to miniphase, as in the future, some linter options could be added through other miniphases, resulting in a single tree traversal. Using a TreeTraverser avoids duplicating the code of the foldOver method in Trees.scala, and the traversals are usually very short (often only type parameter clauses).

I agree that this is a bit confusing. But IMO it should land in another issue and PR.

I agree that it may not be optimal, a should maybe be discussed.

[smarter]

Thanks for the explanation. I think it would be helpful to add a pattern match on tree in prepareForOther to be explicit about the trees that end up here (similar to https://github.com/lampepfl/dotty/blob/b824375daa21f96b12ddbfa8ea98672adc3d2a1a/compiler/src/dotty/tools/dotc/typer/CrossVersionChecks.scala#L165-L172).
Even if we end up doing the same thing for all trees, it would be helpful to understand what the code does exactly. But this doesn't have to be done in this PR of course.

[szymon-rd]

Merging it now.