Skip to content

Scalac preset option in directive #3615

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.

Sign up for GitHub

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

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

Scalac preset option in directive #3615

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
5ae4b9c
Use scalac preset options in directive
yadavan88 Apr 3, 2025
3eeddf8
Merge branch 'main' into scalac-preset-option
yadavan88 Apr 4, 2025
d61c509
enums for preset options
yadavan88 Apr 4, 2025
fcf4301
Merge branch 'main' into scalac-preset-option
yadavan88 Apr 7, 2025
06f5cd7
resolve scalac options based on scala version and preset
yadavan88 Apr 10, 2025
ec393ed
fixed compilation errors
yadavan88 Apr 10, 2025
c8fea6e
updated buildOPtions with preset
yadavan88 Apr 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions DEV.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,8 @@ the unit tests:
To can filter unit test suites:

```bash
./mill 'build-module.test' 'scala.build.tests.BuildTestsScalac.*'
./mill 'build-module.test' 'scala.build.tests.BuildTestsScalac.simple'
./mill 'build-module[].test' 'scala.build.tests.BuildTestsScalac.*'
./mill 'build-module[].test' 'scala.build.tests.BuildTestsScalac.simple'
```

#### Run integration tests with the JVM launcher
Expand Down
19 changes: 19 additions & 0 deletions modules/build/src/test/scala/scala/build/tests/BuildTests.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -778,6 +778,25 @@ abstract class BuildTests(server: Boolean) extends TestUtil.ScalaCliBuildSuite {
}
}

test("use scalac preset options") {
val inputs = TestInputs(
os.rel / "foo.scala" ->
"""//> using scala 2.13
|//> using options.preset suggested
|
|def foo = "bar"
|""".stripMargin
)

inputs.withBuild(defaultOptions, buildThreads, bloopConfigOpt) { (_, _, maybeBuild) =>
val expectedOptions =
Seq("-Xfatal-warnings", "-deprecation", "-unchecked")
val scalacOptions =
maybeBuild.toOption.get.options.scalaOptions.scalacOptions.toSeq.map(_.value.value)
expect(scalacOptions == expectedOptions)
}
}

