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

Fix #9482: implement reflect.Manifest summoning #13129

Closed
wants to merge 4 commits into from

Conversation

bishabosha
Copy link
Member

This PR implements summoning for reflect.Manifest, reflect.OptManifest and reflect.ClassManifest following the algorithm present in Scala 2:
https://github.com/scala/scala/blob/0c011547b1ccf961ca427c3d3459955e618e93d5/src/compiler/scala/tools/nsc/typechecker/Implicits.scala#L1519

A special case is added in Definitions for reflect.ClassManifest so that is not de-aliased at the time of Implicit search, added in c84f057, so careful review is required here.

fixes #9482

@bishabosha
Copy link
Member Author

bishabosha commented Jul 22, 2021

So far there is no restriction on summoning - I can think of several options:

  • enable it only in source:3.0-migration - I think this could be a problem as Manifests themselves are not deprecated
  • forbid it in source:future - by this time the library may have deprecated/dropped them
  • enable it under one of the legacy language features?

@smarter
Copy link
Member

smarter commented Jul 22, 2021

I won't be available to review this in the near future but my first reaction here is that this is way too much code and complexity for a legacy and known-broken feature, I also don't understand why some of the logic and error messages reference TypeTag since this isn't something we support at all. IMO, it's fine to add support for the simplest usecases where Manifest is used instead of ClassTag, but we shouldn't aim to perfectly emulate what Scala 2 does (because we'll never match it exactly anyway).

@bishabosha
Copy link
Member Author

bishabosha commented Jul 22, 2021

The special casing for ClassManifest is causing too much trouble,

And I will try to simplify, there is no need to support type parameters as <:< as is deprecated.

I will also remove the special casing for TypeTag

@smarter
Copy link
Member

smarter commented Jul 22, 2021

So far there is no restriction on summoning - I can think of several options:

I suggest emitting a deprecation warning encouraging the use of ClassTag and http://dotty.epfl.ch/docs/reference/metaprogramming/toc.html whenever we materialize a Manifest.

@bishabosha
Copy link
Member Author

see #13142

@bishabosha bishabosha deleted the fix-9482 branch September 10, 2021 14:05
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.

OptManifest not resolved
2 participants