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

Support for selectively overriding individual fields in Dialect #2751

Closed
blast-hardcheese opened this issue May 10, 2022 · 6 comments · Fixed by #2759
Closed

Support for selectively overriding individual fields in Dialect #2751

blast-hardcheese opened this issue May 10, 2022 · 6 comments · Fixed by #2759

Comments

@blast-hardcheese
Copy link
Contributor

Seems possibly related to #2750
Definitely related to #2734

Now that Foo[?] syntax is default, I expected to be able to

implicit val d = scala.meta.dialects.Scala212.withAllowQuestionMarkPlaceholder(false)

... in order to restore the previous functionality, but I'm greeted with

<console>:14: error: dialect of type scala.meta.Dialect is not supported by quasiquotes (to fix this, import something from scala.meta.dialects, e.g. scala.meta.dialects.Scala212)

Digging into the code, I found this message:

// For now I'll just prohibit quasiquotes for situations when `dialectTree` doesn't point to one of the predefined dialects.

// A natural extension to this would be to allow any static value, not just predefined dialects.
// Furthermore, we could further relax this restriction by doing parsing for a superset of all dialects and then
// delaying validation of resulting ASTs until runtime.

which doesn't inspire much hope. How can this be resolved?

I guess this is only a transitional issue, while everybody upgrades to the latest versions of Scala212 and 213, and I can stay on the old version of scalameta, but I've not seen any discussion around this so I thought I'd ask.

Thank you for your excellent library!


A full reproduction case:

[info] Starting scala interpreter...
Welcome to Scala 2.12.15 (OpenJDK 64-Bit Server VM, Java 11.0.11).
Type in expressions for evaluation. Or try :help.

scala> import scala.meta._
import scala.meta._

scala> q"Foo[_]".syntax
res0: String = Foo[?]  // My consumers can't compile this yet

// An attempt to supply an instance of Dialect is greeted with this message
scala> def foo(implicit dialect: Dialect): String = q"Foo[_]".syntax
<console>:14: error: dialect of type scala.meta.Dialect is not supported by quasiquotes (to fix this, import something from scala.meta.dialects, e.g. scala.meta.dialects.Scala212)
       def foo(implicit dialect: Dialect): String = q"Foo[_]".syntax
                                                    ^
@tgodzik
Copy link
Collaborator

tgodzik commented May 12, 2022

Thanks for reporting! The issue only happens with quasiquotes? As a workaround, you could create the tree manually, does it work in that case?

I am not sure currently how to properly fix it, we could have and additional dialect Scala212Pre16 maybe?

@blast-hardcheese
Copy link
Contributor Author

The issue only happens with quasiquotes?

scala> import scala.meta._
import scala.meta._

scala> t"Foo[_]"
res0: meta.Type.Apply = Foo[_]  // Represented ideally in this case by toString

scala> t"Foo[_]".syntax
res1: String = Foo[?]  // Since it resolves the `Dialect` on `.syntax`, this now does not do what I'm after

scala> res0.structure // Pull the structure out from the one that appeared to be represented ideally
res2: String = Type.Apply(Type.Name("Foo"), List(Type.Placeholder(Type.Bounds(None, None))))

scala> Type.Apply(Type.Name("Foo"), List(Type.Placeholder(Type.Bounds(None, None)))).syntax
res3: String = Foo[?] // Still resolves the source encoding for _ => ? from the default Dialect

I am not sure currently how to properly fix it, we could have and additional dialect Scala212Pre16 maybe?

I think in the long run, the restriction that Dialect must be a static instance as a member of the scala.meta.dialects package object should be lifted -- if it's on me to ensure that an implicit Dialect with the configuration that I'd like is available wherever I call .syntax, I'm willing to take that on.

You've got all these beautiful copy methods that expose a remarkable level of configurability, but they're not available to scalameta consumers. I could see a justification in this, being that by only exposing a select number of Dialect's, it prevents additional burden to maintain scalameta itself, as there may be undefined behaviour arising from unexpected combinations of those parameters, though perhaps a middleground can be reached where only a select number of those overrides are exposed as user-configurable?

