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

Set default source version to 3.5 #20441

Merged
merged 6 commits into from
May 27, 2024
Merged

Set default source version to 3.5 #20441

merged 6 commits into from
May 27, 2024

Conversation

hamzaremmal
Copy link
Member

@hamzaremmal hamzaremmal commented May 20, 2024

Superseeds #20435
Closes #20415

[test_scala2_library_tasty]
[test_windows_full]
[test_java8]

@hamzaremmal hamzaremmal added the backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. label May 20, 2024
@hamzaremmal hamzaremmal added this to the 3.5.0-RC2 milestone May 20, 2024
@hamzaremmal hamzaremmal requested a review from odersky May 20, 2024 15:50
@hamzaremmal hamzaremmal marked this pull request as ready for review May 20, 2024 15:50
@hamzaremmal
Copy link
Member Author

How I fixed it

First, I've changed the default source version from 3.4 to 3.5. This change will trigger a number of uses in scala2-library-bootsrapped and scala2-library-cc. I've then cloned the source code of the scala2 library using the following commands in sbt: scala2-library-cc/run overwrite <path> and scala2-library-bootstrapped/run overwrite <path> (eg: scala2-library-cc/run overwrite scala/Array.scala). After cloning the missing files, I've proceeded with the changes to adapt the library to Scala 3.5

@hamzaremmal
Copy link
Member Author

@Kordyjan This PR will need to be backported to the 3.5 release branch and be included in 3.5.0-RC2

@odersky
Copy link
Contributor

odersky commented May 21, 2024

@hamzaremmal I added you to dotty-staging/dotty. So in the future you can push your PRs there or add commits to existing PRs.

@hamzaremmal hamzaremmal force-pushed the i20415 branch 11 times, most recently from 3b7fd50 to ac1341d Compare May 23, 2024 09:35
@hamzaremmal hamzaremmal force-pushed the i20415 branch 2 times, most recently from 8c0f0ce to f88a97d Compare May 23, 2024 15:03
@Gedochao Gedochao requested a review from natsukagami May 23, 2024 18:14
@hamzaremmal
Copy link
Member Author

hamzaremmal commented May 24, 2024

@odersky Should be good for review now. @natsukagami can you take a look at why semanticdb somtimes include the diagnostics and sometimes it doesn't (should not block this PR since I commented it for now)

@hamzaremmal hamzaremmal enabled auto-merge May 24, 2024 14:51
@natsukagami
Copy link
Contributor

We are emitting the following warning inside the semanticdb test

-- Warning: tests/semanticdb/expect/InventedNames.scala:35:20 ------------------
35 |val c = given_Double
   |                    ^
   |Given search preference for Int between alternatives (givens.intValue : Int) and (givens.given_Char : Char) will change
   |Current choice           : the second alternative
   |New choice from Scala 3.6: the first alternative

... but not always, this seems to be because of the usage of the -usejavacp flag. When used within sbt and with the bootstrapped compiler, my guess is that a bunch of new classpaths are passed into the compiler as javaUserClassPaths, some of them from sbt. This changes the order of the implicits that are found (between char and int...) in searchImplicit.

From the ordering used inside searchImplicit (see compareEligibles, Implicits.scala:1500) these two instances do not differ, so we end up with different ordering of the candidates depending on the order they were found (from elsewhere, I haven't figured out exactly where).

Further up we can see rank uses compareAlternatives (where the warning was introduced) to preemptively remove future worse candidates once it has found one success

case retained: SearchSuccess =>
val newPending =
if (retained eq found) || remaining.isEmpty then remaining
else remaining.filterConserve(cand =>
compareAlternatives(retained, cand) <= 0)
rank(newPending, retained, rfailures)

Here note that with two candidates given_Char and intValue, given_Char will always fail tryImplicit and the other will succeed.
However,

  • If given_Char comes before intValue in the candidate list, it gets eliminated immediately, and no warnings are thrown
  • If intValue comes before, then tryImplicit succeeds, and rank tries to preemptively compare intValue to the (to be failing anyway) given_Char candidate. This produces the warning.

I can see a few things to do here:

  • Enforce a stricter ordering of candidates so that it's more deterministic
  • Perhaps try to somehow not produce the warning when tryImplicit would fail anyway, since it's confusing to the user

@natsukagami natsukagami removed their request for review May 24, 2024 16:40
@odersky
Copy link
Contributor

odersky commented May 25, 2024

@natsukagami That's a great analysis. Let me see if we can somehow delay the warning to the point where we choose one of the implicits that are mentioned in it.

@odersky
Copy link
Contributor

odersky commented May 27, 2024

See #20480 for a fix that should suppress the spurious warnings.

@hamzaremmal hamzaremmal merged commit a594958 into scala:main May 27, 2024
19 checks passed
@hamzaremmal hamzaremmal deleted the i20415 branch May 27, 2024 14:18
@odersky
Copy link
Contributor

odersky commented May 27, 2024

We should remember to re-enable the semanticdb diagnostics when #20480 is merged.

@hamzaremmal
Copy link
Member Author

We should remember to re-enable the semanticdb diagnostics when #20480 is merged.

Indeed, I suggested to do it in the same PR since it was the trigger for this change

hamzaremmal added a commit that referenced this pull request May 27, 2024
The main part of this branch was superseded by #20441, but there were a
couple of tests to add, and also a change
in the treatment of looping implicits in 3.6
@WojciechMazur WojciechMazur added backport:accepted This PR needs to be backported, once it's been backported replace this tag by "backport:done" backport:done This PR was successfully backported. and removed backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. backport:accepted This PR needs to be backported, once it's been backported replace this tag by "backport:done" labels Jun 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:done This PR was successfully backported. how-i-fixed-it
Projects
None yet
4 participants