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.

Sign up for GitHub

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

Jump to bottom

add eq/ne extension for AnyRef|Null to Scala3RunTime #14632

Merged
merged 12 commits into from
May 10, 2022
Merged

add eq/ne extension for AnyRef|Null to Scala3RunTime #14632

merged 12 commits into from
May 10, 2022

Conversation

olhotak
Copy link
Contributor

@olhotak olhotak commented Mar 7, 2022

Addresses part 3 of #14622 .

@olhotak olhotak requested review from noti0na1 and removed request for noti0na1 March 7, 2022 15:01
@olhotak
Copy link
Contributor Author

olhotak commented Mar 7, 2022

It seems this also needs to wait for an update of the reference compiler for bootstrapping. So I'm splitting it into two PRs: one that adds the extension methods to Scala3Runtime, and a second PR will make the compiler use these methods instead of eqn and nen.

@smarter smarter added the needs-minor-release This PR cannot be merged until the next minor release label Mar 7, 2022
@smarter smarter added this to the 3.2.0-RC1 milestone Mar 7, 2022
@olhotak
Copy link
Contributor Author

olhotak commented May 9, 2022

This is failing CoverageTests:

Error:  Test dotty.tools.dotc.coverage.CoverageTests.checkCoverageStatements failed: java.lang.AssertionError: /tmp/coverage16107752629811232167/scoverage.coverage differs from expected /__w/dotty/dotty/tests/coverage/pos/Inlined.scoverage.check, took 1.075 sec
1039

@smarter what does this mean? How can I determine whether this is a bug or just that the .check file needs to be updated?

@smarter
Copy link
Member

smarter commented May 9, 2022

The test is in https://github.com/lampepfl/dotty/blob/main/tests/coverage/pos/Inlined.scala and the expected output in https://github.com/lampepfl/dotty/blob/main/tests/coverage/pos/Inlined.scoverage.check. I assume this is failing just because the line numbers of the assert macro changed, and testCompilation --update-checkfiles will fix it.

@olhotak
Copy link
Contributor Author

olhotak commented May 9, 2022

It seems testCompilation --update-checkfiles doesn't run the coverage tests:

sbt:scala3> testCompilation --update-checkfiles
[info] Test run dotty.tools.dotc.CompilationTests started
[info] Test dotty.tools.dotc.CompilationTests.negAll started
[=======================================] completed (1443/1443, 0 failed, 78s)
[info] Test dotty.tools.dotc.CompilationTests.runAll started
[=======================================] completed (1470/1470, 0 failed, 110s)
[info] Test dotty.tools.dotc.CompilationTests.pickling started
[=======================================] completed (3760/3760, 0 failed, 92s)
[info] Test dotty.tools.dotc.CompilationTests.rewrites started
[=======================================] completed (1/1, 0 failed, 0s)
[=======================================] completed (1/1, 0 failed, 0s)
[=======================================] completed (1/1, 0 failed, 0s)
[=======================================] completed (1/1, 0 failed, 0s)
[=======================================] completed (1/1, 0 failed, 0s)
[=======================================] completed (1/1, 0 failed, 0s)
[=======================================] completed (1/1, 0 failed, 0s)
[=======================================] completed (1/1, 0 failed, 0s)
[=======================================] completed (8/8, 0 failed, 0s)
[info] Test dotty.tools.dotc.CompilationTests.pos started
[=======================================] completed (2702/2702, 0 failed, 114s)
[info] Test dotty.tools.dotc.CompilationTests.checkInit started
[=======================================] completed (141/141, 0 failed, 1s)
[=======================================] completed (40/40, 0 failed, 0s)
[=======================================] completed (23/23, 0 failed, 1s)
[=======================================] completed (1/1, 0 failed, 0s)
[=======================================] completed (1/1, 0 failed, 0s)
[=======================================] completed (1/1, 0 failed, 0s)
[info] Test dotty.tools.dotc.CompilationTests.posTwice started
[=======================================] completed (133/133, 0 failed, 20s)
[info] Test dotty.tools.dotc.CompilationTests.explicitNullsNeg started
[=======================================] completed (59/59, 0 failed, 4s)
[info] Test dotty.tools.dotc.CompilationTests.explicitNullsPos started
[=======================================] completed (74/74, 0 failed, 7s)
[info] Test dotty.tools.dotc.CompilationTests.explicitNullsRun started
[=======================================] completed (9/9, 0 failed, 0s)
[info] Test dotty.tools.dotc.CompilationTests.fuzzyAll started
[=======================================] completed (184/184, 0 failed, 3s)
[info] Test dotty.tools.dotc.CompilationTests.genericJavaSignatures started
[=======================================] completed (23/23, 0 failed, 0s)

================================================================================
Test Report
================================================================================

25 suites passed, 0 failed, 25 total
[info] Test run dotty.tools.dotc.CompilationTests finished: 0 failed, 0 ignored, 12 total, 441.386s
[info] Test run dotty.tools.dotc.BootstrappedOnlyCompilationTests started
[info] Test run dotty.tools.dotc.BootstrappedOnlyCompilationTests finished: 0 failed, 0 ignored, 0 total, 0.0s
[info] Test run dotty.tools.dotc.coverage.CoverageTests started
[info] Test run dotty.tools.dotc.coverage.CoverageTests finished: 0 failed, 0 ignored, 0 total, 0.0s
[info] Passed: Total 12, Failed 0, Errors 0, Passed 12
[success] Total time: 443 s (07:23), completed May 9, 2022, 1:29:19 p.m.
sbt:scala3>

@smarter
Copy link
Member

smarter commented May 9, 2022 •
edited
Loading

Sorry, the coverage tests are only run with the bootstrapped compiler so you need scala3-compiler-bootstrapped/testCompilation --update-checkfiles

@olhotak olhotak requested a review from smarter May 9, 2022 17:23
smarter
smarter approved these changes May 9, 2022
View reviewed changes
Copy link
Member

@smarter smarter left a comment

Choose a reason for hiding this comment

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

Could you add some documentation comments for these methods? Otherwise LGTM.

@olhotak olhotak enabled auto-merge May 9, 2022 20:34
smarter
smarter reviewed May 9, 2022
View reviewed changes
library/src/scala/runtime/stdLibPatches/Predef.scala Outdated

/** Enables an expression of type `T|Null`, where `T` is a subtype of `AnyRef`, to be checked for `null`
* using `eq` and `ne`, rather than only `==` and `!=`. This is needed because `Null` no longer has
* `eq` or `ne` methods, only `==` and `!=` inherited from `Any`. */
Copy link
Member

Choose a reason for hiding this comment

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

I think comments need to be on individual methods, not sure if scaladoc/IDEs pick up anything from the extension block.

@olhotak olhotak disabled auto-merge May 9, 2022 20:41
@olhotak olhotak merged commit cb22645 into scala:main May 10, 2022
@olhotak olhotak deleted the add-eq-ne branch May 10, 2022 05:13
@noti0na1 noti0na1 mentioned this pull request May 13, 2022
Merged
@Kordyjan Kordyjan modified the milestones: 3.2.0-RC1, 3.2.0 Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smarter smarter smarter approved these changes

Assignees

@olhotak olhotak

Labels
needs-minor-release This PR cannot be merged until the next minor release
Projects
None yet
Milestone
3.2.0
Development

Successfully merging this pull request may close these issues.
3 participants
@olhotak @smarter @Kordyjan