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

Reinstantiate restriction to transparent inline methods #20371

Merged
merged 5 commits into from
Jun 12, 2024

Conversation

odersky
Copy link
Contributor

@odersky odersky commented May 9, 2024

Reverts parts of #19922.

Fixes #20342
Fixes #20297

The logic that we should ignore declared result types of inline methods really only applies to transparent inlines.

@odersky
Copy link
Contributor Author

odersky commented May 9, 2024

It seems this breaks more in the CB. Not sure what to do, I think I'll give this up for now.

Copy link
Member

@noti0na1 noti0na1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a test for #20342, and it passes locally as well.
So, LGTM now.

@odersky
Copy link
Contributor Author

odersky commented May 14, 2024

@WojciechMazur So, this test fixes two regressions and probably opens two others that were fixed by #19922. Nevertheless, from a point of the program logic this is the right thing to do. Should we merge this (which would revert to the 3.4 and LTS behavior) or test the open CB before that?

@WojciechMazur
Copy link
Contributor

WojciechMazur commented May 15, 2024

I've run the OpenCB, it does in fact reintroduce the 2 regressions mentioned above. Seems to also introduce a new regression in softwaremill/tapir (although it released a new version in meantime, so the sources are slightly different). I can try to provide a minimization of this new problem.

Project Version Build URL Notes
kalin-rudnicki/harness 4.1.3 -> 4.2.5 Open CB logs #19415
softwaremill/tapir 1.10.0 -> 1.10.6 Open CB logs Cannot find a codec between types: String and T - exists already in main #20420
apache/pekko-connectors 1.0.2 Open CB logs #19479

I can't really tell should it be merged or not, as it seems like in each some projects would fail to compile.

@WojciechMazur
Copy link
Contributor

WojciechMazur commented May 16, 2024

Reproduction for softwaremill/tapir:
Edit: This issue is already failing on main, see: #20420

final class StrictEqual[V]
final class Less[V]
type LessEqual[V] = Less[V] | StrictEqual[V]

object TapirCodecIron:
  trait ValidatorForPredicate[Value, Predicate]
  trait PrimitiveValidatorForPredicate[Value, Predicate]
      extends ValidatorForPredicate[Value, Predicate]

  given validatorForLessEqual[N: Numeric, NM <: N](using
      ValueOf[NM]
  ): PrimitiveValidatorForPredicate[N, LessEqual[NM]] = ???
  given validatorForDescribedOr[N, P](using
      IsDescription[P]
  ): ValidatorForPredicate[N, P] = ???

  trait IsDescription[A]
  object IsDescription:
    given derived[A]: IsDescription[A] = ???

@main def Test = {
  import TapirCodecIron.{*, given}
  type IntConstraint = LessEqual[3]
  summon[ValidatorForPredicate[Int, IntConstraint]]
}

Without -source flag it compiles without any warning
Under -source:3.5 it yields warning:

-- Warning: /Users/wmazur/projects/sandbox/tapir.repro.scala:24:51 -------------
24 |  summon[ValidatorForPredicate[Int, IntConstraint]]
   |                                                   ^
   |Given search preference for TapirCodecIron.ValidatorForPredicate[Int, IntConstraint] between alternatives (TapirCodecIron.validatorForDescribedOr :
   |  [N, P]
   |    (using x$1: TapirCodecIron.IsDescription[P]):
   |      TapirCodecIron.ValidatorForPredicate[N, P]
   |) and (TapirCodecIron.validatorForLessEqual :
   |  [N, NM <: N]
   |    (implicit evidence$1: Numeric[N], x$1: ValueOf[NM]):
   |      TapirCodecIron.PrimitiveValidatorForPredicate[N, LessEqual[NM]]
   |) will change
   |Current choice           : the second alternative
   |New choice from Scala 3.6: none - it's ambiguous
1 warning found

Under 3.5-migration it fails with compilation errror:

-- [E172] Type Error: /Users/wmazur/projects/sandbox/tapir.repro.scala:24:51 ---
24 |  summon[ValidatorForPredicate[Int, IntConstraint]]
   |                                                   ^
   |Ambiguous given instances: both given instance validatorForDescribedOr in object TapirCodecIron and given instance validatorForLessEqual in object TapirCodecIron match type TapirCodecIron.ValidatorForPredicate[Int, IntConstraint] of parameter x of method summon in object Predef

Seems like the 3.5-migration flag is treated as the 3.6

@SethTisue
Copy link
Member

If the broken projects need only add a transparent here and there, that’s plausibly a reasonable thing to ask, but I think we'd need the project maintainers to confirm that it's an acceptable change, without unwanted other effects, in the context of their codebases.

@odersky
Copy link
Contributor Author

odersky commented Jun 11, 2024

@SethTisue One of the failing tests (#19415) could be fixed in its minimization by refining the inference scheme. The other is new code (#19479), using inline givens and could trivially be changed by adding a transparent. So I think we are good to merge.

odersky and others added 5 commits June 11, 2024 15:08
Reverts parts of scala#19922.

Fixes scala#20342, scala#20297

The logic that we should ignore declared result types of inline methods really only applies
to transparent inlines.
 - Take MatchTypes into account
 - Add test cases for tests that failed originally and would otherwise fail again
   after this PR. i19415.scala is fixed by the MatchType extension. i19749.scala was
   fixed by adding a `transparent`.
@odersky odersky merged commit 5cfcfa8 into scala:main Jun 12, 2024
24 checks passed
@odersky odersky deleted the fix-20342 branch June 12, 2024 14:05
@Kordyjan Kordyjan added this to the 3.5.1 milestone Jul 3, 2024
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 this pull request may close these issues.

Regression in augustnagro/magnum for implicit search Regression in getkyo/kyo for typer
5 participants