New using directive syntax #546

romanowski · 2022-01-15T01:50:49Z

This PR changes the syntax of using directives. Following syntax is supported:

using foo bar with using keyword
//> using scala "3" or /*> using scala "#" using keyword within a comment with special indicator >

Following syntax is deprecated:

@using scala 3, // @using scala 3 - with@using global annotation
// using scala "3", /* using scala "3"*/ - within a comment within special indicator.

Deprecated syntaxes result in a warning and will be supported within the next minor release: 0.1.x and results in error within 0.2.x.

Warnings are also working within BSP.

cc @julienrf

alexarchambault

I didn't review the core of the changes yet, but I wish the git history was clearer: as of now, this mixes various unrelated changes (refactorings, new using directive syntax, changes in the build), so I'd rather not squash-and-merge, but I wouldn't want to merge with a merge commit either to avoid "reformating" or "address CI failures" commits (which don't really make sense on their own, when later bisecting issues for example).

alexarchambault · 2022-01-18T13:32:50Z

project/deps.sc

+  Seq(
+    coursier.Repositories.sonatype("snapshots"),
+    // Uncomment for local development
+    coursier.MavenRepository(s"file://${System.getProperty("user.home")}/.m2/repository")


Should be commented out?

yes, my bad

alexarchambault · 2022-01-18T13:34:16Z

scala-cli

@@ -17,7 +17,7 @@ if [ ! -x "$LAUNCHER_DIR/scala" ]; then
  echo "native-image launcher not built yet." 1>&2
  echo "In order to build it, go in $DIR, and run" 1>&2
  echo 1>&2
-  echo "  ./mill cli.nativeImage" 1>&2
+  echo "  ./mill copyTo cli.nativeImage out/cli/nativeImage/dest/scala" 1>&2


Just ./mill cli.nativeImage should be enough. We should change LAUNCHER_DIR if the native image doesn't land there.

alexarchambault · 2022-01-18T13:35:43Z

website/docs/guides/using-directives.md


 Until `using` directives becomes a part of the Scala specification, this is the only way that guarantees that your code will work well with IDEs, code formatters, and other tools.
 :::

+Within one file, only one flavor of using directives can be used. The keyword-base syntax (`using scala "3"`) has precedence over special comments (`//> using scala "3"`) and deprecated, plain comments (`// using scala "3"`) has lowest priority.


Suggested change

Within one file, only one flavor of using directives can be used. The keyword-base syntax (`using scala "3"`) has precedence over special comments (`//> using scala "3"`) and deprecated, plain comments (`// using scala "3"`) has lowest priority.

Within one file, only one flavor of using directives can be used. The keyword-based syntax (`using scala "3"`) has precedence over special comments (`//> using scala "3"`) and deprecated, plain comments (`// using scala "3"`) has lowest priority.

alexarchambault · 2022-01-18T13:36:33Z

website/docs/guides/using-directives.md


 Until `using` directives becomes a part of the Scala specification, this is the only way that guarantees that your code will work well with IDEs, code formatters, and other tools.
 :::

+Within one file, only one flavor of using directives can be used. The keyword-base syntax (`using scala "3"`) has precedence over special comments (`//> using scala "3"`) and deprecated, plain comments (`// using scala "3"`) has lowest priority.
+
+For now `using` and `@using` can be mixed within given syntax however we strongly suggest to does not use deprecated `@using`. 


?

Suggested change

For now `using` and `@using` can be mixed within given syntax however we strongly suggest to does not use deprecated `@using`.

For now `using` and `@using` can be mixed within a given syntax however we strongly suggest not to use deprecated `@using`.

alexarchambault · 2022-01-18T13:38:18Z

website/docs/guides/using-directives.md

+```
+## Deprecated syntax
+
+As a part of `0.0.x` series we experimented with different syntax for using directives. Based on the feedback and discussions with Scala compiler teams we decided to deprecated `@using` (using annotations) and `// using` (using within plain import). Those syntaxes will work as a part of of `0.1.x` series and we will result with an error since `0.2.x`.


?

Suggested change

As a part of `0.0.x` series we experimented with different syntax for using directives. Based on the feedback and discussions with Scala compiler teams we decided to deprecated `@using` (using annotations) and `// using` (using within plain import). Those syntaxes will work as a part of of `0.1.x` series and we will result with an error since `0.2.x`.

As a part of `0.0.x` series we experimented with different syntaxes for using directives. Based on feedback and discussions with the Scala compiler team, we decided to deprecate `@using` (using annotations) and `// using` (using within plain comment). Those syntaxes will keep working in the `0.1.x` series and will result in an error starting from `0.2.x`.

alexarchambault · 2022-01-18T13:38:33Z

website/docs/guides/using-directives.md

+
+As a part of `0.0.x` series we experimented with different syntax for using directives. Based on the feedback and discussions with Scala compiler teams we decided to deprecated `@using` (using annotations) and `// using` (using within plain import). Those syntaxes will work as a part of of `0.1.x` series and we will result with an error since `0.2.x`.
+
+Scala CLI produces a warnings if any of the syntaxes above is used:


Suggested change

Scala CLI produces a warnings if any of the syntaxes above is used:

Scala CLI produces warnings if any of the syntaxes above is used:

alexarchambault

(It seems some *.semanticdb are committed too.)

alexarchambault · 2022-01-18T14:55:11Z

modules/build/src/main/scala/scala/build/bsp/BspImpl.scala

@@ -135,40 +143,42 @@ final class BspImpl(
      buildChangedTest
    )

-    (mainScope, testScope)
+    // ala a a


?

Suggested change

// ala a a

ah,I needed to force recompilation :/

alexarchambault · 2022-01-18T14:56:58Z

modules/build/src/main/scala/scala/build/bsp/BspImpl.scala

      notifyBuildChange(actualLocalServer)
+
    Build.buildOnce(


Independently of the changes of this PR, it seems both Build.buildOnce calls should have been wrapped in a value { … } :/

Checking how it goes in #557.

I can wrap it but let me know if value { } works

alexarchambault · 2022-01-18T15:04:36Z

modules/build/src/main/scala/scala/build/errors/BuildException.scala

  cause: Throwable = null
-) extends Exception(message, cause)
+) extends Exception(message, cause) with Diagnostic {
+  override def severity: Severity = Severity.Error


We shouldn't need to put a severity here… This might lead to methods returning Either[BuildException, T] to return a Left(ex) with ex.severity == Severity.Warning, which doesn't make sense (exceptions on the left here are implicitly assumed to be plain errors). The severity field makes "invalid states representable".

I've put severity here just to be able to process BuildException and diagnostics using the same code and hence I need to fix severity here. I can change it to final def to make sure nobody will treat Exception as warnings.

alexarchambault · 2022-01-18T15:05:29Z

modules/build/src/main/scala/scala/build/preprocessing/ScalaPreprocessor.scala

@@ -1,6 +1,13 @@
 package scala.build.preprocessing

+import _root_.com.virtuslab.using_directives.custom.utils.ast.UsingDefs


NIT

Suggested change

import _root_.com.virtuslab.using_directives.custom.utils.ast.UsingDefs

import com.virtuslab.using_directives.custom.utils.ast.UsingDefs

alexarchambault · 2022-01-18T15:15:42Z

modules/build/src/test/scala/scala/build/tests/ScalaPreprocessorTests.scala

+  case class Warn(messagePattern: String, line: Int, index: Int) extends Check {
+    def test(diags: Seq[Diagnostic]): Boolean = {
+      val matching = diags.filter(_.positions.collect {
+        case Position.File(_, (l, c), _) if l == line && index == c => l


NIT

Suggested change

case Position.File(_, (l, c), _) if l == line && index == c => l

case Position.File(_, (`line`, `index`), _) => line

(That said, I often forget about matching with backticks myself, so there might be other cases in the code base where we can do the same…)

alexarchambault · 2022-01-18T15:18:00Z

modules/build/src/test/scala/scala/build/tests/ScalaPreprocessorTests.scala

+    def test(diags: Seq[Diagnostic]): Boolean = {
+      val matching = diags.filter(_.positions.collect {
+        case Position.File(_, (l, c), _) if l == line && index == c => l
+      }.nonEmpty)


Maybe the .collect { … }.nonEmpty can be changed to .exists { case Position.File(…) => …; case _ => false }?

alexarchambault · 2022-01-18T15:18:49Z

modules/build/src/test/scala/scala/build/tests/ScalaPreprocessorTests.scala

+      matching.map(_.message).collect {
+        case Regex() => true
+      }.nonEmpty


Same as above

Suggested change

matching.map(_.message).collect {

case Regex() => true

}.nonEmpty

matching.map(_.message).exists {

case Regex() => true

case _ => false

}

romanowski · 2022-01-18T22:54:26Z

@alexarchambault I updated the commit structure so it should be easier to review. I can also separate 202f37a to different PR since all those changes are basically replacing // using with //> using

tpasternak · 2022-01-20T16:30:21Z

modules/build/src/main/scala/scala/build/bsp/BspImpl.scala

+        bloopServer
+      ).left.map(_ -> scope)
+
+    value(doBuildOnce(preBuild.mainScope, Scope.Main))


Please use for-comprehensions as long as we have problems with left-types mismatch in EitherCps

Deprecate using directives in plain comments and usign annotation style.

pikinier20 · 2022-01-24T08:46:59Z

project/deps.sc

+    // coursier.MavenRepository(
+    //   (os.Path(System.getProperty("user.home")) / ".m2" / "repository")
+    //     .toNIO.toUri.toASCIIString
+    // )


IIRC you can add MavenLocal repo with: coursier.LocalRepositories.Dangerous.maven2Local

pikinier20 · 2022-01-24T08:48:53Z

website/docs/guides/using-directives.md


 Until `using` directives becomes a part of the Scala specification, this is the only way that guarantees that your code will work well with IDEs, code formatters, and other tools.
 :::

+Within one file, only one flavor of using directives can be used. The keyword-based syntax (`using scala "3"`) has precedence over special comments (`//> using scala "3"`) and deprecated, plain comments (`// using scala "3"`) has lowest priority.


Suggested change

Within one file, only one flavor of using directives can be used. The keyword-based syntax (`using scala "3"`) has precedence over special comments (`//> using scala "3"`) and deprecated, plain comments (`// using scala "3"`) has lowest priority.

Within one file, only one flavor of using directives can be used. The keyword-based syntax (`using scala "3"`) has precedence over special comments (`//> using scala "3"`). The deprecated, plain comments (`// using scala "3"`) have lowest priority.

pikinier20 · 2022-01-24T08:50:09Z

examples/cross-build/Hello.js.scala

+//> using scala 2.13
+//> using target scala-js


Did I omit a moment when we allowed strings without quotes?

I think we do not check examples if compile file, a good catch, I've open a new issue for that: #575

I will also include fixes in a followup PR.

pikinier20 · 2022-01-24T08:50:25Z

examples/cross-build/Hello.native.scala

+//> using scala 2.13
+//> using target scala-native


pikinier20 · 2022-01-24T08:50:40Z

examples/scala-versions/README.md

@@ -95,7 +95,7 @@ Using directives syntax is still experimental and may change in future versions
 So when we will have:

 ```scala title=version.scala
-// using scala 2.12.5
+//> using scala 2.12.5


pikinier20 · 2022-01-24T08:50:47Z

examples/scala-versions/version.scala

@@ -1,6 +1,6 @@
 // File was generated from based on docs/cookbooks/scala-versions.md, do not edit manually!

-// using scala 2.12.5
+//> using scala 2.12.5


romanowski force-pushed the better-comments branch from 6967cfa to 3203ebc Compare January 16, 2022 21:03

romanowski changed the title ~~Better comments~~ New using directive syntax Jan 18, 2022

romanowski marked this pull request as ready for review January 18, 2022 11:11

romanowski requested review from pikinier20, alexarchambault and lwronski January 18, 2022 11:11

romanowski force-pushed the better-comments branch 2 times, most recently from fb87f23 to 6920d8f Compare January 18, 2022 12:05

alexarchambault reviewed Jan 18, 2022

View reviewed changes

romanowski force-pushed the better-comments branch from 6920d8f to a3dedfd Compare January 18, 2022 22:51

romanowski force-pushed the better-comments branch from a3dedfd to 975cd30 Compare January 18, 2022 23:37

romanowski requested a review from alexarchambault January 19, 2022 17:18

romanowski mentioned this pull request Jan 20, 2022

Don't discard errors in BSP server #557

Closed

tpasternak reviewed Jan 20, 2022

View reviewed changes

alexarchambault and others added 13 commits January 23, 2022 18:14

Factor custom repositories use in build

c224dcb

Clean-up

75c577c

Missing "[error]" / "[warn]" prefix when printing diagnostics

4af8f2b

Enforce more Java 17 use

1a2cbad

Fix scala-cli script

d51c690

Switch to new syntax of using directives

0762acc

Deprecate using directives in plain comments and usign annotation style.

Try to add bsp integration for diagnostics

73591e7

Add tests for warnings from using directive syntax

f55139d

Update using directive documentation

3f05dad

Change using directive to special comment syntax

98e5341

Using directives with fixed NPE problem

ab02993

Small refactoring

f1fd1b1

Use for comprehension in critical bsp method

0162676

alexarchambault force-pushed the better-comments branch from df15e2a to 0162676 Compare January 23, 2022 20:39

pikinier20 reviewed Jan 24, 2022

View reviewed changes

romanowski merged commit 2c8add7 into VirtusLab:master Jan 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New using directive syntax #546

New using directive syntax #546

romanowski commented Jan 15, 2022 •

edited

Loading

alexarchambault left a comment

alexarchambault Jan 18, 2022

romanowski Jan 18, 2022

alexarchambault Jan 18, 2022

alexarchambault Jan 18, 2022

alexarchambault Jan 18, 2022

alexarchambault Jan 18, 2022

alexarchambault Jan 18, 2022

alexarchambault left a comment

alexarchambault Jan 18, 2022

romanowski Jan 18, 2022

alexarchambault Jan 18, 2022

alexarchambault Jan 18, 2022

romanowski Jan 18, 2022

alexarchambault Jan 18, 2022

romanowski Jan 18, 2022

alexarchambault Jan 18, 2022

alexarchambault Jan 18, 2022

alexarchambault Jan 18, 2022

alexarchambault Jan 18, 2022

romanowski commented Jan 18, 2022

tpasternak Jan 20, 2022

romanowski Jan 20, 2022

pikinier20 Jan 24, 2022

pikinier20 Jan 24, 2022

pikinier20 Jan 24, 2022

romanowski Jan 24, 2022

pikinier20 Jan 24, 2022

pikinier20 Jan 24, 2022

pikinier20 Jan 24, 2022

	Within one file, only one flavor of using directives can be used. The keyword-base syntax (`using scala "3"`) has precedence over special comments (`//> using scala "3"`) and deprecated, plain comments (`// using scala "3"`) has lowest priority.
	Within one file, only one flavor of using directives can be used. The keyword-based syntax (`using scala "3"`) has precedence over special comments (`//> using scala "3"`) and deprecated, plain comments (`// using scala "3"`) has lowest priority.

	For now `using` and `@using` can be mixed within given syntax however we strongly suggest to does not use deprecated `@using`.
	For now `using` and `@using` can be mixed within a given syntax however we strongly suggest not to use deprecated `@using`.

	As a part of `0.0.x` series we experimented with different syntax for using directives. Based on the feedback and discussions with Scala compiler teams we decided to deprecated `@using` (using annotations) and `// using` (using within plain import). Those syntaxes will work as a part of of `0.1.x` series and we will result with an error since `0.2.x`.
	As a part of `0.0.x` series we experimented with different syntaxes for using directives. Based on feedback and discussions with the Scala compiler team, we decided to deprecate `@using` (using annotations) and `// using` (using within plain comment). Those syntaxes will keep working in the `0.1.x` series and will result in an error starting from `0.2.x`.


		As a part of `0.0.x` series we experimented with different syntax for using directives. Based on the feedback and discussions with Scala compiler teams we decided to deprecated `@using` (using annotations) and `// using` (using within plain import). Those syntaxes will work as a part of of `0.1.x` series and we will result with an error since `0.2.x`.

		Scala CLI produces a warnings if any of the syntaxes above is used:

	Scala CLI produces a warnings if any of the syntaxes above is used:
	Scala CLI produces warnings if any of the syntaxes above is used:

		@@ -1,6 +1,13 @@
		package scala.build.preprocessing

		import _root_.com.virtuslab.using_directives.custom.utils.ast.UsingDefs

	import _root_.com.virtuslab.using_directives.custom.utils.ast.UsingDefs
	import com.virtuslab.using_directives.custom.utils.ast.UsingDefs

	case Position.File(_, (l, c), _) if l == line && index == c => l
	case Position.File(_, (`line`, `index`), _) => line

New using directive syntax #546

New using directive syntax #546

Conversation

romanowski commented Jan 15, 2022 • edited Loading

alexarchambault left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexarchambault left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romanowski commented Jan 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romanowski commented Jan 15, 2022 •

edited

Loading