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

scala.compiletime.ops.long.S doesn't work #18342

Closed
unkarjedy opened this issue Aug 4, 2023 · 10 comments · Fixed by #18426
Closed

scala.compiletime.ops.long.S doesn't work #18342

unkarjedy opened this issue Aug 4, 2023 · 10 comments · Fixed by #18426

Comments

@unkarjedy
Copy link
Contributor

Related #13400

Compiler version

3.3.1-RC4

Minimized code

@main def foo(): Unit =
  //This works:
  val value1: scala.compiletime.ops.int.S[0] = 1

  //This doesn't work:
  val value2: scala.compiletime.ops.long.S[0L] = 1L

Output

Compiler generates error for 1L:
image

Expectation

No compilation error.
Type scala.compiletime.ops.long.S[0L] should be equal to 1L

@unkarjedy unkarjedy added itype:bug stat:needs triage Every issue needs to have an "area" and "itype" label labels Aug 4, 2023
@unkarjedy
Copy link
Contributor Author

JFTR previously found bug in a similar subsystem: #18340

@dwijnand
Copy link
Member

dwijnand commented Aug 7, 2023

Yeah, makes sense as https://github.com/lampepfl/dotty/blob/72f9d7afc3acfd8201e164da6608b99bb9218b64/compiler/src/dotty/tools/dotc/core/TypeComparer.scala#L1488

only considers ops.int, not also the equivalent in ops.long. Seems like a simple fix.

@dwijnand dwijnand added area:typer Spree Suitable for a future Spree and removed stat:needs triage Every issue needs to have an "area" and "itype" label labels Aug 7, 2023
@soronpo
Copy link
Contributor

soronpo commented Aug 15, 2023

Is there a reason to keep S[T] as a special case instead of changing it to S[T <: Int] = +[T, 1] and S[T <: Long] = +[T, 1L] accordingly? We might get some performance penalty for something that relies solely on S, but it seems weird to keep specialized now that have such a wide variety of operations.

@sjrd
Copy link
Member

sjrd commented Aug 15, 2023

S[-1] is not defined, but +[-1, 1] =:= 0. So S is not a special-case of +[T, 1]. Also I think S[MaxValue] is not defined but +[MaxValue, 1] would be MinValue.

And then S needs to special-cased for match types anyway.

@soronpo
Copy link
Contributor

soronpo commented Aug 18, 2023

@unkarjedy what was your actual use-case for this? I'm thinking of issuing a PR deprecating it instead of fixing it, since it was a mistake to create is in the first place, IMO (my mistake).

@dwijnand
Copy link
Member

What's the mistake? Just unnecessary?

@soronpo
Copy link
Contributor

soronpo commented Aug 18, 2023

Yeah, it does not make sense to have an incremental compiletime numbering until 2^64.

@soronpo
Copy link
Contributor

soronpo commented Aug 18, 2023

FWIW, there is already a branch to fix this (no tests ATM), but I think it's better to deprecate it.
https://github.com/soronpo/dotty/tree/fix_longS

@mbovel mbovel removed the Spree Suitable for a future Spree label Aug 18, 2023
@soronpo
Copy link
Contributor

soronpo commented Aug 20, 2023

I issued a PR for deprecation. If it will be decided to keep and fix this feature I will complete that instead.

nicolasstucki added a commit that referenced this issue Aug 21, 2023
@unkarjedy
Copy link
Contributor Author

@unkarjedy what was your actual use-case for this?
I'm thinking of issuing a PR deprecating it instead of fixing it, since it was a mistake to create, is in the first place, IMO (my mistake).

I don't have a real world use case for this.
I randomly stumbled on this new language feature when working with some other stuff with support of Scala 3 in IntelliJ.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants