Fix prioritization of givens over implicits

[EugeneFlesselle]

• Fix a typo in Applications#compare#isAsGood#isGiven which always used alt1, to determine if the altneratives passed to isAsGoodValueType were givens.

• Update isAsGoodValueType to not prefer givens over extensions, by negating the isGiven parameter, letting extensions and givens now have the same priority level as far as that rule is concerned.

• Modify given/implicit definitions from the Namer and the PPrint community-project to resolve ambiguity errors introduced by the changes.
  See the commit message there for details on changes to the community-build.

Fixes #21212

[odersky]

I am not sure how the code changes relate to the description in the PR. AFAIK, the only changes apply to extension methods, which are now treated as equivalent to givens, where before they were equivalent to extension methods.

Is there another change that I have overlooked?

[EugeneFlesselle]

Is there another change that I have overlooked?

The changes in the first commit 9ec671b are only fixing what I assumed was a typo in isGiven which was using the captured alt1 instead of the parameter alt:

scala3/compiler/src/dotty/tools/dotc/typer/Applications.scala

Lines 1824 to 1825 in d48ba03

	def isGiven(alt: TermRef) =
	alt1.symbol.is(Given) && alt.symbol != defn.NotGivenClass

That change fixes the issue in #21212 by now applying the logic prioritizing givens over implicit as intended in #19300.
Unfortunately that change seems to also cause errors with bootstrapping

[EugeneFlesselle]

The second commit addresses another error which was encountered following the first commit, namely that when isAsGoodValueType is called with an extension and a given conversion, it will prefer the conversion over the extension because only the former isGiven.
Which contradicted the logic from searchImplicit which preferred extension over conversions for member selection,
and in particular caused pos/i19715 to fail.

[odersky]

Ah, since the whole definition of isGiven was dropped I overlooked that change!

[EugeneFlesselle]

Ah, since the whole definition of isGiven was dropped I overlooked that change!

No worries, I've updated the commit messages so the changes will hopefully be clearer for the commit history

[WojciechMazur]

@EugeneFlesselle this now causes a warning under -source:3.5 (so also in 3.5.x backports) in https://github.com/scala/scala3/blob/a64c2956f83f8dc53c0bfd7707df4a6e27cd0796/tests/pos/not-looping-implicit.scala
I wonder if this change might be more invasive than expected

-- Warning: /Users/wmazur/projects/sandbox/src/main/scala/test.scala:49:59 -----
49 |  implicit lazy val inputValueSchema: Schema[InputValue] = Schema.gen
   |                                                           ^^^^^^^^^^
   |Given search preference for Schema[List[InputValue]] between alternatives (Schema.gen : [A]: Schema[A]) and (Schema.listSchema : [A](implicit ev: Schema[A]): Schema[List[A]]) will change
   |Current choice           : the second alternative
   |New choice from Scala 3.6: the first alternative
   |----------------------------------------------------------------------------
   |Inline stack trace
   |- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
   |This location contains code that was inlined from test.scala:23
23 |        val builder     = summonInline[Schema[t]].asInstanceOf[Schema[Any]]
   |                                       ^^^^^^^^^
   |- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
   |This location contains code that was inlined from test.scala:23
34 |        lazy val fields           = recurse[m.MirroredElemLabels, m.MirroredElemTypes]()
   |                                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
   |This location contains code that was inlined from test.scala:23
38 |  inline given gen[A]: Schema[A] = derived[A]
   |                                   ^^^^^^^^^^
   |- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
   |This location contains code that was inlined from test.scala:23
23 |        val builder     = summonInline[Schema[t]].asInstanceOf[Schema[Any]]
   |                                       ^
   |- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
   |This location contains code that was inlined from test.scala:23
31 |        lazy val members     = recurse[m.MirroredElemLabels, m.MirroredElemTypes]()
   |                               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
   |This location contains code that was inlined from test.scala:23
38 |  inline given gen[A]: Schema[A] = derived[A]
   |                                   ^^^^^^^^^^
    ----------------------------------------------------------------------------

I'll start the OpenCB to check it on the bigger number of codebases

[odersky]

This should not warn. The logic is wrong here. We should always prefer givens over implicits. The current logic makes implicits more general than givens, which means they will be chosen because we now prefer more general. It should probably be the reverse; we should make implicits more specific than givens.

EDIT: If we don't want to put this under a version flag (which would be cumbersome), we need to link in the choice with preferGeneral. Make implicits more specific than givens iff preferGeneral is true.

[EugeneFlesselle]

Minimization:

@main def Test =
  implicit def list[A](using A): List[A] = ???
  inline given gen[A]: A = ???

  given Int = ???
  summon[List[Int]]
  // Resolves to `list[Int](using given_Int)` in 3.5
  // Resolves to `gen[Nothing]`               in 3.6
  // And both versions warn about the changes correctly (i.e. representative of what is resolved)

We should always prefer givens over implicits. The current logic makes implicits more general than givens, which means they will be chosen because we now prefer more general.

Agreed! which is what we get here as far as I can tell. We used to pick list, but know pick gen since it is the only given of two, which we prefer in the new rules. The warning would then correct, unless there is something I missed ?

[EugeneFlesselle]

This should not be backported for 3.5, the original change this amended #19300 has been delayed to being a warning a 3.6 and change in 3.7.