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

Add deprecation reporting mechanism for using directives #2622

Merged
merged 4 commits into from
Dec 21, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 3 additions & 0 deletions build.sc
Original file line number Diff line number Diff line change
Expand Up @@ -422,7 +422,10 @@ trait Core extends ScalaCliSbtModule with ScalaCliPublishModule with HasTests
| def toolkitOrganization = "${Deps.toolkit.dep.module.organization.value}"
| def toolkitName = "${Deps.toolkit.dep.module.name.value}"
| def toolkitTestName = "${Deps.toolkitTest.dep.module.name.value}"
| def toolkitDefaultVersion = "${Deps.toolkitVersion}"
|
| def typelevelOrganization = "${Deps.typelevelToolkit.dep.module.organization.value}"
| def typelevelToolkitDefaultVersion = "${Deps.typelevelToolkitVersion}"
|
| def defaultScalaVersion = "${Scala.defaultUser}"
| def defaultScala212Version = "${Scala.scala212}"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -123,4 +123,14 @@ object WarningMessages {

val mainScriptNameClashesWithAppWrapper =
"Script file named 'main.sc' detected, keep in mind that accessing it from other scripts is impossible due to a clash of `main` symbols"

def deprecatedWarning(old: String, `new`: String) =
s"Using '$old' is deprecated, use '${`new`}' instead"

def deprecatedToolkitLatest(updatedValue: String = "") =
if updatedValue.isEmpty then
"""Using 'latest' for toolkit is deprecated, use 'default' to get more stable behaviour"""
else
s"""Using 'latest' for toolkit is deprecated, use 'default' to get more stable behaviour:
| $updatedValue""".stripMargin
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
package scala.build.preprocessing

import scala.build.Logger
import scala.build.errors.Diagnostic.TextEdit
import scala.build.internal.Constants
import scala.build.internal.util.WarningMessages.{deprecatedToolkitLatest, deprecatedWarning}
import scala.build.preprocessing.directives.{
DirectiveHandler,
DirectiveUtil,
StrictDirective,
Toolkit
}
import scala.build.warnings.DeprecatedWarning

object DeprecatedDirectives {

/** Used to represent a general form of a deprecated directive, and its replacement
* @param keys
* representation of deprecated keys
* @param values
* representation of deprecated value
*/
case class DirectiveTemplate(keys: Seq[String], values: Option[Seq[String]]) {
def appliesTo(foundKey: String, foundValues: Seq[String]): Boolean =
(keys.isEmpty || keys.contains(foundKey)) &&
// FIXME values.contains is not perfect, but is enough for now since we don't look for specific multiple values
(values.isEmpty || values.contains(foundValues))
}

type WarningAndReplacement = (String, DirectiveTemplate)

private def keyReplacement(replacement: String)(warning: String): WarningAndReplacement =
(warning, DirectiveTemplate(Seq(replacement), None))

private def valueReplacement(replacements: String*)(warning: String): WarningAndReplacement =
(warning, DirectiveTemplate(Nil, Some(replacements.toSeq)))

private def allAliasesOf(key: String, handler: DirectiveHandler[_]): Seq[String] =
handler.keys.find(_.nameAliases.contains(key))
.toSeq
.flatMap(_.nameAliases)

private def allKeysFrom(handler: DirectiveHandler[_]): Seq[String] =
handler.keys.flatMap(_.nameAliases)

private val deprecatedCombinationsAndReplacements = Map[DirectiveTemplate, WarningAndReplacement](
DirectiveTemplate(Seq("lib"), None) -> keyReplacement("dep")(deprecatedWarning("lib", "dep")),
DirectiveTemplate(Seq("libs"), None) -> keyReplacement("dep")(deprecatedWarning(
"libs",
"deps"
)),
DirectiveTemplate(Seq("compileOnly.lib"), None) -> keyReplacement("compileOnly.dep")(
deprecatedWarning("compileOnly.lib", "compileOnly.dep")
),
DirectiveTemplate(Seq("compileOnly.libs"), None) -> keyReplacement("compileOnly.dep")(
deprecatedWarning("compileOnly.libs", "compileOnly.deps")
),
DirectiveTemplate(
allKeysFrom(directives.Toolkit.handler),
Some(Seq("latest"))
) -> valueReplacement("default")(deprecatedToolkitLatest()),
DirectiveTemplate(
allKeysFrom(directives.Toolkit.handler),
Some(Seq(s"${Toolkit.typelevel}:latest"))
) -> valueReplacement(s"${Toolkit.typelevel}:default")(
deprecatedToolkitLatest()
),
DirectiveTemplate(
allKeysFrom(directives.Toolkit.handler),
Some(Seq(s"${Constants.typelevelOrganization}:latest"))
) -> valueReplacement(s"${Toolkit.typelevel}:default")(
deprecatedToolkitLatest()
)
)

def warningAndReplacement(directive: StrictDirective): Option[WarningAndReplacement] =
deprecatedCombinationsAndReplacements
.find(_._1.appliesTo(directive.key, directive.toStringValues))
.map(_._2) // grab WarningAndReplacement

def issueWarnings(
path: Either[String, os.Path],
directives: Seq[StrictDirective],
logger: Logger
) =
directives.map(d => d -> warningAndReplacement(d))
.foreach {
case (directive, Some(warning, replacement)) =>
val newKey = replacement.keys.headOption.getOrElse(directive.key)
val newValues = replacement.values.getOrElse(directive.toStringValues)
val newText = s"$newKey ${newValues.mkString(" ")}"

// TODO use key and/or value positions instead of whole directive
val position = directive.position(path)

val diagnostic = DeprecatedWarning(
warning,
Seq(position),
Some(TextEdit(s"Change to: $newText", newText))
)
logger.log(Seq(diagnostic))
case _ => ()
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -123,8 +123,7 @@ case class DirectivesPreprocessor(
if !allowRestrictedFeatures && (handler.isRestricted || handler.isExperimental) then
Left(DirectiveErrors(
::(WarningMessages.powerDirectiveUsedInSip(scopedDirective, handler), Nil),
// TODO: use positions from ExtractedDirectives to get the full directive underlined
DirectiveUtil.positions(scopedDirective.directive.values, path)
Seq(scopedDirective.directive.position(scopedDirective.maybePath))
))
else
if handler.isExperimental && !shouldSuppressExperimentalFeatures then
Expand All @@ -142,7 +141,7 @@ case class DirectivesPreprocessor(
val res = directives
.iterator
.flatMap {
case d @ StrictDirective(k, _) =>
case d @ StrictDirective(k, _, _) =>
handlersMap.get(k).iterator.map(_(ScopedDirective(d, path, cwd), logger))
}
.toVector
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,14 @@
package scala.build.preprocessing

import com.virtuslab.using_directives.UsingDirectivesProcessor
import com.virtuslab.using_directives.custom.model.UsingDirectives
import com.virtuslab.using_directives.custom.utils.ast.{UsingDef, UsingDefs}
import com.virtuslab.using_directives.custom.model.{
BooleanValue,
EmptyValue,
StringValue,
UsingDirectives,
Value
}
import com.virtuslab.using_directives.custom.utils.ast._

import scala.annotation.targetName
import scala.build.errors.*
Expand All @@ -14,20 +20,17 @@ import scala.jdk.CollectionConverters.*

case class ExtractedDirectives(
directives: Seq[StrictDirective],
positions: Option[Position.File]
position: Option[Position.File]
) {
@targetName("append")
def ++(other: ExtractedDirectives): ExtractedDirectives =
ExtractedDirectives(directives ++ other.directives, positions)
ExtractedDirectives(directives ++ other.directives, position)
}

object ExtractedDirectives {

def empty: ExtractedDirectives = ExtractedDirectives(Seq.empty, None)

val changeToSpecialCommentMsg =
"Using directive using plain comments are deprecated. Please use a special comment syntax: '//> ...'"

def from(
contentChars: Array[Char],
path: Either[String, os.Path],
Expand Down Expand Up @@ -60,14 +63,26 @@ object ExtractedDirectives {
case None => None
}

val flattened = directivesOpt.map(_.getFlattenedMap.asScala.toSeq).getOrElse(Seq.empty)
val strictDirectives =
flattened.map {
case (k, l) =>
StrictDirective(k.getPath.asScala.mkString("."), l.asScala.toSeq)
val strictDirectives = directivesOpt.toSeq.flatMap { directives =>
def toStrictValue(value: UsingValue): Seq[Value[_]] = value match {
case uvs: UsingValues => uvs.values.asScala.toSeq.flatMap(toStrictValue)
case el: EmptyLiteral => Seq(EmptyValue(el))
case sl: StringLiteral => Seq(StringValue(sl.getValue(), sl))
case bl: BooleanLiteral => Seq(BooleanValue(bl.getValue(), bl))
}
def toStrictDirective(ud: UsingDef) = StrictDirective(
ud.getKey(),
toStrictValue(ud.getValue()),
ud.getPosition().getColumn()
)

directives.getAst match
case uds: UsingDefs => uds.getUsingDefs.asScala.toSeq.map(toStrictDirective)
case ud: UsingDef => Seq(toStrictDirective(ud))
case _ => Nil // There should be nothing else here other than UsingDefs or UsingDef
}

Right(ExtractedDirectives(strictDirectives, directivesPositionOpt))
Right(ExtractedDirectives(strictDirectives.reverse, directivesPositionOpt))
Gedochao marked this conversation as resolved.
Show resolved Hide resolved
}
else
maybeCompositeMalformedDirectiveError match {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,13 @@ import scala.build.errors.*
import scala.build.input.{Inputs, ScalaCliInvokeData, ScalaFile, SingleElement, VirtualScalaFile}
import scala.build.internal.Util
import scala.build.options.*
import scala.build.preprocessing.directives
import scala.build.preprocessing.directives.{
DirectiveHandler,
DirectiveUtil,
PreprocessedDirectives,
ScopedDirective
}
import scala.build.preprocessing.{DeprecatedDirectives, directives}
import scala.build.{Logger, Position, Positioned}

case object ScalaPreprocessor extends Preprocessor {
Expand Down Expand Up @@ -219,6 +219,8 @@ case object ScalaPreprocessor extends Preprocessor {
suppressWarningOptions: SuppressWarningOptions,
maybeRecoverOnError: BuildException => Option[BuildException]
)(using ScalaCliInvokeData): Either[BuildException, Option[ProcessingOutput]] = either {
DeprecatedDirectives.issueWarnings(path, extractedDirectives.directives, logger)

val (content0, isSheBang) = SheBang.ignoreSheBangLines(content)
val preprocessedDirectives: PreprocessedDirectives =
value(DirectivesPreprocessor(
Expand Down
24 changes: 24 additions & 0 deletions modules/build/src/test/scala/scala/build/tests/BuildTests.scala
Original file line number Diff line number Diff line change
Expand Up @@ -951,4 +951,28 @@ abstract class BuildTests(server: Boolean) extends TestUtil.ScalaCliBuildSuite {
expect(maybeBuild.exists(_.success))
}
}

for (dirValue <- Seq("default", "typelevel:default"))
test(s"error when toolkit $dirValue is used with Scala 2.12") {
val testInputs = TestInputs(
os.rel / "simple.sc" ->
s"""//> using toolkit $dirValue
|
|val n = 2
|println(s"n=$$n")
|""".stripMargin
)

val scala212Options = baseOptions.copy(
scalaOptions = baseOptions.scalaOptions.copy(
scalaVersion = Some(MaybeScalaVersion(Constants.defaultScala212Version)),
scalaBinaryVersion = None
),
scriptOptions = ScriptOptions(Some(true))
)

testInputs.withBuild(scala212Options, buildThreads, bloopConfigOpt) { (_, _, maybeBuild) =>
expect(maybeBuild.left.exists(_.message.startsWith("Toolkits do not support Scala 2.12")))
}
}
}
10 changes: 5 additions & 5 deletions modules/cli/src/main/scala/scala/cli/commands/fix/Fix.scala
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ object Fix extends ScalaCommand[FixOptions] {
createFormattedLinesAndAppend(allMainDirectives, projectFileContents, isTest = false)

(
transformedMainDirectives.filter(d => isExtractedFromWritableInput(d.positions)),
transformedMainDirectives.filter(d => isExtractedFromWritableInput(d.position)),
testScopeDirectives
)
}
Expand Down Expand Up @@ -111,8 +111,8 @@ object Fix extends ScalaCommand[FixOptions] {

// Remove directives from their original files, skip the project.scala file
directivesFromWritableMainInputs
.filterNot(e => isProjectFile(e.positions))
.foreach(d => removeDirectivesFrom(d.positions))
.filterNot(e => isProjectFile(e.position))
.foreach(d => removeDirectivesFrom(d.position))
directivesFromWritableTestInputs
.filterNot(ttd => isProjectFile(ttd.positions))
.foreach(ttd => removeDirectivesFrom(ttd.positions, toKeep = ttd.noTestPrefixAvailable))
Expand Down Expand Up @@ -233,13 +233,13 @@ object Fix extends ScalaCommand[FixOptions] {
}

val transformedToTestEquivalents = withTestEquivalent.map {
case StrictDirective(key, values) => StrictDirective("test." + key, values)
case StrictDirective(key, values, _) => StrictDirective("test." + key, values)
}

TransformedTestDirectives(
withTestPrefix = transformedToTestEquivalents,
noTestPrefixAvailable = noTestEquivalent,
positions = extractedFromSingleElement.positions
positions = extractedFromSingleElement.position
)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -210,8 +210,8 @@ final case class SharedOptions(
@Tag(tags.must)
compilationOutput: Option[String] = None,
@Group(HelpGroup.Scala.toString)
@HelpMessage("Add toolkit to classPath")
@ValueDescription("version|latest")
@HelpMessage(s"Add toolkit to classPath (not supported in Scala 2.12), 'default' version for Scala toolkit: ${Constants.toolkitDefaultVersion}, 'default' version for typelevel toolkit: ${Constants.typelevelToolkitDefaultVersion}")
@ValueDescription("version|default")
@Name("toolkit")
@Tag(tags.implementation)
@Tag(tags.inShortHelp)
Expand Down Expand Up @@ -393,13 +393,13 @@ final case class SharedOptions(
SharedOptions.parseDependencies(
dependencies.dependency.map(Positioned.none),
ignoreErrors
) ++ SharedOptions.resolveToolkitDependency(withToolkit)
) ++ SharedOptions.resolveToolkitDependency(withToolkit, logger)
),
extraCompileOnlyDependencies = ShadowingSeq.from(
SharedOptions.parseDependencies(
dependencies.compileOnlyDependency.map(Positioned.none),
ignoreErrors
) ++ SharedOptions.resolveToolkitDependency(withToolkit)
) ++ SharedOptions.resolveToolkitDependency(withToolkit, logger)
)
),
internal = bo.InternalOptions(
Expand Down Expand Up @@ -719,8 +719,21 @@ object SharedOptions {
}
}

private def resolveToolkitDependency(toolkitVersion: Option[String])
: Seq[Positioned[AnyDependency]] =
private def resolveToolkitDependency(
toolkitVersion: Option[String],
logger: Logger
): Seq[Positioned[AnyDependency]] = {
if (
toolkitVersion.contains("latest")
|| toolkitVersion.contains(Toolkit.typelevel + ":latest")
|| toolkitVersion.contains(Constants.typelevelOrganization + ":latest")
) logger.message(
WarningMessages.deprecatedToolkitLatest(
s"--toolkit ${toolkitVersion.map(_.replace("latest", "default")).getOrElse("default")}"
)
)

toolkitVersion.toList.map(Positioned.commandLine)
.flatMap(Toolkit.resolveDependenciesWithRequirements(_).map(_.value))
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
package scala.build.errors

import scala.build.Position

final class ToolkitVersionError(msg: String, positions: Seq[Position])
extends BuildException(msg, positions)
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
package scala.build.warnings

import scala.build.Position
import scala.build.errors.Diagnostic.TextEdit
import scala.build.errors.{BuildException, Diagnostic, Severity}

final case class DeprecatedWarning(
message: String,
positions: Seq[Position],
override val textEdit: Option[TextEdit]
) extends Diagnostic {
def severity: Severity = Severity.Warning
}
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,8 @@ final case class Dependency(
@DirectiveName("test.deps")
@DirectiveName("test.dependencies")
testDependency: List[Positioned[String]] = Nil,
@DirectiveName("compileOnly.lib")
@DirectiveName("compileOnly.libs")
@DirectiveName("compileOnly.lib") // backwards compat
@DirectiveName("compileOnly.libs") // backwards compat
@DirectiveName("compileOnly.dep")
@DirectiveName("compileOnly.deps")
@DirectiveName("compileOnly.dependencies")
Expand Down
Loading