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

Some givens ignore @compileTimeOnly annotation #17002

Closed
pweisenburger opened this issue Feb 23, 2023 · 5 comments · Fixed by #17091
Closed

Some givens ignore @compileTimeOnly annotation #17002

pweisenburger opened this issue Feb 23, 2023 · 5 comments · Fixed by #17091

Comments

@pweisenburger
Copy link
Contributor

pweisenburger commented Feb 23, 2023

Compiler version

3.3.1-RC1-bin-20230216-2507577-NIGHTLY

Minimized code

Some givens ignore their @compileTimeOnly annotation and let code compile when it should error. See comments in the code below:

import scala.annotation.compileTimeOnly

sealed trait Test[T]

object Test:
  @compileTimeOnly("Error")
  given test0[T]: Test[T] = ???

  @compileTimeOnly("Error")
  given test1[T]: Test[T]()

  test0 // produces a compiler error, which is correct

  test1 // compiles but should not

EDIT: A further observation (that may be related): The following code fails to compile but should IMHO:

import scala.annotation.compileTimeOnly

sealed trait Test

object Test:
  @compileTimeOnly("Error")
  given Test()

Error message:

[error] 35 |object Test:
[error]    |       ^
[error]    |       Error
[error] one error found
@pweisenburger pweisenburger added itype:bug stat:needs triage Every issue needs to have an "area" and "itype" label labels Feb 23, 2023
@Kordyjan Kordyjan added area:transform area:erasure and removed stat:needs triage Every issue needs to have an "area" and "itype" label area:transform labels Feb 24, 2023
@Kordyjan Kordyjan added this to the Future versions milestone Feb 24, 2023
@odersky
Copy link
Contributor

odersky commented Mar 7, 2023

The second failing test is due to the method ordinal that's generated for the mirror of Test.

import scala.annotation.compileTimeOnly

sealed trait Test

object Test:
  @compileTimeOnly("Error")
  given Test()

@odersky
Copy link
Contributor

odersky commented Mar 7, 2023

The first example works as expected if @compileTimeOnly is replaced with erased.

The reason it does not work with compileTimeOnly is that annotations are not copied from the class to the accessor. I believe it would be dangerous to copy all annotations, so we'd have to make a special case for @compileTimeOnly. Given that compileTimeOnly is meant to be replaced by erased, the problem will be solved eventually anyway.

@nicolasstucki
Copy link
Contributor

I'm not sure that we can replace @compileTimeOnly with erased. @compileTimeOnly complements erased with the ability to give an extra message.

Probably, all @compileTimeOnly should be marked as erased. Though we should double-check in the community if there are uses where this would not work. One case that could fail (but not sure if it exists) is if we override a method with @compileTimeOnly, stating that this method should have been optimized away but is still callable through the overridden version.

@pweisenburger
Copy link
Contributor Author

The reason it does not work with compileTimeOnly is that annotations are not copied from the class to the accessor. I believe it would be dangerous to copy all annotations, so we'd have to make a special case for @compileTimeOnly.

This sounds to me like the more principled way to control where compileTimeOnly goes are the scala.annotation.meta._ annotations. But maybe this is not possible in this case?

In any case, I would also advocate keeping the possibility to add a customized message to the compiler error.

@odersky
Copy link
Contributor

odersky commented Mar 12, 2023

We could use scala.annotation.meta.companionClass and companionMethod for that. Right now, these meta annotations are ignored in dotty. They don't work for implicit classes either.

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.

4 participants