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

Deprecate procedure syntax without -Xfuture #6325

Merged
merged 2 commits into from
Apr 25, 2018

Conversation

eed3si9n
Copy link
Member

@eed3si9n eed3si9n commented Feb 14, 2018

Ref scala/bug#7605

#3076 deprecated the procedure syntax, but only under -Xfuture flag. This deprecates it without it, and drops it under -Xsource:2.14.

This PR also migrates all code under src/compiler/ and test/ to : Unit =-style, mostly automatically by using ScalaFix's ProcedureSyntax (and ExplicitUnit, which will be merged to ProcedureSyntax) rule.

@scala-jenkins scala-jenkins added this to the 2.13.0-M4 milestone Feb 14, 2018
@SethTisue
Copy link
Member

you're on a roll!

restype = scalaUnitConstr
}
newmods |= Flags.DEFERRED
EmptyTree
} else if (restype.isEmpty && in.token == LBRACE) {
if (settings.future)
deprecationWarning(in.offset, s"Procedure syntax is deprecated. Convert procedure `$name` to method by adding `: Unit =`.", "2.12.0")
if (settings.isScala214) syntaxError(in.lastOffset, msg("unsupported"))
Copy link
Member

Choose a reason for hiding this comment

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

defmsg

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice catch.

@eed3si9n eed3si9n changed the title Deprecate procedure syntax without -Xfuture Deprecate procedure syntax without -Xfuture [ci: last-only] Feb 15, 2018
@eed3si9n

This comment has been minimized.

@SethTisue
Copy link
Member

SethTisue commented Feb 22, 2018

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"

@@ -2718,18 +2718,20 @@ self =>
val vparamss = paramClauses(name, contextBoundBuf.toList, ofCaseClass = false)
newLineOptWhenFollowedBy(LBRACE)
var restype = fromWithinReturnType(typedOpt())
def msg(what: String) = s"procedure syntax is $what. convert procedure `$name` to method by adding `: Unit`."
def defmsg(what: String) = s"procedure syntax is $what. convert procedure `$name` to method by adding `: Unit =`."
Copy link
Member Author

Choose a reason for hiding this comment

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

@SethTisue = I think is in the error message when it is a definition. Do you have a negative test link that shows otherwise?

Copy link
Member

Choose a reason for hiding this comment

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

sorry, I missed that, nm.

@eed3si9n
Copy link
Member Author

@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."

@SethTisue
Copy link
Member

SethTisue commented Feb 22, 2018

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" ?

@eed3si9n
Copy link
Member Author

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

@eed3si9n
Copy link
Member Author

eed3si9n commented Apr 8, 2018

Ping?

@SethTisue
Copy link
Member

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.

@adriaanm
Copy link
Contributor

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

restype = scalaUnitConstr
}
newmods |= Flags.DEFERRED
EmptyTree
} else if (restype.isEmpty && in.token == LBRACE) {
if (settings.future)
deprecationWarning(in.offset, s"Procedure syntax is deprecated. Convert procedure `$name` to method by adding `: Unit =`.", "2.12.0")
if (settings.isScala214) syntaxError(in.lastOffset, msg("unsupported", ": Unit ="))
Copy link
Contributor

Choose a reason for hiding this comment

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

The error should probably be positioned at in.offset. I'll push a fix once I've updated the check files

@adriaanm adriaanm force-pushed the wip/deprecate-procedure branch 2 times, most recently from 55bf785 to e13047c Compare April 10, 2018 13:39
@adriaanm adriaanm changed the title Deprecate procedure syntax without -Xfuture [ci: last-only] Deprecate procedure syntax without -Xfuture Apr 10, 2018
@adriaanm
Copy link
Contributor

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.)

@SethTisue
Copy link
Member

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

@SethTisue
Copy link
Member

SethTisue commented Apr 24, 2018

(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.)

@eed3si9n
Copy link
Member Author

(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.

@SethTisue
Copy link
Member

go Jenkins go!

@SethTisue
Copy link
Member

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

@SethTisue
Copy link
Member

@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.

@eed3si9n
Copy link
Member Author

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.

@eed3si9n
Copy link
Member Author

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.

@SethTisue
Copy link
Member

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

Procedure syntax was deprecated under -Xfuture flag in scala#3076.
This deprecates it unconditionally, and drops it under -Xsource:2.14.
See scala/bug#7605

To update the tests, this drops procedure syntax from test/ using ScalaFix

```
$ coursier launch ch.epfl.scala:scalafix-cli_2.12.3:0.5.3 -- -r ProcedureSyntax test
$ coursier launch ch.epfl.scala:scalafix-cli_2.12.4:0.5.10 -- -r ExplicitUnit test
```
```
$ coursier launch ch.epfl.scala:scalafix-cli_2.12.3:0.5.3 -- -r ProcedureSyntax src/compiler
$ coursier launch ch.epfl.scala:scalafix-cli_2.12.4:0.5.10 -- -r ExplicitUnit src/compiler
```
@eed3si9n
Copy link
Member Author

@SethTisue Rebased and squashed.

@SethTisue SethTisue merged commit b579903 into scala:2.13.x Apr 25, 2018
@SethTisue
Copy link
Member

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

@SethTisue
Copy link
Member

thank you, Eugene

@eed3si9n eed3si9n deleted the wip/deprecate-procedure branch April 25, 2018 14:51
@eed3si9n
Copy link
Member Author

Thanks for the merge!

@SethTisue SethTisue added the release-notes worth highlighting in next release notes label May 4, 2018
@SethTisue
Copy link
Member

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.

@SethTisue
Copy link
Member

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

@som-snytt
Copy link
Contributor

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.

@SethTisue
Copy link
Member

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

@som-snytt
Copy link
Contributor

som-snytt commented Nov 27, 2018

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`

@eed3si9n
Copy link
Member Author

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.

@eed3si9n
Copy link
Member Author

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

@som-snytt
Copy link
Contributor

som-snytt commented Nov 27, 2018

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.

@eed3si9n
Copy link
Member Author

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

@som-snytt
Copy link
Contributor

som-snytt commented Nov 27, 2018

OK, good!

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

eed3si9n added a commit to eed3si9n/scala that referenced this pull request Nov 27, 2018
In scala#6325 Seth reported that procedure syntax
deprecation warnings are not displayed on REPL.
This is because warnings were filtered out by `withoutWarnings` in scala#5647 to fix scala/bug#10130.
On 2.13.x, it seems to work ok without the `withoutWarnings` filter during parse.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes worth highlighting in next release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants