you're on a roll!

to facilitate review, let's not squash until we're totally ready to merge this. (the full, combined diff is........ l-o-n-g.......)

@eed3si9n you didn't write it, but I think the "convert procedure to method" wording in the error message is... not ideal. "procedures" are still methods, they aren't some entirely separate category. also, "add : Unit" isn't precisely right, it should be "add : Unit =" (with the equals sign). after all it's the equals sign that's 100% necessary, the : Unit is actually optional in many cases since it would be inferred anyway. (I'm comfortable with always recommending it regardless.)

how about replacing "convert..." with simply "instead, add : Unit = to method $name"

@SethTisue

before

proc.scala:4: warning: procedure syntax is deprecated: convert procedure `boo` to method by adding `: Unit`
  def boo(i: Int, l: Long)
                          ^
proc.scala:5: warning: procedure syntax is deprecated: convert procedure `boz` to method by adding `: Unit =`
  def boz(i: Int, l: Long) {}
                           ^

after

proc.scala:4: warning: procedure syntax is deprecated: instead, add `: Unit` to `boo`
  def boo(i: Int, l: Long)
                          ^
proc.scala:5: warning: procedure syntax is deprecated: instead, add `: Unit =` to `boz`
  def boz(i: Int, l: Long) {}
                           ^

With either of these messages, is it be clear for the users to know what to do?

Problem 1-B: Suppose the user, who is otherwise proficient in Scala, has never heard of the term "procedure syntax."

Suppose the user, who is otherwise proficient in Scala, has never heard of the term "procedure syntax."

I don't mean to be flip, but: they can Google it?

I don't see a way to do better without writing a substantially longer error message. (doing that wouldn't be a bad idea, but I think the PR is mergeable regardless.)

maybe "procedure syntax is deprecated: instead, add : Unit = to explicitly declareboz's return type" ?

+1 on "procedure syntax is deprecated: instead, add : Unit = to explicitly declare boz's return type".

Ping?

It would be good if we expedited this a bit in order for Eugene not to have to do a bunch of annoying rebases. I’ll try to take a final look this week.

Did some rebasing and squashing to compensate for my tardiness. Hope you won't mind my push -f.

Just realized the last two commits will need another rebase since we'll merge the new collections first. (One line in src/compiler/scala/tools/nsc/ast/parser/xml/MarkupParserCommon.scala and 29 tests.)

@eed3si9n sorry, could we ask you for one more rebase, then we'll merge?

(P.S. if you're worried that our current merge promises are no more trustworthy than previous ones :-/, you might consider splitting this into two PRs, one big one that just removes uses of procedure syntax codebase-wide — since we could easily merge that the moment Jenkins liked it, there's no downside — and then a second, small PR that does the deprecation.)

(P.S. if you're worried that our current merge promises are no more trustworthy than previous ones :-/, you might consider splitting this into two PRs, one big one that just removes uses of procedure syntax codebase-wide — since we could easily merge that the moment Jenkins liked it, there's no downside — and then a second, small PR that does the deprecation.)

Given that the collection library is merged, I am hoping that there's no more blockers to this PR once all the tests pass.

go Jenkins go!

tests need updating. even the first commit fails, probably because the collections merge introduced new tests that are now causing deprecation warnings.

@eed3si9n wouldn't it minimize churn if you first got rid of all the uses, then deprecated? if done in that order, the deprecation commit would be small.

I don't think the ordering changes things as long as it affects a bunch of files, and if it stays open for months, assuming that deprecation itself is not blocked/controversial.

I originally had it [ci: last-only] so I can keep appending more commits to update the tests. Once they are passing, I can squash them all to get greens.

okay. regardless, I'm standing by to hit "merge" as soon as Jenkins likes it.

@SethTisue Rebased and squashed.

your revenge is that now everybody else has to rebase their PRs 😁

thank you, Eugene

Thanks for the merge!

this came up at the SIP meeting today. I get a deprecation warning with the M5 compiler:

% cat > S.scala
object S { def foo() { } }
% /usr/local/scala/scala-2.13.0-M5/bin/scalac -deprecation -feature S.scala
S.scala:1: warning: procedure syntax is deprecated: instead, add `: Unit =` to explicitly declare `foo`'s return type

but in the REPL, I don't:

% /usr/local/scala/scala-2.13.0-M5/bin/scala -deprecation        
Welcome to Scala 2.13.0-M5 (Java HotSpot(TM) 64-Bit Server VM, Java 1.8.0_192).
Type in expressions for evaluation. Or try :help.

scala> def foo() { }
foo: ()Unit

scala> object S { def foo() { } }
defined object S

scala> (new Thread).stop()
                    ^
       warning: method stop in class Thread is deprecated: see corresponding Javadoc for more information.

@som-snytt @adriaanm any clue why this might be happening, where an investigation should start?

From now on, "procedure syntax" will refer only to the order and form of the words you must use at a SIP meeting to get your point across.

I'll take that as a "no" :-)

I'll take a closer look. Deprecation works in general. So it's not a lost flag.

BTW -deprecation -feature are on by default.

Also -Xsource:2.14 correctly errors. Luckily, there is pizza in the break room.

Looks like just the parser deprecations aren't forwarded.

scala> for (i <- List(1); val j = i + 1) yield j
res0: List[Int] = List(2)

scala> :replay -Xsource:2.14
Replaying: for (i <- List(1); val j = i + 1) yield j
                                ^
       error: `val` keyword in for comprehension is unsupported: instead, bind the value without `val`

https://github.com/scala/scala/blob/v2.13.0-M5/src/repl/scala/tools/nsc/interpreter/IMain.scala#L1082-L1085

  def parse(line: String): Either[Result, (List[Tree], Position)] = {
    var isIncomplete = false
    currentRun.parsing.withIncompleteHandler((_, _) => isIncomplete = true) {
      withoutWarnings {

REPL parsing code is silencing all warnings.

This was added in #5647 to fix scala/bug#10130 about multiple warnings showing up.

It parses again after templating, IIRC. Edit: I see that's my comment on the PR you point out.

I thought maybe I broke something in touching the reporters.

On 2.13.x, it seems to work without withoutWarnings:

> scala -deprecation
....
scala> '\060'
        ^
       error: octal escape literals are unsupported: use \u0030 instead

scala> def foo() { }
                 ^
       warning: procedure syntax is deprecated: instead, add `: Unit =` to explicitly declare `foo`'s return type
foo: ()Unit

OK, good!

I see, Adriaan keeps the trees and splices it into the template.