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 -Wperformance lints for *Ref boxing and nonlocal return #9889

Merged
merged 6 commits into from Mar 11, 2022

Conversation

som-snytt
Copy link
Contributor

@som-snytt som-snytt commented Jan 2, 2022

Warn when local variables are captured by lambdas or by-name args and moved to the heap.

The warnings are behind a new -Wperformance flag, as the user did nothing wrong but might be surprised by a difference in performance.

Similarly, -Xlint:nonlocal-return is also enabled by the new flag, though it is not removed from -Xlint.

Revives #8691 by @hrhino.

@scala-jenkins scala-jenkins added this to the 2.13.9 milestone Jan 2, 2022
@SethTisue SethTisue marked this pull request as draft January 2, 2022 16:27
-Xlint:nonlocal-return is supported for compatibility.
@som-snytt som-snytt changed the title Potentially annoying lints for -Wperformance -Wperformance lints *Ref boxing and nonlocal return Jan 2, 2022
@som-snytt som-snytt marked this pull request as ready for review January 2, 2022 19:12
@som-snytt
Copy link
Contributor Author

It would be nice to report whether boxings are eliminated by optimizer after closure inlining.

@lrytz lrytz requested a review from SethTisue January 3, 2022 11:06
@SethTisue SethTisue added the release-notes worth highlighting in next release notes label Jan 19, 2022
@SethTisue
Copy link
Member

@som-snytt I don't doubt this will be merged in the end, but would you mind filling in the PR description, to aid reviewers? (But also as end-user documentation, since this is release-notable.)

@SethTisue
Copy link
Member

Let's wait to merge until @lrytz is available.

@som-snytt som-snytt marked this pull request as draft March 4, 2022 13:54
@som-snytt
Copy link
Contributor Author

This is under-specified, but 2.13.8 says

scala> StringContext.glob(Nil,"")
java.lang.NegativeArraySizeException: -1
  at scala.StringContext$.glob(StringContext.scala:233)
  ... 32 elided

and 2.13.9 will say

scala> StringContext.glob(Nil,"")
java.util.NoSuchElementException: head of empty list
  at scala.collection.immutable.Nil$.head(List.scala:662)
  at scala.collection.immutable.Nil$.head(List.scala:661)
  at scala.StringContext$.glob(StringContext.scala:229)
  ... 34 elided

@som-snytt som-snytt marked this pull request as ready for review March 4, 2022 16:00
@som-snytt
Copy link
Contributor Author

Those other commits must have been prompted by the warning. But IIRC they are a balance of style rather than benchmark-driven.

Somewhere I must have seen someone's suggestion for the hand-rolled function object.

@SethTisue
Copy link
Member

SethTisue commented Mar 11, 2022

I see that back in February 2020 my take on Harrison's PR was:

agree this more properly would be part of a separate linter or Scalafix ruleset

and, well, I still wonder about that a bit. but I don't recall my whole thought process, there.

the implementation doesn't seem like a likely maintenance burden. and perhaps my previous thinking was merely pedantic.

@SethTisue SethTisue merged commit 2358bf0 into scala:2.13.x Mar 11, 2022
@som-snytt som-snytt deleted the topic/xlint-perf branch March 11, 2022 20:42
var result = immutable.Map.empty[K, CC[B]]
m.foreach { case (k, v) =>
result = result + ((k, v.result()))
object result extends Function[(K, Builder[B, CC[B]]), Unit] {
Copy link
Contributor

Choose a reason for hiding this comment

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

use class and AbstractFunction ? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I will also add a lint for that.

@SethTisue SethTisue changed the title -Wperformance lints *Ref boxing and nonlocal return Add -Wperformance lints for *Ref boxing and nonlocal return Sep 1, 2022
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
6 participants