[Scala3] Add support for handling unapplys in NamingPhase

[adkian-sifive]

Missing feature from #4889

Tried to keep the implementation as logically similar as possible to the Scala 2 version

Contributor Checklist

• Did you add Scaladoc to every public function/method?
• Did you add at least one test demonstrating the PR?
• Did you delete any extraneous printlns/debugging code?
• Did you specify the type of improvement?
• Did you add appropriate documentation in docs/src?
• Did you request a desired merge strategy?
• Did you add text to be included in the Release Notes for this change?

Type of Improvement

• Internal or build-related (includes code refactoring/cleanup)

Desired Merge Strategy

• Squash: The PR will be squashed and merged (choose this if you have no preference).

Release Notes

NamingPhase updated to handle unapplys similar to Scala 2

Reviewer Checklist (only modified by reviewer)

• Did you add the appropriate labels? (Select the most appropriate one based on the "Type of Improvement")
• Did you mark the proper milestone (Bug fix: 3.6.x, 5.x, or 6.x depending on impact, API modification or big change: 7.0)?
• Did you review?
• Did you check whether all relevant Contributor checkboxes have been checked?
• Did you do one of the following when ready to merge:
  • Squash: You/ the contributor Enable auto-merge (squash) and clean up the commit message.
  • Merge: Ensure that contributor has cleaned up their commit history, then merge with Create a merge commit.

[adkian-sifive]

refactor this handling

[jackkoenig]

I'm unresolving this because I think this should be cleaned up.

[jackkoenig]

I do not want regex matching in performance critical places like compiler plugins, can we do this a different way?

[jackkoenig]

Some ideas--Name has some methods that might be useful, like is(kind: NameKind). Alternatively, if we know the qual is of type tuple and that the name is the right kind of name (probably isTermName? or one of the name kinds), then we can potentially extract the number directly, or just .tail the String if we're sure.

[adkian-sifive]

tuple accessor names are predictable, so we can just check for tuple types and extract accessor names manually based on the pattern. i'll try it out

[jackkoenig]

If there is an actual flag or type in the AST that indicates a tuple extraction that would be better though. It is not wise to rely on the names being predictable in case that predictability changes.

[jackkoenig]

Also we want to distinguish it from user code that might look similar, e.g. what happens if I write this:

val foo = returnsATuple(...)
val bar = foo._1

If the plugin matches that, it's a bad thing.

Here's why, what if someone wrote this?

val foo @ (fizz, _) = returnsATuple(...)
// then later for some reason:
val bar = foo._1

It's important that we actually distinguish what is an unapply and not just match the pattern of extractor names.

[adkian-sifive]

yeah that's exactly the case i'm checking. this is the issue with unapplys in scala 3 in general though, iirc there's no way to tell between unapplys and tuple destructuring

[jackkoenig]

yeah that's exactly the case i'm checking. this is the issue with unapplys in scala 3 in general though, iirc there's no way to tell between unapplys and tuple destructuring

Is that true in Scala 3 in general or is it true because we're running as a later phase?

Also, what about Flags like Synthetic, Case, CaseAccessor?

[adkian-sifive]

Pretty sure this was still an issue in the original naming PR and we were running earlier then

Regarding flags, these are used in helpers to filter stuff but these same flags are used in, for example, Match-based unapplys so on their own won't help here

[jackkoenig]

I'm approving this to unblock moving tests over, but I still want several improvements, see comments but certainly including:

1. Do collectUnapplyNames in a single traversal of the statements
2. Remove regex in collectUnapplyNames
3. Clean up syntheicTupleValues stuff
4. Get rid of String-y stuff in okVal
5. Use built-ins for isTupleType and probably tupleArity

[jackkoenig]

I'm unresolving this because I think this should be cleaned up.