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

Remove default import of scala.Predef._ + new option ImportScalaPredef to reintroduce it. #180

Merged
merged 1 commit into from
Feb 16, 2022

Conversation

martingd
Copy link
Contributor

@martingd martingd commented Feb 14, 2022

Fixes #171

The sbt-buildinfo plugin generates imports of scala.Predef._ and scala.Any that are not necessary unless you use compiler option -Yno-imports which the vast majority of projects don't. Furthermore, the first import:

import scala.Predef._

fails compilation with the Scala 3 compiler option -source:future because the _ should be replaced by a * in Scala 3 syntax.

I see (at least) these three options for fixing the issue:

  1. Change the plugin to generate sources for both Scala 2 and 3 with different import for this import.
  2. Make a new option to make the plugin use * instead of _ for the generated import.
  3. Make a new option to make the plugin skip generating these imports altogether.

In this PR, I chose to go for 3) for the reasons described above: few projects use -Yno-imports and probably even fewer that also use -source:future. For backwards compatibility, the new option SkipImports prevents generation of the imports. Without using this new option, everything is as before.

@martingd
Copy link
Contributor Author

fixes #171

@martingd
Copy link
Contributor Author

Hi @eed3si9n
I would appreaciate if you could take a look at this PR. Thank you.

Copy link
Member

@eed3si9n eed3si9n left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome. I wonder if we should flip the default and make it generate import scala.Predef under some options?

@martingd
Copy link
Contributor Author

martingd commented Feb 15, 2022

I had the same thought (that the default should be not to generate these imports). However, I was afraid to break somebody's build when they upgraded the sbt-buildinfo plugin.

But yes, instead of what I have done now, it could be:

  • Don't generate import scala.Predef._ (and import scala.Any in the ScalaCaseClassRenderer) by default.
  • If setting option BuildInfoOption.ExplicitScalaImports it will put these import into the generated code.

Let me know, if you think that it is better to flip it around.
And in that case, what should the name of the option then be? ExplicitScalaImports?

@eed3si9n
Copy link
Member

If something fails at build-time, then I think it's ok (as opposed to runtime).
It's already failing someone's build so it's really a matter of which trolley victim group would be larger, and my guess is that very small % of the ppl use -Yno-imports.

Let me know, if you think that it is better to flip it around.

So yeah. Let's do it.

And in that case, what should the name of the option then be? ExplicitScalaImports?

ImportScalaPredef?

@martingd
Copy link
Contributor Author

Ok, I'll update the pull request tomorrow.

Regarding the name of the option: ScalaCaseClassRenderer generates these imports:

import scala.Predef._
import scala.Any

but I think I can get rid of the scala.Any import by fully qualifying the explicit reference to Any in the generated code.

In that case, I like your suggestion ImportScalaPredef.

@martingd
Copy link
Contributor Author

It looks like almost all types are fully qualified in the generated code so there is also the possibility that it would be possible to change the code a little so it will compile with and without -Yno-imports without the need for any import of scala.Predef._ at all. That would be the best solution. I'll see if that is possible.

@martingd
Copy link
Contributor Author

Ok, there are SO many things missing if java.lang and scala.Predef._ is not imported. So to avoid making a lot of changes and risk breaking things, I will go for what we talked about:

  • Fully qualify scala.Any, so that is not needed as an explicit import.
  • New option ImportScalaPredef that imports just scala.Predef._ for the few that wants to compile with -Yno-imports.

I will leave all the currently, fully qualified types (scala.Int, scala.Boolean, etc.) untouched to keep the changes minimal.

Added option `ImportScalaPredef` to include import of `scala.Predef`.
@martingd martingd force-pushed the feature-nopredefimport branch from 4370ebc to 965a7d0 Compare February 16, 2022 20:25
@martingd
Copy link
Contributor Author

martingd commented Feb 16, 2022

@eed3si9n I have now updated the PR. I have squashed all changes into one commit.
Summary:

  • The import scala.Predef._ is no longer generated.
  • I have removed the import of scala.Any from ScalaCaseClassRenderer and instead changed the code-generation to emit scala.Any in the only location, where it was necessary.
  • I have added the new option BuildInfoOption.ImportScalaPredef that – if present – reintroduces the import scala.Predef._ into the generated code.
  • All existing tests have been updated with option BuildInfoOption.ImportScalaPredef so they work exactly as before.
  • Two new tests have been added:
    • skipimportscaseobject : Copy of test options but without option BuildInfoOption.ImportScalaPredef and without Scala compiler option -Yno-imports in order to test code generation without the import of scala.Predef._.
    • skipimportscaseclass: Copy of test caseclassrenderer but with same changes as above to test code generation without the import of scala.Predef._.
  • Documentation has been updated
    • Description of the new option BuildInfoOption.ImportScalaPredef
    • The line import scala.Predef._ from the sample code in the beginning has been removed as this is no longer generated by default.

Let me hear if it makes sense. :)

@martingd martingd changed the title SkipImports options to leave out imports that conflicts with -source:future. Remove default import of scala.Predef._ + new option ImportScalaPredef to reintroduce it. Feb 16, 2022
Copy link
Member

@eed3si9n eed3si9n left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@martingd
Copy link
Contributor Author

@eed3si9n I can see the PR is not linked to issue #171. Not sure how to do that.

@eed3si9n eed3si9n closed this Feb 16, 2022
@eed3si9n eed3si9n reopened this Feb 16, 2022
@eed3si9n eed3si9n merged commit 47d5a0f into sbt:master Feb 16, 2022
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.

Generated import not compatible with new Scala 3 syntax
2 participants