I guess that gets us back to your suggestion of just exposing yet another dialect, your Scala212Pre16. The bummer here I guess is that since that 2.12.16+ behaviour was merged and released before 2.12.16 was released, basically 2.12 prettyprinting is completely broken until consumers update.

@blast-hardcheese
Copy link
Contributor Author

Alright. I've created three PRs, all of which I intend to be merged (the two that mention this ticket are intended to be merged and released ASAP to resolve the breakage, the third should be released once 2.12.16 and 2.13.9 are available for consumption).

@blast-hardcheese
Copy link
Contributor Author

Thank you again and perpetually for scalameta, it is an absolute joy to use. I would not have half as much fun metaprogramming without such an excellent library 🙂

@kitbellew
Copy link
Contributor

I think in the long run, the restriction that Dialect must be a static instance as a member of the scala.meta.dialects package object should be lifted -- if it's on me to ensure that an implicit Dialect with the configuration that I'd like is available wherever I call .syntax, I'm willing to take that on.

The failure is not in .syntax, it's in q"..." which ends up invoking a scalameta parser at compile time. If you wish to avoid this failure, just instantiate the scalameta trees directly -- or invoke the parser explicitly, without quasiquotes.

for instance,

// q"Foo[_]
scala.meta.parsers.Parse.parseStat(scala.meta.inputs.Input.String("Foo[_]", dialect)

@tgodzik
Copy link
Collaborator

tgodzik commented May 23, 2022

Also, maybe q"Foo[_]".syntax can be replaced by q"Foo[_]".toString ? Which should work correctly.

tgodzik added a commit to tgodzik/scalameta that referenced this issue May 23, 2022
Previously, we would always print `?` for the placeholder even if the Scala version wouldn't allow it. Now, we only print `?` if it was explicitely used in the code.

Fixes scalameta#2751

The other solution would be introducing another dialect, but that would require a lot of people having to also change that, so it would require a lot more effort in the long run.
tgodzik added a commit to tgodzik/scalameta that referenced this issue May 23, 2022
Previously, we would always print `?` for the placeholder even if the Scala version wouldn't allow it. Now, we only print `?` if it was explicitely used in the code.

Fixes scalameta#2751

The other solution would be introducing another dialect, but that would require a lot of people having to also change that, so it would require a lot more effort in the long run.
tgodzik added a commit to tgodzik/scalameta that referenced this issue May 23, 2022
Previously, we would always print `?` for the placeholder even if the Scala version wouldn't allow it. Now, we only print `?` if it was explicitely used in the code.

Fixes scalameta#2751

The other solution would be introducing another dialect, but that would require a lot of people having to also change that, so it would require a lot more effort in the long run.
tgodzik added a commit to tgodzik/scalameta that referenced this issue May 23, 2022
Previously, we would always print `?` for the placeholder even if the Scala version wouldn't allow it. Now, we only print `?` if it was explicitely used in the code.

Fixes scalameta#2751

The other solution would be introducing another dialect, but that would require a lot of people having to also change that, so it would require a lot more effort in the long run.
tgodzik added a commit to tgodzik/scalameta that referenced this issue May 23, 2022
Previously, we would always print `?` for the placeholder even if the Scala version wouldn't allow it. Now, we only print `?` if it was explicitely used in the code.

Fixes scalameta#2751

The other solution would be introducing another dialect, but that would require a lot of people having to also change that, so it would require a lot more effort in the long run.
tgodzik added a commit to tgodzik/scalameta that referenced this issue May 23, 2022
Previously, we would always print `?` for the placeholder even if the Scala version wouldn't allow it. Now, we only print `?` if it was explicitely used in the code.

Fixes scalameta#2751

The other solution would be introducing another dialect, but that would require a lot of people having to also change that, so it would require a lot more effort in the long run.
tgodzik added a commit to tgodzik/scalameta that referenced this issue May 23, 2022
Previously, we would always print `?` for the placeholder even if the Scala version wouldn't allow it. Now, we only print `?` if it was explicitely used in the code.

Fixes scalameta#2751

The other solution would be introducing another dialect, but that would require a lot of people having to also change that, so it would require a lot more effort in the long run.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment