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

Expand compiletime.ops #13400

Merged
merged 10 commits into from
Nov 29, 2021
Merged

Conversation

soronpo
Copy link
Contributor

@soronpo soronpo commented Aug 26, 2021

This PR expands compiletime.ops as follows:

  • Adds ops.string.{Length, Substring, Matches}
  • Adds ops.int.NumberOfLeadingZeros
  • Adds ops.long as equivalent to ops.int, but for Long operations
  • Adds ops.float and ops.double
  • Adds ops.any.IsConst
  • Deprecates ops.int.ToString (from 3.2.x) and moves it to ops.any.ToString
  • Fixes constant termrefs types not being recognized (e.g.: one.type + two.type, where one and two are final vals).

@soronpo soronpo requested a review from MaximeKjaer August 27, 2021 00:04
Copy link
Contributor

@MaximeKjaer MaximeKjaer left a comment

Choose a reason for hiding this comment

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

In general, this looks good to me! But seeing that I'm not currently very active in the day-to-day Dotty development, I'm adding @OlivierBlanvillain as a reviewer, so that we have the approval of one of the core maintainers too.

I do also have a higher-level question about Strings in particular: should we aim to have full support of all the methods in java.lang.String? We're missing methods like charAt, startsWith, etc. Some of these methods also take objects, such as format which takes a String or a Locale for the locale parameter. We can't support Locale, but we could support String.

compiler/src/dotty/tools/dotc/core/Types.scala Outdated Show resolved Hide resolved
compiler/src/dotty/tools/dotc/core/Types.scala Outdated Show resolved Hide resolved
library/src/scala/compiletime/ops/int.scala Outdated Show resolved Hide resolved
tests/neg/singleton-ops-int.scala Show resolved Hide resolved
compiler/src/dotty/tools/dotc/core/Types.scala Outdated Show resolved Hide resolved
compiler/src/dotty/tools/dotc/core/Types.scala Outdated Show resolved Hide resolved
@soronpo
Copy link
Contributor Author

soronpo commented Sep 6, 2021

I do also have a higher-level question about Strings in particular: should we aim to have full support of all the methods in java.lang.String? We're missing methods like charAt, startsWith, etc. Some of these methods also take objects, such as format which takes a String or a Locale for the locale parameter. We can't support Locale, but we could support String.

I currently do not support Char. Seems redundant to me, and can always be converted to an operation on a string in length of 1. So CharAt is redundant if we have Substring. Additionally, Matches covers more use cases, and users can create their own StartsWith etc. using type alias compositions. I think this PR covers most common use cases, and if we implement the SIP https://contributors.scala-lang.org/t/pre-sip-custom-dependent-operation-types/5236 , then the rest can be supported rather easily.

@smarter smarter added the needs-minor-release This PR cannot be merged until the next minor release label Sep 6, 2021
@soronpo
Copy link
Contributor Author

soronpo commented Sep 6, 2021

@smarter since we are currently in RC1, can we still get this in 3.1.0 if there will be RC2?

@smarter
Copy link
Member

smarter commented Sep 6, 2021

No, only fixes to regressions are backported into the release branch.

@smarter
Copy link
Member

smarter commented Sep 6, 2021

Though the added operations could be added in 3.1.1 as @experimental (but ops.int.ToString cannot be deprecated until there's a non-experimental replacement).

@soronpo
Copy link
Contributor Author

soronpo commented Sep 6, 2021

OK, then the deprecation for int.ToString needs to change to 3.1.1. I'll handle that as well.

@mbovel
Copy link
Member

mbovel commented Oct 1, 2021

I am very interested by this PR, I was going to add the same fix for getting the underlying types of termrefs 😃

What is the status? Could we merge this soon or are there any blockers?

@smarter
Copy link
Member

smarter commented Oct 1, 2021

It needs to be reviewed and the added definitions need to be marked @experimental so we can drop the "needs-minor-release" label.

@soronpo
Copy link
Contributor Author

soronpo commented Oct 1, 2021

I am very interested by this PR, I was going to add the same fix for getting the underlying types of termrefs 😃

What is the status? Could we merge this soon or are there any blockers?

I'll try to complete this soon.

@cquiroz
Copy link
Contributor

cquiroz commented Oct 25, 2021

This PR would be very useful to me. I'd need it to cross build coulomb to dotty
erikerlandson/coulomb#130

… termref type not being considered.

fix check file

more ops wip

Added ops.float and ops.double
Update MiMaFilters.scala

Update MiMaFilters.scala
Main changes:
* Apply common protection against wrong number of arguments.
* Exception in an operation is converted into a type error.
@soronpo soronpo removed the needs-minor-release This PR cannot be merged until the next minor release label Oct 26, 2021
@soronpo
Copy link
Contributor Author

soronpo commented Oct 26, 2021

Ready for review @OlivierBlanvillain

@johnynek
Copy link

BTW: I think this PR really brings up a weakness of match types: we shouldn't need to have a PR in the compiler to implement these functions.

It seems like if metaprogramming could be combined with match types, then you could implement these functions in your own code. That seems like it would be a promising area.

@soronpo
Copy link
Contributor Author

soronpo commented Oct 28, 2021

BTW: I think this PR really brings up a weakness of match types: we shouldn't need to have a PR in the compiler to implement these functions.

It seems like if metaprogramming could be combined with match types, then you could implement these functions in your own code. That seems like it would be a promising area.

Hence, this Pre-SIP: https://contributors.scala-lang.org/t/pre-sip-custom-dependent-operation-types/5236/14

Copy link
Contributor

@OlivierBlanvillain OlivierBlanvillain left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -70,8 +73,6 @@ object Test {

val t48: ToString[213] = "213"
val t49: ToString[-1] = "-1"
val t50: ToString[0] = "-0" // error
val t51: ToString[200] = "100" // error
Copy link
Contributor

Choose a reason for hiding this comment

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

What's wrong with that test?

Copy link
Contributor Author

@soronpo soronpo Nov 22, 2021

Choose a reason for hiding this comment

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

What's wrong with that test?

Nothing was wrong. At first I wanted to remove all int.ToString tests because it was deprecated, but then I decided not to remove the tests until it was removed completely. So perhaps I forgot to bring these tests back as well. I could change the commit, but I don't think it's a big deal.

library/src/scala/compiletime/ops/long.scala Outdated Show resolved Hide resolved
@OlivierBlanvillain
Copy link
Contributor

OlivierBlanvillain commented Nov 17, 2021

BTW: I think this PR really brings up a weakness of match types: we shouldn't need to have a PR in the compiler to implement these functions.

@johnynek IMO it's perfectly normal to require compiler support for type-level operations on primitive types. In the long term, the goal should be to be on par with term-level operations on primitives. If somebody is interested in working on a follow-up PR to add support for Char and other String operations, it's very likely to get in!

@OlivierBlanvillain OlivierBlanvillain merged commit e7f7e19 into scala:master Nov 29, 2021
@@ -181,4 +183,47 @@ object int:
* ```
* @syntax markdown
*/
@deprecated("Use compiletime.ops.any.ToString instead.","3.2.0")
Copy link
Member

@smarter smarter Dec 2, 2021

Choose a reason for hiding this comment

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

@soronpo I think this warning will be shown even before 3.2.0 is released which means users will have to choose between a deprecated and an experimental definition, could you comment out the deprecated annotation for now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@smarter why would it show before? The version is specified. Is there a bug in @deprecated? If so, we should fix this instead.

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.

8 participants