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

Adding -source:future causes compilation errors due to deprecated wildcard syntax #496

Closed
wvandermerwe opened this issue Oct 6, 2022 · 13 comments · Fixed by #736
Closed
Labels
good first issue Good for newcomers

Comments

@wvandermerwe
Copy link

On Scala 3.2.0, adding the "-source:future" compiler option causes compilation to fail with the following error message:
_ is deprecated for wildcard arguments of types: use ? instead

@wvandermerwe wvandermerwe changed the title Adding -source:future causes compilation errors due to deprecated wildcard syntax. Adding -source:future causes compilation errors due to deprecated wildcard syntax Oct 6, 2022
@Baccata
Copy link
Contributor

Baccata commented Oct 6, 2022

As a workaround, you could probably use a multi-module build and code-generate in a separate module, so that your application code could still use -source:future.

We'll probably provide a pathway to opt-in ? syntax eventually, but truth be told, it's gonna be pretty low on the priority list. If you want to contribute, we could possibly offer some guidance.

@kubukoz
Copy link
Member

kubukoz commented Oct 6, 2022

You can disable warnings in generated sources. Might be a good idea because at some point smithy4s might start generating @deprecated annotations on methods/types that have the @deprecated trait, for example.

I'm using something like this in one of the projects:

scalacOptions += "-Wconf:src=src_managed/.*:silent"

edit: unless this is actually an error and not a warning + -Xfatal-warnings 😅

@wvandermerwe
Copy link
Author

@Baccata I would love to contribute (smithy4s is awesome!) but would definitely need some guidance as I don't have any OSS experience.

@kubukoz
Copy link
Member

kubukoz commented Nov 12, 2022

The biggest problem right now would probably be to figure out how the users would configure this... but as long as you can parameterize the renderer with something, we can probably figure that out later - you could start a PR that hardcodes the renderer to use the Scala 3 syntax, and we could pick it up later to pass that all the way from whatever is going to set that. I'm thinking codegen-cli / sbt-plugin / mill-plugin settings.

That being unclear, here are some things that are clear atm and should be useful if you want to give this a shot:


1. Where's the code

In general, you're gonna work in the codegen module.

Generating code roughly starts in the Codegen type (you shouldn't have to deal with it much). It runs in basically two phases:

  • Smithy model -> IR (intermediate representation), called SmithyToIR and
  • IR -> String, called Renderer.

The exact look of the generated code is only a concern of the latter, which lives in object Renderer (codegen module).

You can look around and draw some inspiration from #599 which touches a couple areas in the renderer.

2. How to check that it works

The testing ability is quite limited as of now, as testing is mostly done by either:

  • we have some sources in modules/example/src that are generated (from sources in sampleSpecs/*.smithy) and checked-in. The CI setup checks if these are up-to-date after everything has been generated, compiled and tested
  • sbt/mill plugin tests, which don't look very closely at the exact contents of the generated files, but they validate that the codegen happened and that the generated sources are usable in handwritten code.

I'd say your best feedback loop will be this:

  1. Run sbt compile at least once on a fresh clone of series/0.17 and import the project in your editor
  2. Make changes in the renderer
  3. Run the sbt task: example/managedSources
  4. Look at how your changes affected the files in modules/example/src
  5. Go to 2 and repeat

I would suggest that you start with small changes (e.g. adding some whitespace, or just random stuff really).

3. Other development notes

Make sure you're branching off from series/0.17, as that's where current development takes place. Tthere's been a good chunk of changes in the codegen in there, so to avoid conflicts it's best not to do too much on main (which is 0.16.x).

For this feature, I think we're gonna want to add some sort of RenderingConfig parameter to the Renderer class, and that will contain members like:

def wildcardArgument: String
def wildcardImport: String
//etc

the provider of the config would specify the correct symbols to use. We would use these members wherever we currently use plain symbols.

@scala-impala sorry for the long wait, but I also didn't know that much about the renderer to provide any useful hints before :)

@Baccata
Copy link
Contributor

Baccata commented Nov 12, 2022

The biggest problem right now would probably be to figure out how the users would configure this

That is a very accurate hunch over why this is tricky. Ideally you wouldn't have to configure anything and the plugin would derive it directly from the fact that the project is using the Scala 3 compiler and has a particular flag set-up, but that's involved.

What we've been staring to do is use "smithy4s"-prefixed smithy metadata for this kind of stuff. It might be where the configuration should live, for the time being anyway.

@kubukoz
Copy link
Member

kubukoz commented Nov 12, 2022

the fact that the project is using the Scala 3 compiler

I thought about this... are we guaranteed to get separate generated sources for each Scala version in the build matrix?

What we've been staring to do is use "smithy4s"-prefixed smithy metadata for this kind of stuff. It might be where the configuration should live, for the time being anyway.

I'm not sure this is very user-friendly, though. You might be consuming a library containing Smithy specs and not want to set up any Smithy files for this. Also, I take it that metadata would only have effect on the namespace it's contained in?

@Baccata
Copy link
Contributor

Baccata commented Nov 13, 2022

Also, I take it that metadata would only have effect on the namespace it's contained in?

Nope, metadata is a global, somewhat-semigroupal value.

You might be consuming a library containing Smithy specs and not want to set up any Smithy files for this.

Way ahead of you

@Baccata
Copy link
Contributor

Baccata commented Nov 14, 2022

You might be consuming a library containing Smithy specs and not want to set up any Smithy files for this.

Wow, totally misread/mis-responded. Right. So I think I'd be adversed to having options related to rendering live outside of smithy files. Providing other means of configuration would have the effect of increasing maintenance burden, and would lose the effect of teaching our users about the smithy language and what it allows for.

For instance, you can control some aspects on the rendering by applying some traits to shapes a-posteriori. It teaches the user that traits can be applied via the apply keyword, which is not something that is tremendously clear in the files. Similarly, forcing users to use metadata to configure rendering has a nice effect of teaching them that metadata are a thing and may be used for other things.

At the end of the day, the configuration has to happen somewhere. It makes sense to me to use smithy files for this, because it works in all contexts (build-tools / clis) without having to multiply the same code in places.

@kubukoz
Copy link
Member

kubukoz commented Nov 14, 2022

That last point convinces me.

@wvandermerwe
Copy link
Author

Thanks @kubukoz @Baccata I'll give this a shot over the next few weekends

@kubukoz
Copy link
Member

kubukoz commented Jan 20, 2023

wait, we should also do this for imports, right?

@Baccata
Copy link
Contributor

Baccata commented Jan 20, 2023

what do you mean imports ?

@kubukoz
Copy link
Member

kubukoz commented Jan 20, 2023

scala 2

import foo.bar._

scala 3

import foo.bar.*

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants