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

Symbol.newClass does not support class parameters #21739

Closed
bishabosha opened this issue Oct 9, 2024 · 4 comments · Fixed by #21880
Closed

Symbol.newClass does not support class parameters #21739

bishabosha opened this issue Oct 9, 2024 · 4 comments · Fixed by #21880
Assignees
Labels
area:metaprogramming:reflection Issues related to the quotes reflection API itype:enhancement
Milestone

Comments

@bishabosha
Copy link
Member

bishabosha commented Oct 9, 2024

Compiler version

3.5.1

Minimized example

Mill's Cross Modules are implemented with a macro that generates a class, and a factory function that creates a new instance of the class given some context.

// mill definitions (simplified)
package mill

trait Module(using Context) 

object Cross:
  trait Module[String] extends mill.Module:
    def crossValue: String
// user code
trait MyCrossModule extends Cross.Module[String]
object myCross extends Cross[MyCrossModule]("foo", "bar")
// simplified generated code as part of implicit conversion from `"foo"` to `Factory[MyCrossModule]`

factoryArgs.map[MyCrossModule]({ (arg: "foo") =>
  class C(ctx: Context) extends MyCrossModule with mill.Module(using ctx) {
    def crossValue: String = arg
  }
  (classOf[C], ctx => new C(ctx))
})

The new class has to be generated via Macro, not quotes and splices, because the Specific cross module trait is not known statically.

The problem is that there is no way via Symbol.newClass to add the necessary (ctx: Context) parameter to the generated class C

Expectation

Allow to customise the parameters of the class, even if its just single term parameter list, I think generic is less needed because the macro can specialise the types.

@bishabosha bishabosha added stat:needs triage Every issue needs to have an "area" and "itype" label itype:enhancement area:metaprogramming:reflection Issues related to the quotes reflection API and removed stat:needs triage Every issue needs to have an "area" and "itype" label labels Oct 9, 2024
@jchyb jchyb self-assigned this Oct 17, 2024
@goshacodes
Copy link

goshacodes commented Jan 16, 2025

Not sure whether I should create a new issue or ask here. I think this issue is closely related to mine.

I have an issue ScalaMock/ScalaMock#191 created long time ago.

And to fix it I need possibility to annotate a class created with Symbol.newClass. I didn't find a way to do it currently.
Can this also be added?

@jchyb
Copy link
Contributor

jchyb commented Feb 4, 2025

@goshacodes I've now added the ability to add annotations to classes in #21880. I will probably end up changing the title/explanations of the PR before review from the compiler team members, but in terms of the code/functionality itself - I consider it done, and if nothing bad comes up, and after merging with @experimental for 3.7 I would like to stabilize it in 3.8/3.9. As a power user of newClass, It would be helpful to me if you could look at the added functionality/api/documentation there (only if you are available, of course), to see if something major is missing, before the main review.

@goshacodes
Copy link

Looks like everything is covered, has nothing special in mind currently, thank you!

@goshacodes I've now added the ability to add annotations to classes in #21880. I will probably end up changing the title/explanations of the PR before review from the compiler team members, but in terms of the code/functionality itself - I consider it done, and if nothing bad comes up, and after merging with @experimental for 3.7 I would like to stabilize it in 3.8/3.9. As a power user of newClass, It would be helpful to me if you could look at the added functionality/api/documentation there (only if you are available, of course), to see if something major is missing, before the main review.

@goshacodes
Copy link

goshacodes commented Feb 7, 2025

Found one test case recently, which looks strange:

  class MyClass
  class MyTycon[T]
  class WithGeneric[T, B[_]](b: B[T], t: T)

mock[WithGeneric[MyClass, Option]] // not compiles
mock[WithGeneric[MyClass, MyTycon]] // not compiles

Error is:

[error] 57 |      mock[WithGeneric[MyClass, Option]]
[error]    |      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[error]    |      Found:    Option[MyClass]
[error]    |      Required: Option[MyClass]
[error]    |

----
[error]    | Explanation (enabled by `-explain`)
[error]    |- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
[error]    |
[error]    | Tree: org.scalamock.util.Defaultable.default[Option[Int]].default.asInstanceOf[
[error]    |   Option[Int]]
[error]    | I tried to show that
[error]    |   Option[Int]
[error]    | conforms to
[error]    |   Option[Int]
[error]    | but none of the attempts shown below succeeded:
[error]    |
[error]    |   ==> Option[Int]  <:  Option[Int]
[error]    |     ==> Any  <:  Option[Int]  = false
[error]    |
[error]    | The tests were made under a constraint with:
[error]    |  uninstantiated variables: X0
[error]    |  constrained types: [X0]: X0
[error]    |  bounds:
[error]    |      X0
[error]    |  ordering:
[error]    |  co-deps:
[error]    |  contra-deps:

Now I'm replacing all generic types with applied types by hand and casting nulls to that type - will be nice to check if this can be done now with Symbol.newClass.

This is exactly what I do, maybe I'm just doing something wrong:
https://github.com/ScalaMock/ScalaMock/pull/585/files

UPDATE: found correct solution

@goshacodes I've now added the ability to add annotations to classes in #21880. I will probably end up changing the title/explanations of the PR before review from the compiler team members, but in terms of the code/functionality itself - I consider it done, and if nothing bad comes up, and after merging with @experimental for 3.7 I would like to stabilize it in 3.8/3.9. As a power user of newClass, It would be helpful to me if you could look at the added functionality/api/documentation there (only if you are available, of course), to see if something major is missing, before the main review.

@WojciechMazur WojciechMazur added this to the 3.7.0 milestone Mar 12, 2025
WojciechMazur pushed a commit that referenced this issue Mar 12, 2025
…Class in reflect API (#21880)

Instead of replacing the one newMethod we have, we instead add two more
with varying complexity (similarly to how newMethod is handled). This is
also so we can keep the initial newClass implementation (the one
creating newClass with public empty primary constructor) intact, which
despite being experiemental - already sees use in libraries and
projects.

Fixes #21739 and addresses some
old TODOs (from the stdlibExperimentalDefinitions.scala file).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:metaprogramming:reflection Issues related to the quotes reflection API itype:enhancement
Projects
None yet
4 participants