test("multiple times scalac options with -Xplugin prefix") {
val inputs = TestInputs(
os.rel / "foo.scala" ->
Expand Down
47 changes: 41 additions & 6 deletions modules/directives/src/main/scala/scala/build/preprocessing/directives/ScalacOptions.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ package scala.build.preprocessing.directives

import scala.build.Positioned
import scala.build.directives.*
import scala.build.errors.BuildException
import scala.build.errors.{BuildException, DirectiveErrors, InputsException}
import scala.build.options.WithBuildRequirements.*
import scala.build.options.{
BuildOptions,
Expand Down Expand Up @@ -31,6 +31,9 @@ import scala.cli.commands.SpecificationLevel
|`//> using test.scalacOptions` _option1_ _option2_ …
|`//> using test.options` _option1_ _option2_ …
|
|`//> using options` _option1_ _option2_ …
|
|`//> using options.preset` _suggested_ | _ci_ | _strict_
|""".stripMargin
)
@DirectiveDescription("Add Scala compiler options")
Expand All @@ -44,12 +47,39 @@ final case class ScalacOptions(
@DirectiveName("test.options")
@DirectiveName("test.scalacOption")
@DirectiveName("test.scalacOptions")
testOptions: List[Positioned[String]] = Nil
testOptions: List[Positioned[String]] = Nil,
@DirectiveName("option.preset")
@DirectiveName("options.preset")
presetOptions: Option[String] = None
) extends HasBuildOptionsWithRequirements {
def buildOptionsList: List[Either[BuildException, WithBuildRequirements[BuildOptions]]] = List(
ScalacOptions.buildOptions(options).map(_.withEmptyRequirements),
ScalacOptions.buildOptions(testOptions).map(_.withScopeRequirement(Scope.Test))
)
def buildOptionsList: List[Either[BuildException, WithBuildRequirements[BuildOptions]]] = {
val explicitScalacOptions = List(
ScalacOptions.buildOptions(options).map(_.withEmptyRequirements),
ScalacOptions.buildOptions(testOptions).map(_.withScopeRequirement(Scope.Test))
)

presetOptions match {
case None => explicitScalacOptions
case Some("suggested") =>
val presetOptions =
ScalacOptions.buildOptions(ScalacOptions.presetOptionsSuggested.map(Positioned.none))
explicitScalacOptions :+ presetOptions.map(_.withEmptyRequirements)
case Some("ci") =>
val presetOptions =
ScalacOptions.buildOptions(ScalacOptions.presetOptionsCI.map(Positioned.none))
explicitScalacOptions :+ presetOptions.map(_.withEmptyRequirements)
case Some("strict") =>
val presetOptions =
ScalacOptions.buildOptions(ScalacOptions.presetOptionsStrict.map(Positioned.none))
explicitScalacOptions :+ presetOptions.map(_.withEmptyRequirements)
case Some(other) =>
List(Left(
InputsException(
s"Unknown preset options: $other. Available options are: suggested, ci, strict"
Copy link
Contributor

Choose a reason for hiding this comment

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

We should have an enumeration of the available presets somewhere, with tied option lists.
then we should just call the full list here, rather than hardcoding it like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, where should this be defined. I was only making a quick draft just to see if I understood the requirement correctly. The issue seems to be really interesting as I tend to add some options manually and I never remember what should be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added these options as enums

)
))
}
}
}

object ScalacOptions {
Expand All @@ -62,4 +92,9 @@ object ScalacOptions {
)
)
}

// todo: this should be based on the scala version. This might not be the correct location
Copy link
Contributor

Choose a reason for hiding this comment

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

yup... might be annoying to keep up-to-date as well.
I actually wonder if the best place for defining the presets wouldn't be the compiler itself... but that could be done as an improvement.
just theorising, since this is an early draft.

Copy link
Contributor

Choose a reason for hiding this comment

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

the 3 presets for a start are good I think (having suggested, ci and strict; still need to establish what would each of them entail)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I just wanted to add some ideas, I am also not fully clear what should they be, but added there just to make sure that we should handle multiple presets.

Copy link
Contributor

Choose a reason for hiding this comment

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

Regarding what they should be, I believe the idea was to start with something like what https://github.com/typelevel/sbt-tpolecat has...

We could also start a discussion for suggestions somewhere... A thread on https://contributors.scala-lang.org/, maybe... or just a discussion on this repo?
Either way, if/when suggestions on which options should be included come, we could tweak it gradually.
The feature should first come out as experimental, anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about the duplicates yday, but I saw some test related to duplicate scalaC args are ignored or something like that. SO thought it can be ignored. I will try to handle them. How about getting the scalaVersion? We should have the sugested and other presets based on the scala versions right. I assumes optsForScala212, optsForScala213, optsForScala3 which supports all versions within 2.13, 2.12 and 3.
Can you point me how to get tht scala version within this scalacOptions code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the command line args needs to be processed, then is it not possible to add the scalac options in the ScalacOptions.scala file right? I mean, tif the preset is also passed as command line option, we need to keep the preset value till the last moment and then decide priority (command line over directive). So, the preset value should be somehow carried over and not the flattened scalac options). Am I right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Gedochao Could you have a look at it when you get time?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you point me how to get tht scala version within this scalacOptions code?

You can't. We don't know what's the final Scala version until using directives are parsed.

If the command line args needs to be processed, then is it not possible to add the scalac options in the ScalacOptions.scala file right? I mean, tif the preset is also passed as command line option, we need to keep the preset value till the last moment and then decide priority (command line over directive). So, the preset value should be somehow carried over and not the flattened scalac options). Am I right?

Yep, the presets can only be applied once the Scala version is finalised, which generally should be after (or within) scala.build.CrossSources.forInputs.

I thought about the duplicates yday, but I saw some test related to duplicate scalaC args are ignored or something like that.

We do have handling for when there's duplicates introduced by the command line vs directives. If duplicates are explicitly added by the user in one context, we don't remove them and let the compiler handle it.
However, for when a preset has a duplicate with whatever explicit options the user passed, you need to make sure it's handled correctly. Just add a test for this case.

def presetOptionsSuggested = List("-Xfatal-warnings", "-deprecation", "-unchecked")
def presetOptionsCI = List("-Xfatal-warnings", "-deprecation", "-unchecked")
def presetOptionsStrict = List("-Xfatal-warnings", "-deprecation", "-unchecked")
}
Loading