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

Add compiletime.ops.string.CharAt #14431

Merged
merged 2 commits into from
May 20, 2022

Conversation

OlivierBlanvillain
Copy link
Contributor

@OlivierBlanvillain OlivierBlanvillain commented Feb 8, 2022

This PR builds on #13400 to add a type-level CharAt operation. It's almost possible to define that function in user space:

type CharAt[R <: String, At <: Int] =
  Substring[R, At, At + 1]

The only difference being that Substring returns a String, while CharAt returns a Char.

@soronpo I've added you as a reviewer, let me know if you have time to have a look!

@soronpo
Copy link
Contributor

soronpo commented Feb 8, 2022

The implementation LGTM.
I'm not clear on motivation/use-case. Why is Char needed? And if it's needed why is just CharAt enough (and we don't need comparisons and other operations)?

@OlivierBlanvillain
Copy link
Contributor Author

OlivierBlanvillain commented Feb 9, 2022

I'm not clear on motivation/use-case. Why is Char needed? And if it's needed why is just CharAt enough (and we don't need comparisons and other operations)?

In my mind length and charAt are the two string primitives that one needs to analyze strings. They give an array-like view that is rather universal, and work pretty well with our current support for type-level integers.

In terms of use-case, I've been working on a regex library with type- and null-safe capture groups. It analyses string with recursive match types, and it's only missing CharAt. I haven't measured performance yet, but I suspect that working directly with characters should be slightly faster than building single-character strings with Substring...

@soronpo
Copy link
Contributor

soronpo commented Feb 9, 2022

OK. From the milestones I'm guessing that the compiler team is getting ready for 3.2.0-RC1.
If this PR will only be added there, I think it is better to slightly expand the scope of it, drop the @experimental for all the compiletime operations and uncomment the int.toString deprecation.
What do you think?

@OlivierBlanvillain
Copy link
Contributor Author

Sounds good, I'll update the PR!

@OlivierBlanvillain OlivierBlanvillain force-pushed the scala-compiletime-ops-string-charat branch from f08e8c0 to 62cf1c6 Compare March 1, 2022 13:41
@OlivierBlanvillain OlivierBlanvillain added this to the 3.2.0-RC1 milestone Mar 1, 2022
@OlivierBlanvillain
Copy link
Contributor Author

OlivierBlanvillain commented Mar 1, 2022

I updated the annotations. Did I understood correctly that we don't need to mark the new operation as @experimental since the next release (3.2.0) is non-minor?

@soronpo
Copy link
Contributor

soronpo commented Mar 1, 2022

Yes, but the question is if we need a SIP committee approval for all these operation additions, before we can merge this.
@smarter

@soronpo soronpo requested a review from smarter March 1, 2022 14:10
@smarter
Copy link
Member

smarter commented Mar 1, 2022

No, historically library additions didn't require SIP approval.

@soronpo soronpo removed the request for review from smarter March 1, 2022 14:10
Copy link
Contributor

@soronpo soronpo left a comment

Choose a reason for hiding this comment

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

LGTM

@smarter
Copy link
Member

smarter commented Mar 1, 2022

The baseVersion in the main branch is still 3.1.3-RC1, I don't know if it will be bumped before the next release so until this is changed we'd need the @experimental flag to merge this.

@soronpo
Copy link
Contributor

soronpo commented Mar 1, 2022

Why wouldn't it be bumped before? AFAIK, there is no rush on this, so can wait for 3.2.0-RC1.

@smarter
Copy link
Member

smarter commented Mar 1, 2022

Well, we need a reason to bump, that can be because enough experimental changes have accumulated or because there's some big PR that requires a bump and we don't want to keep it open too long, I think that it'd make sense to do it now but it requires someone to take the time to go through experimental changes, etc.

@bishabosha bishabosha added the needs-minor-release This PR cannot be merged until the next minor release label Mar 7, 2022
Copy link
Contributor

@Kordyjan Kordyjan left a comment

Choose a reason for hiding this comment

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

All symbols that are made not-experimental should be marked with @since("3.2") annotation.

This is no longer true.

@OlivierBlanvillain OlivierBlanvillain force-pushed the scala-compiletime-ops-string-charat branch from 62cf1c6 to 263b4d9 Compare April 6, 2022 15:55
@OlivierBlanvillain
Copy link
Contributor Author

Blocked on #14866 and its followups.

@mbovel
Copy link
Member

mbovel commented May 10, 2022

Seems that we still cannot use @since("3.2") with 3.1.3-RC2 as the reference version because it was released after #14866 was merged.

Are these annotations still needed with the new versioning scheme? @Kordyjan


Are we sure we want to remove the experimental flags? @soronpo @OlivierBlanvillain

I am asking because I am investigating more general ways to represent term applications at the type-level, which would make these types unneeded. For example, if we can represent the type of the result of an addition natively with something like val z: {x + y} = x + y, we would not need a separate + type anymore.

Also, there are still some open questions about the signatures of these types like singletonness and variance:

type +[A <: Singleton & Int, B <: Singleton & Int] <: Singleton & Int
type +[+A <: Int, +B <: Int] <: Int
type +[A <: Int, B <: Int] <: Int

(I am actually now pretty sure that I should revert #14452 and go with the third option.)

Is it a blocker for you to have these types as experimental @soronpo?

@soronpo
Copy link
Contributor

soronpo commented May 10, 2022

Are we sure we want to remove the experimental flags? @soronpo @OlivierBlanvillain

I think the feature is useful and was enough time under @experimental. There is no "surprise" element to it.

Is it a blocker for you to have these types as experimental @soronpo?

Not a blocker for me. But maybe there are users of the singleton-ops library that it's a blocker for them to migrate to Scala 3.

I am asking because I am investigating more general ways to represent term applications at the type-level, which would make these types unneeded. For example, if we can represent the type of the result of an addition natively with something like val z: {x + y} = x + y, we would not need a separate + type anymore.

It would be interesting to see. But this is a language change vs. compiletime.ops which is a library change. So I don't see such language change merged before Scala 3.3.0, so it may make sense to just merge this PR for users that require it and deprecate it later IF we find a better approach.

@Kordyjan
Copy link
Contributor

Kordyjan commented May 16, 2022

@mbovel:

Are these annotations still needed with the new versioning scheme?

No. My previous comment is now outdated.

@OlivierBlanvillain OlivierBlanvillain force-pushed the scala-compiletime-ops-string-charat branch from 263b4d9 to dd0bab0 Compare May 16, 2022 14:19
@mbovel
Copy link
Member

mbovel commented May 16, 2022

We discussed this with the compiler team and decided that type-level operations should stay experimental for now, to let us time to evaluate potential alternatives.

@OlivierBlanvillain
Copy link
Contributor Author

OlivierBlanvillain commented May 16, 2022

What about the operations that are already non-experimental, should we annotate those as experimental?

I would personally advocate for making what we have non-experimental since it's working and already being used, even if that means deprecating things later when we have better alternatives. For context, we've been discussing the possibility of adding a better representation for term-level functions in types since 2018 (#3887).

@mbovel
Copy link
Member

mbovel commented May 17, 2022

Oh, I didn't realize that some type-level operations were already stable.

So we re-discussed it and I got the approval for making all operations in this PR non-experimental 🥳

@mbovel
Copy link
Member

mbovel commented May 17, 2022

Last failing tests seem to be related to a list of experimental APIs that needs to be updated:

java.lang.AssertionError: assertion failed: Failed @experimental definitions check

Listed @experimental definition was not found in the library
scala.compiletime.ops.any$.IsConst
scala.compiletime.ops.any$.ToString
scala.compiletime.ops.double
scala.compiletime.ops.double$
scala.compiletime.ops.float
scala.compiletime.ops.float$
scala.compiletime.ops.int$.NumberOfLeadingZeros
scala.compiletime.ops.int$.ToDouble
scala.compiletime.ops.int$.ToFloat
scala.compiletime.ops.int$.ToLong
scala.compiletime.ops.long
scala.compiletime.ops.long$
scala.compiletime.ops.string$.Length
scala.compiletime.ops.string$.Matches
scala.compiletime.ops.string$.Substring

If experimental definition was removed or stabilized, remove from the list in tests/run-custom-args/tasty-inspector/stdlibExperimentalDefinitions.scala

@mbovel mbovel self-requested a review May 17, 2022 15:48
@mbovel mbovel requested a review from Kordyjan May 17, 2022 15:49
@OlivierBlanvillain OlivierBlanvillain force-pushed the scala-compiletime-ops-string-charat branch from dd0bab0 to 96a4c1b Compare May 17, 2022 16:48
@mbovel
Copy link
Member

mbovel commented May 20, 2022

Let's get that merged :)

@OlivierBlanvillain
Copy link
Contributor Author

If that's OK I'll let someone else pull the trigger because of the release/version implications, which I'm not really on top of :)

@mbovel mbovel merged commit d75c32d into scala:main May 20, 2022
@mbovel mbovel deleted the scala-compiletime-ops-string-charat branch May 20, 2022 23:30
"scala.compiletime.ops.long", "scala.compiletime.ops.long$",
"scala.compiletime.ops.string$.Length",
"scala.compiletime.ops.string$.Matches",
"scala.compiletime.ops.string$.Substring",
Copy link
Contributor

Choose a reason for hiding this comment

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

@mbovel didn't we say that these operations would stay experimental?

Copy link
Member

Choose a reason for hiding this comment

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

See #14431 (comment); when we first discussed it, I hadn't realized that some compile-time operations were already stable. Given this, I re-discussed it with Martin and he agreed to make these operations stable as well.

The argument is that operations on long, float and double are exactly the same as the ones on int which are already stable, so it makes sense to make them stable as well. In term of maintenance, this doesn't change much.

Including operations on strings as well makes sense are they are important building blocks to do anything useful with strings at the type-level (Olivier used them for his work on regular expressions for example).

@Kordyjan Kordyjan modified the milestones: 3.2.0-RC1, 3.2.0 Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-minor-release This PR cannot be merged until the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants