From 951939d1b35ebfd1ac1f1a96ca02cbff0c056b63 Mon Sep 17 00:00:00 2001 From: Seth Tisue Date: Mon, 13 Jul 2015 18:56:11 -0400 Subject: contributor guide: add a morsel salvaged from GitHub wiki --- CONTRIBUTING.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 54334aea48..e6e0e0b923 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -47,6 +47,7 @@ A new language feature requires a SIP (Scala Improvement Process) proposal. For #### Summary 1. We require regression tests for bug fixes. New features and enhancements must be supported by a respectable test suite. + - Consider including comments in the test files that indicates what you're testing and why. Expected outcome, what happened before the fix, what happens now, that sort of thing. 2. Documentation. Yep! Also required :-) 3. Please follow these standard code standards, though in moderation (scouts quickly learn to let sleeping dogs lie): - Not violate [DRY](http://programmer.97things.oreilly.com/wiki/index.php/Don%27t_Repeat_Yourself). -- cgit v1.2.3 From e93ca409ae0d7cd6489c66f696ad564c77bcb6a2 Mon Sep 17 00:00:00 2001 From: Seth Tisue Date: Mon, 13 Jul 2015 18:57:45 -0400 Subject: drop in pull request policy from old wiki --- CONTRIBUTING.md | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 65 insertions(+) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index e6e0e0b923..d7705512a3 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -54,3 +54,68 @@ A new language feature requires a SIP (Scala Improvement Process) proposal. For - [Boy Scout Rule](http://programmer.97things.oreilly.com/wiki/index.php/The_Boy_Scout_Rule) should be applied. Please also have a look at our [Pull Request Policy](https://github.com/scala/scala/wiki/Pull-Request-Policy), as well as the [Scala Hacker Guide](http://www.scala-lang.org/contribute/hacker-guide.html) by @xeno-by. + + +# Pull Request Policy + +taken from https://github.com/scala/scala/wiki/Pull-Request-Policy +before it was put out of its misery + +Hi there, pull request submitter! + +Your pull request should: + - (... have been discussed on scala-internals) + - merge cleanly + - consist of commits with messages that clearly state which problem this commit resolves and how + - it should be stated in the active, present tense + - its subject should be 60 characters or less + - [conventions](http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html) + - for a bug fix, the title must look like "SI-NNNN - don't crash when moon is in wrong phase" + - overall, think of the first line of the commit as a description of the action performed by the commit on the code base, so use the present tense -- that also makes them easy to reuse in release notes + - backports should be tagged as [backport], it's also nice to mention this when a commit purely refactors and is not intended to change behaviour + - when working on maintenance branches (e.g., 2.10.x), include [nomerge] if this commit should not be merged forward into the next release branch + - come with tests (included in the same commit as the functional change), or explain in detail why it cannot be tested (discuss this on scala-internals first). The tests itself should: + - be minimal, deterministic, stable (unaffected by irrelevant changes), easy to understand and review + - have minimal dependencies: a compiler bug should not depend on, e.g. the Scala library + - typically fail before your fix is applied (so we see that you are fixing a legitimate bug) and should obviously pass after your fix + - come with appropriate documentation + - for example, any API additions should include Scaladoc + - each commit must pass the test suite (checked automatically by the build bot by running approximately `ant test-opt`) + - a commit is considered a unit of useful change and must thus pass the test suite + (this way we stand a chance running git bisect later) + - be assigned to one or more reviewers (if you're not sure, see the list below or contact scala-internals) + - to assign a reviewer, add a "review by @reviewer" to your PR description. NOTE: it's best not to @mention in commit messages, as github pings you every time a commit with your @name on it shuffles through the system (even in other repos, on merges,...) + - get the green light from the reviewer ("LGTM" -- looks good to me) + - review feedback may be addressed by pushing new commits to the request, + if these commits stand on their own + +Once all these conditions are met, and we agree with the change +(we are available on scala-internals to discuss this beforehand), +we will merge your changes. + +Please note: you are responsible for meeting these criteria (reminding your reviewer, for example). + +### Pull request bot mechanics +* The build bot automatically builds commits as they appear in a PR. Click on the little x next to a commit sha to go to the overview of the PR validation job. To diagnose a failure, consult the console output of the job that failed. + * 'PLS REBUILD ALL' will force the bot to rebuild (only necessary when a spurious failure occurred -- i.e., you expect a different validation outcome without changing the commit shas that make up the PR) + +### List of reviewers by area: + +* library: @phaller (Philipp Haller), @axel22 (Aleksander Prokopec -- concurrent & collection) +* specialisation: @axel22 (Aleksander Prokopec), @vladureche (Vlad Ureche), @dragos (Iulian Dragos) +* named / default args, annotations, plugins: @lrytz (Lukas Rytz) +* macros, reflection, manifests, string interpolation: @xeno-by (Eugene Burmako), @cvogt (Christopher Vogt) +* type checker, inference: @odersky (Martin Odersky), @adriaanm (Adriaan Moors) +* Language specification, value classes: @odersky (Martin Odersky) +* new pattern matcher, implicit search: @adriaanm (Adriaan Moors) +* partest, Continuations Plugin: @phaller (Philipp Haller) +* error handling, lazy vals: @hubertp (Hubert Plociniczak) +* backend: @magarciaEPFL (Miguel Garcia), @gkossakowski (Grzegorz Kossakowski), @dragos (Iulian Dragos) +* repl, compiler performance: @retronym (Jason Zaugg) +* swing: @ingoem (Ingo Maier) +* scaladoc: @dickwall (Dick Wall) +* optimizer: @vladureche (Vlad Ureche), @magarciaEPFL (Miguel Garcia) +* build: @jsuereth (Josh Suereth) +* random compiler bugs: @lrytz, @adriaanm, @hubertp +* documentation: @heathermiller (Heather Miller), @dickwall (Dick Wall) +* cps: @TiarkRompf (Tiark Rompf) -- cgit v1.2.3 From 197845620cb827209272bb02e6740feac9f73a48 Mon Sep 17 00:00:00 2001 From: Seth Tisue Date: Wed, 15 Jul 2015 09:53:35 -0400 Subject: merge in text from pull request policy from old wiki --- CONTRIBUTING.md | 168 +++++++++++++++++++++++++++++++------------------------- README.md | 22 +++++++- 2 files changed, 114 insertions(+), 76 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index d7705512a3..1ea7e5513b 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -38,84 +38,104 @@ Please make sure the JIRA ticket's fix version corresponds to the upcoming miles #### Enhancement or New Feature -For longer-running development, likely required for this category of code contributions, we suggest you include "topic" or "wip" in your branch name, to indicate that this is work in progress, and that others should be prepared to rebase if they branch off your branch. +For longer-running development, likely required for this category of code contributions, we suggest you include "topic/" or "wip/" in your branch name, to indicate that this is work in progress, and that others should be prepared to rebase if they branch off your branch. Any language change (including bug fixes) must be accompanied by the relevant updates to the spec, which lives in the same repository for this reason. A new language feature requires a SIP (Scala Improvement Process) proposal. For more details on submitting SIPs, see [how to submit a SIP](http://docs.scala-lang.org/sips/sip-submission.html). -#### Summary - -1. We require regression tests for bug fixes. New features and enhancements must be supported by a respectable test suite. - - Consider including comments in the test files that indicates what you're testing and why. Expected outcome, what happened before the fix, what happens now, that sort of thing. -2. Documentation. Yep! Also required :-) -3. Please follow these standard code standards, though in moderation (scouts quickly learn to let sleeping dogs lie): - - Not violate [DRY](http://programmer.97things.oreilly.com/wiki/index.php/Don%27t_Repeat_Yourself). - - [Boy Scout Rule](http://programmer.97things.oreilly.com/wiki/index.php/The_Boy_Scout_Rule) should be applied. - -Please also have a look at our [Pull Request Policy](https://github.com/scala/scala/wiki/Pull-Request-Policy), as well as the [Scala Hacker Guide](http://www.scala-lang.org/contribute/hacker-guide.html) by @xeno-by. - - -# Pull Request Policy - -taken from https://github.com/scala/scala/wiki/Pull-Request-Policy -before it was put out of its misery - -Hi there, pull request submitter! - -Your pull request should: - - (... have been discussed on scala-internals) - - merge cleanly - - consist of commits with messages that clearly state which problem this commit resolves and how - - it should be stated in the active, present tense - - its subject should be 60 characters or less - - [conventions](http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html) - - for a bug fix, the title must look like "SI-NNNN - don't crash when moon is in wrong phase" - - overall, think of the first line of the commit as a description of the action performed by the commit on the code base, so use the present tense -- that also makes them easy to reuse in release notes - - backports should be tagged as [backport], it's also nice to mention this when a commit purely refactors and is not intended to change behaviour - - when working on maintenance branches (e.g., 2.10.x), include [nomerge] if this commit should not be merged forward into the next release branch - - come with tests (included in the same commit as the functional change), or explain in detail why it cannot be tested (discuss this on scala-internals first). The tests itself should: - - be minimal, deterministic, stable (unaffected by irrelevant changes), easy to understand and review - - have minimal dependencies: a compiler bug should not depend on, e.g. the Scala library - - typically fail before your fix is applied (so we see that you are fixing a legitimate bug) and should obviously pass after your fix - - come with appropriate documentation - - for example, any API additions should include Scaladoc - - each commit must pass the test suite (checked automatically by the build bot by running approximately `ant test-opt`) - - a commit is considered a unit of useful change and must thus pass the test suite - (this way we stand a chance running git bisect later) - - be assigned to one or more reviewers (if you're not sure, see the list below or contact scala-internals) - - to assign a reviewer, add a "review by @reviewer" to your PR description. NOTE: it's best not to @mention in commit messages, as github pings you every time a commit with your @name on it shuffles through the system (even in other repos, on merges,...) - - get the green light from the reviewer ("LGTM" -- looks good to me) - - review feedback may be addressed by pushing new commits to the request, - if these commits stand on their own - -Once all these conditions are met, and we agree with the change -(we are available on scala-internals to discuss this beforehand), -we will merge your changes. +## Guidelines -Please note: you are responsible for meeting these criteria (reminding your reviewer, for example). +Here is some advice on how to craft a pull request with the best possible +chance of being accepted. + +### Merging + +A pull request should merge cleanly. (If enough time passes after +your initial submission, we may ask you to rebase it onto the current +mainline code.) + +### Tests + +Bug fixes should include regression tests -- in the same commit as the fix. + +If testing isn't feasible, the commit message should explain why. +(Consider discussing on scala-internals first.) + +New features and enhancements must be supported by a respectable test suite. + +Consider including comments in the test files that indicate what you're testing and why. Expected outcome, what happened before the fix, what happens now, that sort of thing. + +Some characteristics of good tests: + +* be minimal, deterministic, stable (unaffected by irrelevant changes), easy to understand and review +* have minimal dependencies: a compiler bug test should not depend on, e.g., the Scala library +* typically fail before your fix is applied (so we see that you are fixing a legitimate bug) and should obviously pass after your fix + +### Documentation + +This is of course required for new features and enhancements. + +Any API additions should include Scaladoc. + +Don't forget to update the package-level doc (in the package object), if appropriate. + +### Coding standards + +Please follow these standard code standards, though in moderation (scouts quickly learn to let sleeping dogs lie): + +* Don't violate [DRY](http://programmer.97things.oreilly.com/wiki/index.php/Don%27t_Repeat_Yourself). +* Follow the [Boy Scout Rule](http://programmer.97things.oreilly.com/wiki/index.php/The_Boy_Scout_Rule). + +Please also have a look at the [Scala Hacker Guide](http://www.scala-lang.org/contribute/hacker-guide.html) by @xeno-by. + +### Clean commits, clean history -### Pull request bot mechanics -* The build bot automatically builds commits as they appear in a PR. Click on the little x next to a commit sha to go to the overview of the PR validation job. To diagnose a failure, consult the console output of the job that failed. - * 'PLS REBUILD ALL' will force the bot to rebuild (only necessary when a spurious failure occurred -- i.e., you expect a different validation outcome without changing the commit shas that make up the PR) - -### List of reviewers by area: - -* library: @phaller (Philipp Haller), @axel22 (Aleksander Prokopec -- concurrent & collection) -* specialisation: @axel22 (Aleksander Prokopec), @vladureche (Vlad Ureche), @dragos (Iulian Dragos) -* named / default args, annotations, plugins: @lrytz (Lukas Rytz) -* macros, reflection, manifests, string interpolation: @xeno-by (Eugene Burmako), @cvogt (Christopher Vogt) -* type checker, inference: @odersky (Martin Odersky), @adriaanm (Adriaan Moors) -* Language specification, value classes: @odersky (Martin Odersky) -* new pattern matcher, implicit search: @adriaanm (Adriaan Moors) -* partest, Continuations Plugin: @phaller (Philipp Haller) -* error handling, lazy vals: @hubertp (Hubert Plociniczak) -* backend: @magarciaEPFL (Miguel Garcia), @gkossakowski (Grzegorz Kossakowski), @dragos (Iulian Dragos) -* repl, compiler performance: @retronym (Jason Zaugg) -* swing: @ingoem (Ingo Maier) -* scaladoc: @dickwall (Dick Wall) -* optimizer: @vladureche (Vlad Ureche), @magarciaEPFL (Miguel Garcia) -* build: @jsuereth (Josh Suereth) -* random compiler bugs: @lrytz, @adriaanm, @hubertp -* documentation: @heathermiller (Heather Miller), @dickwall (Dick Wall) -* cps: @TiarkRompf (Tiark Rompf) +A pull request should consist of commits with messages that clearly state what problem the commit resolves and how. + +Commit logs should be stated in the active, present tense. + +A commit's subject should be 60 characters or less. Overall, think of +the first line of the commit as a description of the action performed +by the commit on the code base, so use the active voice and the +present tense. That also makes the commit subjects easy to reuse in +release notes. + +For a bugfix, the title must look like "SI-NNNN - don't crash when +moon is in wrong phase". + +If a commit purely refactors and is not intended to change behaviour, +say so. + +Backports should be tagged as "[backport]". + +When working on maintenance branches (e.g., 2.11.x), include "[nomerge]" +if this commit should not be merged forward into the next release +branch. + +Here are is standard advice on good commit messages: +http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html + +### Pass Scabot + +Our pull request bot, Scabot, automatically builds all the commits in a PR individually. (All, so we can `git bisect` later.) + +Click on the little x next to a commit sha to go to the overview of the PR validation job. To diagnose a failure, consult the console output of the job that failed. + +See the [scala-jenkins-infra repo](https://github.com/scala/scala-jenkins-infra) and [Scabot repo](https://github.com/) for full details on PR validation. One tip you should know is that commenting `/rebuild` on a PR asks validation to be run again on the same commits. This is only necessary when a spurious failure occurred. + +### Pass code review + +Your PR will need to be assigned to one or more reviewers. You can suggest reviewers yourself; if you're not sure, see the list in [README.md](README.md) or ask on scala-internals. + +To assign a reviewer, add a "review by @reviewer" to your PR description. + +NOTE: it's best not to @mention in commit messages, as github pings you every time a commit with your @name on it shuffles through the system (even in other repos, on merges,...). + +A reviewer gives the green light by commenting "LGTM" (looks good to me). + +A review feedback may be addressed by pushing new commits to the request, if these commits stand on their own. + +Once all these conditions are met, and we agree with the change (we are available on scala-internals to discuss this beforehand, before you put in the coding work!), we will merge your changes. + +Please note: you are responsible for meeting these criteria (reminding your reviewer, for example). diff --git a/README.md b/README.md index e722c88e41..e2174e8013 100644 --- a/README.md +++ b/README.md @@ -23,8 +23,26 @@ If you need some help with your PR at any time, please feel free to @-mention an | [`@xeno-by`](https://github.com/xeno-by) | macros and reflection | | [`@dickwall`](https://github.com/dickwall) | process & community | - -PS: If you have some spare time to help out around here, we would be delighted to add your name to this list! +additional reviewer suggestions: + +* library: @phaller (Philipp Haller), @axel22 (Aleksander Prokopec -- concurrent & collection) +* specialisation: @axel22 (Aleksander Prokopec), @vladureche (Vlad Ureche), @dragos (Iulian Dragos) +* named / default args, annotations, plugins: @lrytz (Lukas Rytz) +* macros, reflection, manifests, string interpolation: @xeno-by (Eugene Burmako), @cvogt (Christopher Vogt) +* type checker, inference: @odersky (Martin Odersky), @adriaanm (Adriaan Moors) +* language specification, value classes: @odersky (Martin Odersky) +* new pattern matcher, implicit search: @adriaanm (Adriaan Moors) +* partest: @phaller (Philipp Haller) +* error handling, lazy vals: @hubertp (Hubert Plociniczak) +* backend: @lrytz (Lukas Rytz), @retronym (Jason Zaugg), @dragos (Iulian Dragos) +* repl, compiler performance: @retronym (Jason Zaugg) +* scaladoc: @dickwall (Dick Wall), @vladureche (Vlad Ureche) +* optimizer: @vladureche (Vlad Ureche), @lrytz (Lukas Rytz) +* build: @jsuereth (Josh Suereth) +* random compiler bugs: @retronym, @lrytz, @adriaanm +* documentation: @heathermiller (Heather Miller), @dickwall (Dick Wall) + +P.S.: If you have some spare time to help out around here, we would be delighted to add your name to this list! # Handy Links - [A wealth of documentation](http://docs.scala-lang.org) -- cgit v1.2.3 From 80e98b03a1cf2cc332e65c80f0d0baf49621b3df Mon Sep 17 00:00:00 2001 From: Seth Tisue Date: Wed, 15 Jul 2015 22:16:19 -0400 Subject: tiny readme fix --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index e2174e8013..74a4ce97a8 100644 --- a/README.md +++ b/README.md @@ -169,7 +169,7 @@ to build the compiler. You can run `sbt test` to run unit (JUnit) tests. Use `sbt test/it:test` to run integration (partest) tests. We would like to migrate to sbt build as quickly as possible. If you would -like to help please contact scala-internals@ mailing list to discuss your +like to help please use the scala-internals mailing list to discuss your ideas and coordinate your effort with others. ### Tips and tricks -- cgit v1.2.3 From f9ca6863d46ede2b8ce4b7753c5c54120c0864ad Mon Sep 17 00:00:00 2001 From: Seth Tisue Date: Thu, 16 Jul 2015 08:08:42 -0400 Subject: readme/contributor's guide tweaks based on feedback from lrytz --- CONTRIBUTING.md | 9 --------- README.md | 12 +++++------- 2 files changed, 5 insertions(+), 16 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 1ea7e5513b..9471830571 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -49,18 +49,11 @@ A new language feature requires a SIP (Scala Improvement Process) proposal. For Here is some advice on how to craft a pull request with the best possible chance of being accepted. -### Merging - -A pull request should merge cleanly. (If enough time passes after -your initial submission, we may ask you to rebase it onto the current -mainline code.) - ### Tests Bug fixes should include regression tests -- in the same commit as the fix. If testing isn't feasible, the commit message should explain why. -(Consider discussing on scala-internals first.) New features and enhancements must be supported by a respectable test suite. @@ -137,5 +130,3 @@ A reviewer gives the green light by commenting "LGTM" (looks good to me). A review feedback may be addressed by pushing new commits to the request, if these commits stand on their own. Once all these conditions are met, and we agree with the change (we are available on scala-internals to discuss this beforehand, before you put in the coding work!), we will merge your changes. - -Please note: you are responsible for meeting these criteria (reminding your reviewer, for example). diff --git a/README.md b/README.md index 74a4ce97a8..45138a281b 100644 --- a/README.md +++ b/README.md @@ -25,20 +25,18 @@ If you need some help with your PR at any time, please feel free to @-mention an additional reviewer suggestions: -* library: @phaller (Philipp Haller), @axel22 (Aleksander Prokopec -- concurrent & collection) +* library: @axel22 (Aleksander Prokopec -- concurrent & collection) * specialisation: @axel22 (Aleksander Prokopec), @vladureche (Vlad Ureche), @dragos (Iulian Dragos) * named / default args, annotations, plugins: @lrytz (Lukas Rytz) -* macros, reflection, manifests, string interpolation: @xeno-by (Eugene Burmako), @cvogt (Christopher Vogt) -* type checker, inference: @odersky (Martin Odersky), @adriaanm (Adriaan Moors) -* language specification, value classes: @odersky (Martin Odersky) +* macros, reflection, manifests, string interpolation: @xeno-by (Eugene Burmako) +* type checker, inference: @adriaanm (Adriaan Moors) +* language specification: @adriaanm (Adriaan Moors) * new pattern matcher, implicit search: @adriaanm (Adriaan Moors) -* partest: @phaller (Philipp Haller) -* error handling, lazy vals: @hubertp (Hubert Plociniczak) * backend: @lrytz (Lukas Rytz), @retronym (Jason Zaugg), @dragos (Iulian Dragos) * repl, compiler performance: @retronym (Jason Zaugg) * scaladoc: @dickwall (Dick Wall), @vladureche (Vlad Ureche) * optimizer: @vladureche (Vlad Ureche), @lrytz (Lukas Rytz) -* build: @jsuereth (Josh Suereth) +* build: @SethTisue * random compiler bugs: @retronym, @lrytz, @adriaanm * documentation: @heathermiller (Heather Miller), @dickwall (Dick Wall) -- cgit v1.2.3 From e136e4ad473623cc6b88652589e1a38c2ccbf440 Mon Sep 17 00:00:00 2001 From: Seth Tisue Date: Thu, 16 Jul 2015 08:14:57 -0400 Subject: tighten up CONTRIBUTING.md a little --- CONTRIBUTING.md | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 9471830571..e6557d78dd 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -57,13 +57,11 @@ If testing isn't feasible, the commit message should explain why. New features and enhancements must be supported by a respectable test suite. -Consider including comments in the test files that indicate what you're testing and why. Expected outcome, what happened before the fix, what happens now, that sort of thing. - Some characteristics of good tests: +* includes comments: what is being tested and why? * be minimal, deterministic, stable (unaffected by irrelevant changes), easy to understand and review * have minimal dependencies: a compiler bug test should not depend on, e.g., the Scala library -* typically fail before your fix is applied (so we see that you are fixing a legitimate bug) and should obviously pass after your fix ### Documentation @@ -71,7 +69,7 @@ This is of course required for new features and enhancements. Any API additions should include Scaladoc. -Don't forget to update the package-level doc (in the package object), if appropriate. +Consider updating the package-level doc (in the package object), if appropriate. ### Coding standards @@ -106,7 +104,7 @@ When working on maintenance branches (e.g., 2.11.x), include "[nomerge]" if this commit should not be merged forward into the next release branch. -Here are is standard advice on good commit messages: +Here is standard advice on good commit messages: http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html ### Pass Scabot -- cgit v1.2.3 From e0aac7c9ef6d25a3283d3afe386c8d7da16ca941 Mon Sep 17 00:00:00 2001 From: Seth Tisue Date: Mon, 13 Jul 2015 16:02:50 -0400 Subject: bump copyright year to 2015 (just hitting the highlights here, not worrying yet about bumping the dates in every modified source file) --- build.sbt | 2 +- build.xml | 2 +- doc/LICENSE.md | 4 ++-- doc/License.rtf | 4 ++-- src/scaladoc/scala/tools/nsc/doc/html/page/Template.scala | 2 +- src/scalap/decoder.properties | 2 +- 6 files changed, 8 insertions(+), 8 deletions(-) diff --git a/build.sbt b/build.sbt index 1e3bef2541..7690df5430 100644 --- a/build.sbt +++ b/build.sbt @@ -131,7 +131,7 @@ lazy val setJarLocation: Setting[_] = lazy val scalaSubprojectSettings: Seq[Setting[_]] = commonSettings :+ setJarLocation lazy val generatePropertiesFileSettings = Seq[Setting[_]]( - copyrightString := "Copyright 2002-2013, LAMP/EPFL", + copyrightString := "Copyright 2002-2015, LAMP/EPFL", resourceGenerators in Compile += generateVersionPropertiesFile.map(file => Seq(file)).taskValue, generateVersionPropertiesFile := generateVersionPropertiesFileImpl.value ) diff --git a/build.xml b/build.xml index f595e464f8..ed4f3114d7 100755 --- a/build.xml +++ b/build.xml @@ -184,7 +184,7 @@ TODO: - + diff --git a/doc/LICENSE.md b/doc/LICENSE.md index 6b039afd68..55e82f64ba 100644 --- a/doc/LICENSE.md +++ b/doc/LICENSE.md @@ -2,9 +2,9 @@ Scala is licensed under the [BSD 3-Clause License](http://opensource.org/license ## Scala License -Copyright (c) 2002-2013 EPFL +Copyright (c) 2002-2015 EPFL -Copyright (c) 2011-2013 Typesafe, Inc. +Copyright (c) 2011-2015 Typesafe, Inc. All rights reserved. diff --git a/doc/License.rtf b/doc/License.rtf index 62ec2d023c..c475bda3ef 100644 --- a/doc/License.rtf +++ b/doc/License.rtf @@ -10,8 +10,8 @@ \fs48 Scala License \fs40 \ -\fs26 Copyright (c) 2002-2013 EPFL\ -Copyright (c) 2011-2013 Typesafe, Inc.\ +\fs26 Copyright (c) 2002-2015 EPFL\ +Copyright (c) 2011-2015 Typesafe, Inc.\ All rights reserved.\ \ Redistribution and use in source and binary forms, with or without modification, are permitted provided that the following conditions are met:\ diff --git a/src/scaladoc/scala/tools/nsc/doc/html/page/Template.scala b/src/scaladoc/scala/tools/nsc/doc/html/page/Template.scala index 81036b4908..7f47b38334 100644 --- a/src/scaladoc/scala/tools/nsc/doc/html/page/Template.scala +++ b/src/scaladoc/scala/tools/nsc/doc/html/page/Template.scala @@ -280,7 +280,7 @@ class Template(universe: doc.Universe, generator: DiagramGenerator, tpl: DocTemp { if (Set("epfl", "EPFL").contains(tpl.universe.settings.docfooter.value)) - + else } diff --git a/src/scalap/decoder.properties b/src/scalap/decoder.properties index 961c60f48c..333f6ce715 100644 --- a/src/scalap/decoder.properties +++ b/src/scalap/decoder.properties @@ -1,2 +1,2 @@ version.number=2.0.1 -copyright.string=(c) 2002-2013 LAMP/EPFL +copyright.string=(c) 2002-2015 LAMP/EPFL -- cgit v1.2.3 From 0b35bc29aa94b0b7ec280d01ffd55101ac18817a Mon Sep 17 00:00:00 2001 From: Lukas Rytz Date: Thu, 23 Jul 2015 13:42:53 +0200 Subject: Ignore OpenJDK warnings in partest filters When running a 2.11.x validation on JDK 8, there are OpenJDK warnings that fail tests. Backports a part of 8d2d3c70 to 2.11.x. The missing filter causes merge commits to fail in PRs that merge 2.11 to 2.12, e.g., https://github.com/scala/scala/pull/4655. In the future we probably want to trigger an integration build for merge commits, but this doesn't happen yet AFAICT. --- test/files/filters | 1 + test/scaladoc/filters | 1 + 2 files changed, 2 insertions(+) diff --git a/test/files/filters b/test/files/filters index 51a7507848..e91ca0eb36 100644 --- a/test/files/filters +++ b/test/files/filters @@ -1,6 +1,7 @@ # #Java HotSpot(TM) 64-Bit Server VM warning: Failed to reserve shared memory (errno = 28). Java HotSpot\(TM\) .* warning: +OpenJDK .* warning: # Hotspot receiving VM options through the $_JAVA_OPTIONS # env variable outputs them on stderr Picked up _JAVA_OPTIONS: diff --git a/test/scaladoc/filters b/test/scaladoc/filters index 51a7507848..e91ca0eb36 100644 --- a/test/scaladoc/filters +++ b/test/scaladoc/filters @@ -1,6 +1,7 @@ # #Java HotSpot(TM) 64-Bit Server VM warning: Failed to reserve shared memory (errno = 28). Java HotSpot\(TM\) .* warning: +OpenJDK .* warning: # Hotspot receiving VM options through the $_JAVA_OPTIONS # env variable outputs them on stderr Picked up _JAVA_OPTIONS: -- cgit v1.2.3 From 44e2761a9bbe942a96f5092b2a39b5a696aaabbd Mon Sep 17 00:00:00 2001 From: Lukas Rytz Date: Thu, 25 Jun 2015 16:53:46 +0200 Subject: [backport] SI-6613 fixed in GenBCode It was fixed in GenASM in 44807a7852. --- src/compiler/scala/tools/nsc/backend/jvm/BCodeHelpers.scala | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/compiler/scala/tools/nsc/backend/jvm/BCodeHelpers.scala b/src/compiler/scala/tools/nsc/backend/jvm/BCodeHelpers.scala index 6aa3a62295..23e0a4e17a 100644 --- a/src/compiler/scala/tools/nsc/backend/jvm/BCodeHelpers.scala +++ b/src/compiler/scala/tools/nsc/backend/jvm/BCodeHelpers.scala @@ -329,7 +329,8 @@ abstract class BCodeHelpers extends BCodeIdiomatic with BytecodeWriters { // If the `sym` is a java module class, we use the java class instead. This ensures that we // register the class (instead of the module class) in innerClassBufferASM. // The two symbols have the same name, so the resulting internalName is the same. - val classSym = if (sym.isJavaDefined && sym.isModuleClass) sym.linkedClassOfClass else sym + // Phase travel (exitingPickler) required for SI-6613 - linkedCoC is only reliable in early phases (nesting) + val classSym = if (sym.isJavaDefined && sym.isModuleClass) exitingPickler(sym.linkedClassOfClass) else sym getClassBTypeAndRegisterInnerClass(classSym).internalName } -- cgit v1.2.3 From 8bafa8ed88da0d522425c120daf7e0a4e09f88d9 Mon Sep 17 00:00:00 2001 From: Lukas Rytz Date: Tue, 30 Jun 2015 14:12:43 +0200 Subject: [backport] Java parser: default methods in interfaces are not `DEFERRED` The Java parser should not set the `DEFERRED` flag for default methods or static methods in interfaces. Their bytecode doesn't have it either. Also tightens parsing of Java abstract methods to disallow a method body. Here's the log of how Lukas diagnosed this: ``` quick.bin: ... BUILD FAILED /Users/luc/scala/scala/build.xml:69: The following error occurred while executing this line: ... /Users/luc/scala/scala/build-ant-macros.xml:350: Could not create type mk-bin due to java.lang.BootstrapMethodError: call site initialization exception at java.lang.invoke.CallSite.makeSite(CallSite.java:341) at java.lang.invoke.MethodHandleNatives.linkCallSiteImpl(MethodHandleNatives.java:307) at java.lang.invoke.MethodHandleNatives.linkCallSite(MethodHandleNatives.java:297) at scala.sys.BooleanProp$.keyExists(BooleanProp.scala:72) at scala.sys.SystemProperties$.bool(SystemProperties.scala:78) at scala.sys.SystemProperties$.noTraceSupression$lzycompute(SystemProperties.scala:89) at scala.sys.SystemProperties$.noTraceSupression(SystemProperties.scala:89) at scala.util.control.NoStackTrace$.(NoStackTrace.scala:31) at scala.util.control.NoStackTrace$.(NoStackTrace.scala) at scala.util.control.NoStackTrace$class.fillInStackTrace(NoStackTrace.scala:22) at scala.util.control.BreakControl.fillInStackTrace(Breaks.scala:94) at java.lang.Throwable.(Throwable.java:250) at scala.util.control.BreakControl.(Breaks.scala:94) at scala.util.control.Breaks.(Breaks.scala:29) at scala.collection.Traversable$.(Traversable.scala:95) at scala.collection.Traversable$.(Traversable.scala) at scala.package$.(package.scala:40) at scala.package$.(package.scala) at scala.Predef$.(Predef.scala:89) at scala.Predef$.(Predef.scala) at scala.tools.ant.ScalaTool.(ScalaTool.scala:58) [...] Caused by: java.lang.invoke.LambdaConversionException: Incorrect number of parameters for static method invokeStatic scala.sys.BooleanProp$.scala$sys$BooleanProp$$$anonfun$2$adapted:(String)Object; 0 captured parameters, 0 functional interface method parameters, 1 implementation parameters at java.lang.invoke.AbstractValidatingLambdaMetafactory.validateMetafactoryArgs(AbstractValidatingLambdaMetafactory.java:193) at java.lang.invoke.LambdaMetafactory.altMetafactory(LambdaMetafactory.java:473) at java.lang.invoke.CallSite.makeSite(CallSite.java:325) ``` [source code](https://github.com/scala/scala/blob/2.11.x/src/library/scala/sys/BooleanProp.scala#L72): ``` s => s == "" || s.equalsIgnoreCase("true") ``` bytecode: ``` INVOKEDYNAMIC $init$()Lscala/compat/java8/JFunction1; [ // handle kind 0x6 : INVOKESTATIC java/lang/invoke/LambdaMetafactory.altMetafactory(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/MethodType;[Ljava/lang/Object;)Ljava/lang/invoke/CallSite; // arguments: ()V, // handle kind 0x6 : INVOKESTATIC scala/sys/BooleanProp$.scala$sys$BooleanProp$$$anonfun$2$adapted(Ljava/lang/String;)Ljava/lang/Object;, (Ljava/lang/String;)Ljava/lang/Object;, 3, 1, Lscala/Serializable;.class, 0 ] CHECKCAST scala/Function1 ``` The mistake seems to be that the Scala compiler incorrectly selects `$init$` ([which is a default method](https://github.com/scala/scala/blob/640ffe7fceb5d573b2c12a7c7da09bfd751036a0/src/library/scala/compat/java8/JFunction1.java#L10)) as the abstract method of `JFunction1`, whereas it should be `apply` (inherited from `Function1`). Since we're doing mixed compilation, this is almost certainly a problem of the Java parser. --- src/compiler/scala/tools/nsc/javac/JavaParsers.scala | 8 +++++--- test/files/neg/t6013/Base.java | 2 +- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/compiler/scala/tools/nsc/javac/JavaParsers.scala b/src/compiler/scala/tools/nsc/javac/JavaParsers.scala index 9708cba281..03f0236734 100644 --- a/src/compiler/scala/tools/nsc/javac/JavaParsers.scala +++ b/src/compiler/scala/tools/nsc/javac/JavaParsers.scala @@ -489,8 +489,8 @@ trait JavaParsers extends ast.parser.ParsersCommon with JavaScanners { val vparams = formalParams() if (!isVoid) rtpt = optArrayBrackets(rtpt) optThrows() - val isStatic = mods hasFlag Flags.STATIC - val bodyOk = !inInterface || ((mods hasFlag Flags.DEFAULTMETHOD) || isStatic) + val isConcreteInterfaceMethod = !inInterface || (mods hasFlag Flags.DEFAULTMETHOD) || (mods hasFlag Flags.STATIC) + val bodyOk = !(mods1 hasFlag Flags.DEFERRED) && isConcreteInterfaceMethod val body = if (bodyOk && in.token == LBRACE) { methodBody() @@ -509,7 +509,9 @@ trait JavaParsers extends ast.parser.ParsersCommon with JavaScanners { EmptyTree } } - if (inInterface && !isStatic) mods1 |= Flags.DEFERRED + // for abstract methods (of classes), the `DEFERRED` flag is alredy set. + // here we also set it for interface methods that are not static and not default. + if (!isConcreteInterfaceMethod) mods1 |= Flags.DEFERRED List { atPos(pos) { DefDef(mods1, name.toTermName, tparams, List(vparams), rtpt, body) diff --git a/test/files/neg/t6013/Base.java b/test/files/neg/t6013/Base.java index b73d7fd821..ce6ee47e64 100644 --- a/test/files/neg/t6013/Base.java +++ b/test/files/neg/t6013/Base.java @@ -2,7 +2,7 @@ abstract public class Base { // This must considered to be overridden by Abstract#foo based // on the erased signatures. This special case is handled by // `javaErasedOverridingSym` in `RefChecks`. - public abstract void bar(java.util.List foo) { return; } + public void bar(java.util.List foo) { return; } // But, a concrete method in a Java superclass must not excuse // a deferred method in the Java subclass! -- cgit v1.2.3 From 1b5772316911e68ac0fb15a17390277f520e823a Mon Sep 17 00:00:00 2001 From: Lukas Rytz Date: Tue, 30 Jun 2015 22:09:39 +0200 Subject: [backport] `deserializeLambda` should not use encoded class name `javaBinaryName` returns the internal name of a class. Also used in BTypesFromsymbols.classBTypeFromSymbol. Weirdly, this was discovered due to a bizarre osgi bnd error: ``` [bnd] # addAll '/Users/luc/scala/scala/build/pack/lib/scala-library.jar' with :, [bnd] # addAll '/Users/luc/scala/scala/build/osgi/scala-library.bnd' with , [bnd] 1 ERRORS [bnd] The default package '.' is not permitted by the Import-Package syntax. [bnd] This can be caused by compile errors in Eclipse because Eclipse creates [bnd] valid class files regardless of compile errors. [bnd] The following package(s) import from the default package [scala.collection.generic, scala.sys.process, scala.collection.parallel.mutable, scala.util, scala.collection.parallel.immutable, scala.reflect, scala.concurrent.impl, scala.util.hashing, scala.collection.parallel, scala.collection.convert, scala.io, scala, scala.collection.concurrent, scala.util.control, scala.beans, scala.concurrent.duration, scala.collection, scala.runtime, scala.math, scala.collection.mutable, scala.concurrent, scala.sys, scala.collection.immutable, scala.ref, scala.util.matching] [bnd] /Users/luc/scala/scala/build/osgi/scala-library.bnd: bnd failed ``` Lukas diagnosed it as a problem of the generated `$deserializeLambda$` function: One example is `scala/App$class`. Its bytecode contains this: ``` private static synthetic $deserializeLambda$(Ljava/lang/invoke/SerializedLambda;)Ljava/lang/Object; GETSTATIC scala$divApp$class.$deserializeLambdaCache$ : Ljava/util/Map; [...] ``` so it's a static field read of a top-level class. `$div` should obviously be `/` (which this commit rectifies) --- src/compiler/scala/tools/nsc/backend/jvm/BCodeHelpers.scala | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/compiler/scala/tools/nsc/backend/jvm/BCodeHelpers.scala b/src/compiler/scala/tools/nsc/backend/jvm/BCodeHelpers.scala index 23e0a4e17a..d53e88a11c 100644 --- a/src/compiler/scala/tools/nsc/backend/jvm/BCodeHelpers.scala +++ b/src/compiler/scala/tools/nsc/backend/jvm/BCodeHelpers.scala @@ -715,7 +715,8 @@ abstract class BCodeHelpers extends BCodeIdiomatic with BytecodeWriters { { val mv = cw.visitMethod(ACC_PRIVATE + ACC_STATIC + ACC_SYNTHETIC, "$deserializeLambda$", "(Ljava/lang/invoke/SerializedLambda;)Ljava/lang/Object;", null, null) mv.visitCode() - mv.visitFieldInsn(GETSTATIC, clazz.javaBinaryName.encoded, "$deserializeLambdaCache$", "Ljava/util/Map;") + // javaBinaryName returns the internal name of a class. Also used in BTypesFromsymbols.classBTypeFromSymbol. + mv.visitFieldInsn(GETSTATIC, clazz.javaBinaryName.toString, "$deserializeLambdaCache$", "Ljava/util/Map;") mv.visitVarInsn(ASTORE, 1) mv.visitVarInsn(ALOAD, 1) val l0 = new asm.Label() @@ -725,7 +726,7 @@ abstract class BCodeHelpers extends BCodeIdiomatic with BytecodeWriters { mv.visitMethodInsn(INVOKESPECIAL, "java/util/HashMap", "", "()V", false) mv.visitVarInsn(ASTORE, 1) mv.visitVarInsn(ALOAD, 1) - mv.visitFieldInsn(PUTSTATIC, clazz.javaBinaryName.encoded, "$deserializeLambdaCache$", "Ljava/util/Map;") + mv.visitFieldInsn(PUTSTATIC, clazz.javaBinaryName.toString, "$deserializeLambdaCache$", "Ljava/util/Map;") mv.visitLabel(l0) mv.visitFrame(asm.Opcodes.F_APPEND,1, Array("java/util/Map"), 0, null) mv.visitMethodInsn(INVOKESTATIC, "java/lang/invoke/MethodHandles", "lookup", "()Ljava/lang/invoke/MethodHandles$Lookup;", false) -- cgit v1.2.3 From e511375a902e19cbed2340e7b66272692307df93 Mon Sep 17 00:00:00 2001 From: Lukas Rytz Date: Thu, 2 Jul 2015 16:11:50 +0200 Subject: [backport] Fix superclass for Java interface symbols created in JavaMirrors According to the spec [1] the superclass of an interface is always Object. Restores the tests that were moved to pending in bf951ec1, fixex part of SI-9374. [1] https://docs.oracle.com/javase/specs/jvms/se8/html/jvms-4.html#jvms-4.1 --- src/compiler/scala/tools/nsc/symtab/classfile/ClassfileParser.scala | 2 +- src/reflect/scala/reflect/runtime/JavaMirrors.scala | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/compiler/scala/tools/nsc/symtab/classfile/ClassfileParser.scala b/src/compiler/scala/tools/nsc/symtab/classfile/ClassfileParser.scala index 660028eab8..91355693ee 100644 --- a/src/compiler/scala/tools/nsc/symtab/classfile/ClassfileParser.scala +++ b/src/compiler/scala/tools/nsc/symtab/classfile/ClassfileParser.scala @@ -284,7 +284,7 @@ abstract class ClassfileParser { def getType(index: Int): Type = getType(null, index) def getType(sym: Symbol, index: Int): Type = sigToType(sym, getExternalName(index)) - def getSuperClass(index: Int): Symbol = if (index == 0) AnyClass else getClassSymbol(index) + def getSuperClass(index: Int): Symbol = if (index == 0) AnyClass else getClassSymbol(index) // the only classfile that is allowed to have `0` in the super_class is java/lang/Object (see jvm spec) private def createConstant(index: Int): Constant = { val start = starts(index) diff --git a/src/reflect/scala/reflect/runtime/JavaMirrors.scala b/src/reflect/scala/reflect/runtime/JavaMirrors.scala index 8c32a92ecd..d0670f337a 100644 --- a/src/reflect/scala/reflect/runtime/JavaMirrors.scala +++ b/src/reflect/scala/reflect/runtime/JavaMirrors.scala @@ -755,6 +755,7 @@ private[scala] trait JavaMirrors extends internal.SymbolTable with api.JavaUnive val ifaces = jclazz.getGenericInterfaces.toList map typeToScala val isAnnotation = JavaAccFlags(jclazz).isAnnotation if (isAnnotation) AnnotationClass.tpe :: ClassfileAnnotationClass.tpe :: ifaces + else if (jclazz.isInterface) ObjectTpe :: ifaces // interfaces have Object as superclass in the classfile (see jvm spec), but getGenericSuperclass seems to return null else (if (jsuperclazz == null) AnyTpe else typeToScala(jsuperclazz)) :: ifaces } finally { parentsLevel -= 1 -- cgit v1.2.3 From 1b0703e74d22ed54b95a134216835569a20d2325 Mon Sep 17 00:00:00 2001 From: Lukas Rytz Date: Thu, 2 Jul 2015 20:17:57 +0200 Subject: [backport] SI-9376 don't crash when inlining a closure body that throws. If the closure body method has return type Nothing$, add an `ATHROW` instruction after the callsite. This is required for computing stack map frames, as explained in a comment in BCodeBodyBuilder.adapt. Similar for closure bodies with return type Null$. --- .../tools/nsc/backend/jvm/opt/BytecodeUtils.scala | 22 +++++++++++++++++++++- .../nsc/backend/jvm/opt/ClosureOptimizer.scala | 4 ++++ 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/src/compiler/scala/tools/nsc/backend/jvm/opt/BytecodeUtils.scala b/src/compiler/scala/tools/nsc/backend/jvm/opt/BytecodeUtils.scala index 0ec550981a..cd36fd8bba 100644 --- a/src/compiler/scala/tools/nsc/backend/jvm/opt/BytecodeUtils.scala +++ b/src/compiler/scala/tools/nsc/backend/jvm/opt/BytecodeUtils.scala @@ -12,7 +12,7 @@ import scala.collection.mutable import scala.reflect.internal.util.Collections._ import scala.tools.asm.commons.CodeSizeEvaluator import scala.tools.asm.tree.analysis._ -import scala.tools.asm.{MethodWriter, ClassWriter, Label, Opcodes} +import scala.tools.asm.{MethodWriter, ClassWriter, Label, Opcodes, Type} import scala.tools.asm.tree._ import GenBCode._ import scala.collection.convert.decorateAsScala._ @@ -330,6 +330,26 @@ object BytecodeUtils { )).toList } + /** + * This method is used by optimizer components to eliminate phantom values of instruction + * that load a value of type `Nothing$` or `Null$`. Such values on the stack don't interact well + * with stack map frames. + * + * For example, `opt.getOrElse(throw e)` is re-written to an invocation of the lambda body, a + * method with return type `Nothing$`. Similarly for `opt.getOrElse(null)` and `Null$`. + * + * During bytecode generation this is handled by BCodeBodyBuilder.adapt. See the comment in that + * method which explains the issue with such phantom values. + */ + def fixLoadedNothingOrNullValue(loadedType: Type, loadInstr: AbstractInsnNode, methodNode: MethodNode, bTypes: BTypes): Unit = { + if (loadedType == bTypes.coreBTypes.RT_NOTHING.toASMType) { + methodNode.instructions.insert(loadInstr, new InsnNode(Opcodes.ATHROW)) + } else if (loadedType == bTypes.coreBTypes.RT_NULL.toASMType) { + methodNode.instructions.insert(loadInstr, new InsnNode(Opcodes.ACONST_NULL)) + methodNode.instructions.insert(loadInstr, new InsnNode(Opcodes.POP)) + } + } + /** * A wrapper to make ASM's Analyzer a bit easier to use. */ diff --git a/src/compiler/scala/tools/nsc/backend/jvm/opt/ClosureOptimizer.scala b/src/compiler/scala/tools/nsc/backend/jvm/opt/ClosureOptimizer.scala index 1648a53ed8..743a454678 100644 --- a/src/compiler/scala/tools/nsc/backend/jvm/opt/ClosureOptimizer.scala +++ b/src/compiler/scala/tools/nsc/backend/jvm/opt/ClosureOptimizer.scala @@ -283,6 +283,10 @@ class ClosureOptimizer[BT <: BTypes](val btypes: BT) { val isInterface = bodyOpcode == INVOKEINTERFACE val bodyInvocation = new MethodInsnNode(bodyOpcode, lambdaBodyHandle.getOwner, lambdaBodyHandle.getName, lambdaBodyHandle.getDesc, isInterface) methodNode.instructions.insertBefore(invocation, bodyInvocation) + + val returnType = Type.getReturnType(lambdaBodyHandle.getDesc) + fixLoadedNothingOrNullValue(returnType, bodyInvocation, methodNode, btypes) // see comment of that method + methodNode.instructions.remove(invocation) // update the call graph -- cgit v1.2.3 From 404e86239e445307fdff4fb0cb4e512993ac56b8 Mon Sep 17 00:00:00 2001 From: Lukas Rytz Date: Fri, 3 Jul 2015 08:25:30 +0200 Subject: [backport] Prevent infinite recursion in ProdConsAnalyzer When an instruction is its own producer or consumer, the `initialProducer` / `ultimateConsumer` methods would loop. While loops or @tailrec annotated methods can generate such bytecode. --- .../scala/tools/nsc/backend/jvm/AsmUtils.scala | 18 ++++++---- .../backend/jvm/analysis/ProdConsAnalyzer.scala | 12 +++++-- .../jvm/analysis/ProdConsAnalyzerTest.scala | 42 ++++++++++++++++++++++ 3 files changed, 63 insertions(+), 9 deletions(-) diff --git a/src/compiler/scala/tools/nsc/backend/jvm/AsmUtils.scala b/src/compiler/scala/tools/nsc/backend/jvm/AsmUtils.scala index 0df1b2029d..cd7e0b83e8 100644 --- a/src/compiler/scala/tools/nsc/backend/jvm/AsmUtils.scala +++ b/src/compiler/scala/tools/nsc/backend/jvm/AsmUtils.scala @@ -10,6 +10,7 @@ import java.io.{StringWriter, PrintWriter} import scala.tools.asm.util.{CheckClassAdapter, TraceClassVisitor, TraceMethodVisitor, Textifier} import scala.tools.asm.{ClassWriter, Attribute, ClassReader} import scala.collection.convert.decorateAsScala._ +import scala.tools.nsc.backend.jvm.analysis.InitialProducer import scala.tools.nsc.backend.jvm.opt.InlineInfoAttributePrototype object AsmUtils { @@ -81,13 +82,16 @@ object AsmUtils { /** * Returns a human-readable representation of the given instruction. */ - def textify(insn: AbstractInsnNode): String = { - val trace = new TraceMethodVisitor(new Textifier) - insn.accept(trace) - val sw = new StringWriter - val pw = new PrintWriter(sw) - trace.p.print(pw) - sw.toString.trim + def textify(insn: AbstractInsnNode): String = insn match { + case _: InitialProducer => + insn.toString + case _ => + val trace = new TraceMethodVisitor(new Textifier) + insn.accept(trace) + val sw = new StringWriter + val pw = new PrintWriter(sw) + trace.p.print(pw) + sw.toString.trim } /** diff --git a/src/compiler/scala/tools/nsc/backend/jvm/analysis/ProdConsAnalyzer.scala b/src/compiler/scala/tools/nsc/backend/jvm/analysis/ProdConsAnalyzer.scala index 40f91cbed4..cf63bf54a4 100644 --- a/src/compiler/scala/tools/nsc/backend/jvm/analysis/ProdConsAnalyzer.scala +++ b/src/compiler/scala/tools/nsc/backend/jvm/analysis/ProdConsAnalyzer.scala @@ -103,7 +103,11 @@ class ProdConsAnalyzer(methodNode: MethodNode, classInternalName: InternalName) def initialProducersForValueAt(insn: AbstractInsnNode, slot: Int): Set[AbstractInsnNode] = { def initialProducers(insn: AbstractInsnNode, producedSlot: Int): Set[AbstractInsnNode] = { if (isCopyOperation(insn)) { - _initialProducersCache.getOrElseUpdate((insn, producedSlot), { + val key = (insn, producedSlot) + _initialProducersCache.getOrElseUpdate(key, { + // prevent infinite recursion if an instruction is its own producer or consumer + // see cyclicProdCons in ProdConsAnalyzerTest + _initialProducersCache(key) = Set.empty val (sourceValue, sourceValueSlot) = copyOperationSourceValue(insn, producedSlot) sourceValue.insns.iterator.asScala.flatMap(initialProducers(_, sourceValueSlot)).toSet }) @@ -121,7 +125,11 @@ class ProdConsAnalyzer(methodNode: MethodNode, classInternalName: InternalName) def ultimateConsumersOfValueAt(insn: AbstractInsnNode, slot: Int): Set[AbstractInsnNode] = { def ultimateConsumers(insn: AbstractInsnNode, consumedSlot: Int): Set[AbstractInsnNode] = { if (isCopyOperation(insn)) { - _ultimateConsumersCache.getOrElseUpdate((insn, consumedSlot), { + val key = (insn, consumedSlot) + _ultimateConsumersCache.getOrElseUpdate(key, { + // prevent infinite recursion if an instruction is its own producer or consumer + // see cyclicProdCons in ProdConsAnalyzerTest + _ultimateConsumersCache(key) = Set.empty for { producedSlot <- copyOperationProducedValueSlots(insn, consumedSlot) consumer <- consumersOfValueAt(insn.getNext, producedSlot) diff --git a/test/junit/scala/tools/nsc/backend/jvm/analysis/ProdConsAnalyzerTest.scala b/test/junit/scala/tools/nsc/backend/jvm/analysis/ProdConsAnalyzerTest.scala index 9af9ef54fc..a5b3faced8 100644 --- a/test/junit/scala/tools/nsc/backend/jvm/analysis/ProdConsAnalyzerTest.scala +++ b/test/junit/scala/tools/nsc/backend/jvm/analysis/ProdConsAnalyzerTest.scala @@ -246,4 +246,46 @@ class ProdConsAnalyzerTest extends ClearAfterClass { testSingleInsn(a.consumersOfOutputsFrom(l2i), "IRETURN") testSingleInsn(a.producersForInputsOf(ret), "L2I") } + + @Test + def cyclicProdCons(): Unit = { + import Opcodes._ + val m = genMethod(descriptor = "(I)I")( + Label(1), + VarOp(ILOAD, 1), + IntOp(BIPUSH, 10), + Op(IADD), // consumer of the above ILOAD + + Op(ICONST_0), + Jump(IF_ICMPNE, Label(2)), + + VarOp(ILOAD, 1), + VarOp(ISTORE, 1), + Jump(GOTO, Label(1)), + + Label(2), + IntOp(BIPUSH, 9), + Op(IRETURN) + ) + m.maxLocals = 2 + m.maxStack = 2 + val a = new ProdConsAnalyzer(m, "C") + + val List(iadd) = findInstr(m, "IADD") + val firstLoad = iadd.getPrevious.getPrevious + assert(firstLoad.getOpcode == ILOAD) + val secondLoad = findInstr(m, "ISTORE").head.getPrevious + assert(secondLoad.getOpcode == ILOAD) + + testSingleInsn(a.producersForValueAt(iadd, 2), "ILOAD") + testSingleInsn(a.initialProducersForValueAt(iadd, 2), "ParameterProducer(1)") + testMultiInsns(a.producersForInputsOf(firstLoad), List("ParameterProducer", "ISTORE")) + testMultiInsns(a.producersForInputsOf(secondLoad), List("ParameterProducer", "ISTORE")) + + testSingleInsn(a.ultimateConsumersOfOutputsFrom(firstLoad), "IADD") + testSingleInsn(a.ultimateConsumersOfOutputsFrom(secondLoad), "IADD") + + testSingleInsn(a.consumersOfOutputsFrom(firstLoad), "IADD") + testSingleInsn(a.consumersOfOutputsFrom(secondLoad), "ISTORE") + } } -- cgit v1.2.3 From 60747c75555fdcfefc9d10460050be5e2a11ad85 Mon Sep 17 00:00:00 2001 From: Lukas Rytz Date: Fri, 3 Jul 2015 10:33:21 +0200 Subject: [backport] Skip mirror class when invoking deserializeLambda Generate the invocation to LambdaDeserializer.deserializeLambda by loading the static MODULE$ field instead of calling the static method in the mirror class. This is more scala-y. Also, mirror classes don't have an InlineInfo classfile attribute, so the inliner would yield a warning about the mirror class callsite. Also skip the stack map frame instruction - frames are computed by the ams classfile writer. --- src/compiler/scala/tools/nsc/backend/jvm/BCodeHelpers.scala | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/compiler/scala/tools/nsc/backend/jvm/BCodeHelpers.scala b/src/compiler/scala/tools/nsc/backend/jvm/BCodeHelpers.scala index d53e88a11c..65a6b82570 100644 --- a/src/compiler/scala/tools/nsc/backend/jvm/BCodeHelpers.scala +++ b/src/compiler/scala/tools/nsc/backend/jvm/BCodeHelpers.scala @@ -728,11 +728,11 @@ abstract class BCodeHelpers extends BCodeIdiomatic with BytecodeWriters { mv.visitVarInsn(ALOAD, 1) mv.visitFieldInsn(PUTSTATIC, clazz.javaBinaryName.toString, "$deserializeLambdaCache$", "Ljava/util/Map;") mv.visitLabel(l0) - mv.visitFrame(asm.Opcodes.F_APPEND,1, Array("java/util/Map"), 0, null) + mv.visitFieldInsn(GETSTATIC, "scala/compat/java8/runtime/LambdaDeserializer$", "MODULE$", "Lscala/compat/java8/runtime/LambdaDeserializer$;") mv.visitMethodInsn(INVOKESTATIC, "java/lang/invoke/MethodHandles", "lookup", "()Ljava/lang/invoke/MethodHandles$Lookup;", false) mv.visitVarInsn(ALOAD, 1) mv.visitVarInsn(ALOAD, 0) - mv.visitMethodInsn(INVOKESTATIC, "scala/compat/java8/runtime/LambdaDeserializer", "deserializeLambda", "(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/util/Map;Ljava/lang/invoke/SerializedLambda;)Ljava/lang/Object;", false) + mv.visitMethodInsn(INVOKEVIRTUAL, "scala/compat/java8/runtime/LambdaDeserializer$", "deserializeLambda", "(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/util/Map;Ljava/lang/invoke/SerializedLambda;)Ljava/lang/Object;", false) mv.visitInsn(ARETURN) mv.visitEnd() } -- cgit v1.2.3 From ef9d84567655d6f8c9b8541aef4beb706f2a13ec Mon Sep 17 00:00:00 2001 From: Lukas Rytz Date: Mon, 6 Jul 2015 20:13:42 +0200 Subject: [backport] Support methodHandle / invokeDynamic constant pool entries in scalap Add support in scalap to parse new constant pool entries - MethodHandle - MethodType - InvokeDynamic Spec: https://docs.oracle.com/javase/specs/jvms/se8/html/jvms-4.html --- .../tools/scalap/scalax/rules/scalasig/ClassFileParser.scala | 6 ++++++ test/files/run/scalapInvokedynamic.check | 5 +++++ test/files/run/scalapInvokedynamic.scala | 11 +++++++++++ 3 files changed, 22 insertions(+) create mode 100644 test/files/run/scalapInvokedynamic.check create mode 100644 test/files/run/scalapInvokedynamic.scala diff --git a/src/scalap/scala/tools/scalap/scalax/rules/scalasig/ClassFileParser.scala b/src/scalap/scala/tools/scalap/scalax/rules/scalasig/ClassFileParser.scala index cfd750055b..eed76c3774 100644 --- a/src/scalap/scala/tools/scalap/scalax/rules/scalasig/ClassFileParser.scala +++ b/src/scalap/scala/tools/scalap/scalax/rules/scalasig/ClassFileParser.scala @@ -114,6 +114,9 @@ object ClassFileParser extends ByteCodeReader { val methodRef = memberRef("Method") val interfaceMethodRef = memberRef("InterfaceMethod") val nameAndType = u2 ~ u2 ^^ add1 { case name ~ descriptor => pool => "NameAndType: " + pool(name) + ", " + pool(descriptor) } + val methodHandle = u1 ~ u2 ^^ add1 { case referenceKind ~ referenceIndex => pool => "MethodHandle: " + referenceKind + ", " + pool(referenceIndex) } + val methodType = u2 ^^ add1 { case descriptorIndex => pool => "MethodType: " + pool(descriptorIndex) } + val invokeDynamic = u2 ~ u2 ^^ add1 { case bootstrapMethodAttrIndex ~ nameAndTypeIndex => pool => "InvokeDynamic: " + "bootstrapMethodAttrIndex = " + bootstrapMethodAttrIndex + ", " + pool(nameAndTypeIndex) } val constantPoolEntry = u1 >> { case 1 => utf8String @@ -127,6 +130,9 @@ object ClassFileParser extends ByteCodeReader { case 10 => methodRef case 11 => interfaceMethodRef case 12 => nameAndType + case 15 => methodHandle + case 16 => methodType + case 18 => invokeDynamic } val interfaces = u2 >> u2.times diff --git a/test/files/run/scalapInvokedynamic.check b/test/files/run/scalapInvokedynamic.check new file mode 100644 index 0000000000..8e4b08f234 --- /dev/null +++ b/test/files/run/scalapInvokedynamic.check @@ -0,0 +1,5 @@ +class C extends scala.AnyRef { + def this() = { /* compiled code */ } + def m: java.lang.String = { /* compiled code */ } +} + diff --git a/test/files/run/scalapInvokedynamic.scala b/test/files/run/scalapInvokedynamic.scala new file mode 100644 index 0000000000..670cf26662 --- /dev/null +++ b/test/files/run/scalapInvokedynamic.scala @@ -0,0 +1,11 @@ +class C { + def m = { + val f = (x: String) => x.trim + f(" H ae i ") + } +} + +object Test extends App { + val testClassesDir = System.getProperty("partest.output") + scala.tools.scalap.Main.main(Array("-cp", testClassesDir, "C")) +} \ No newline at end of file -- cgit v1.2.3 From 1c1d8259b51ede24ccad116ce6f0590d11426a34 Mon Sep 17 00:00:00 2001 From: Lukas Rytz Date: Tue, 7 Jul 2015 10:04:36 +0200 Subject: [backport] Fix bytecode stability When there are multiple closure allocations and invocations in the same method, ensure that the callsites are re-written to the body methods in a consistent order. Otherwsie the bytecode is not stable (the local variable indices depend on the order in which the calls are re-written) --- .../nsc/backend/jvm/opt/ClosureOptimizer.scala | 25 ++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/src/compiler/scala/tools/nsc/backend/jvm/opt/ClosureOptimizer.scala b/src/compiler/scala/tools/nsc/backend/jvm/opt/ClosureOptimizer.scala index 743a454678..8da209b269 100644 --- a/src/compiler/scala/tools/nsc/backend/jvm/opt/ClosureOptimizer.scala +++ b/src/compiler/scala/tools/nsc/backend/jvm/opt/ClosureOptimizer.scala @@ -24,8 +24,29 @@ class ClosureOptimizer[BT <: BTypes](val btypes: BT) { import callGraph._ def rewriteClosureApplyInvocations(): Unit = { - closureInstantiations foreach { - case (indy, (methodNode, ownerClass)) => + implicit object closureInitOrdering extends Ordering[(InvokeDynamicInsnNode, MethodNode, ClassBType)] { + // Note: this code is cleaned up in a future commit, no more tuples. + override def compare(x: (InvokeDynamicInsnNode, MethodNode, ClassBType), y: (InvokeDynamicInsnNode, MethodNode, ClassBType)): Int = { + val cls = x._3.internalName compareTo y._3.internalName + if (cls != 0) return cls + + val mName = x._2.name compareTo y._2.name + if (mName != 0) return mName + + val mDesc = x._2.desc compareTo y._2.desc + if (mDesc != 0) return mDesc + + def pos(indy: InvokeDynamicInsnNode) = x._2.instructions.indexOf(indy) + pos(x._1) - pos(y._1) + } + } + + val sorted = closureInstantiations.iterator.map({ + case (indy, (methodNode, ownerClass)) => (indy, methodNode, ownerClass) + }).to[collection.immutable.TreeSet] + + sorted foreach { + case (indy, methodNode, ownerClass) => val warnings = rewriteClosureApplyInvocations(indy, methodNode, ownerClass) warnings.foreach(w => backendReporting.inlinerWarning(w.pos, w.toString)) } -- cgit v1.2.3 From 8f272c0ad2d1435b4032188eb2e4cba641605dd4 Mon Sep 17 00:00:00 2001 From: Lukas Rytz Date: Mon, 6 Jul 2015 20:12:53 +0200 Subject: [backport] Accessibility checks for methods with an InvokeDynamic instruction Implements the necessary tests to check if a method with an InvokeDynamic instruction can be inlined into a destination class. Only InvokeDynamic instructions with LambdaMetaFactory as bootstrap methods can be inlined. The accessibility checks cannot be implemented generically, because it depends on what the bootstrap method is doing. In particular, the bootstrap method receives a Lookup object as argument which can be used to access private methods of the class where the InvokeDynamic method is located. A comment in the inliner explains the details. --- .../tools/nsc/backend/jvm/BackendReporting.scala | 7 +- .../scala/tools/nsc/backend/jvm/opt/Inliner.scala | 65 +++++++++++++++++- .../scala/tools/partest/ASMConverters.scala | 78 +++++++++++++--------- 3 files changed, 116 insertions(+), 34 deletions(-) diff --git a/src/compiler/scala/tools/nsc/backend/jvm/BackendReporting.scala b/src/compiler/scala/tools/nsc/backend/jvm/BackendReporting.scala index 4fc05cafdc..4eb24d13e3 100644 --- a/src/compiler/scala/tools/nsc/backend/jvm/BackendReporting.scala +++ b/src/compiler/scala/tools/nsc/backend/jvm/BackendReporting.scala @@ -1,7 +1,7 @@ package scala.tools.nsc package backend.jvm -import scala.tools.asm.tree.{AbstractInsnNode, MethodNode} +import scala.tools.asm.tree.{InvokeDynamicInsnNode, AbstractInsnNode, MethodNode} import scala.tools.nsc.backend.jvm.BTypes.InternalName import scala.reflect.internal.util.Position import scala.tools.nsc.settings.ScalaSettings @@ -246,6 +246,11 @@ object BackendReporting { case class ResultingMethodTooLarge(calleeDeclarationClass: InternalName, name: String, descriptor: String, callsiteClass: InternalName, callsiteName: String, callsiteDesc: String) extends CannotInlineWarning + case object UnknownInvokeDynamicInstruction extends OptimizerWarning { + override def toString = "The callee contains an InvokeDynamic instruction with an unknown bootstrap method (not a LambdaMetaFactory)." + def emitWarning(settings: ScalaSettings): Boolean = settings.YoptWarningEmitAtInlineFailed + } + /** * Used in `rewriteClosureApplyInvocations` when a closure apply callsite cannot be rewritten * to the closure body method. diff --git a/src/compiler/scala/tools/nsc/backend/jvm/opt/Inliner.scala b/src/compiler/scala/tools/nsc/backend/jvm/opt/Inliner.scala index e8e848161c..4a022b7bc4 100644 --- a/src/compiler/scala/tools/nsc/backend/jvm/opt/Inliner.scala +++ b/src/compiler/scala/tools/nsc/backend/jvm/opt/Inliner.scala @@ -9,6 +9,7 @@ package opt import scala.annotation.tailrec import scala.tools.asm +import asm.Handle import asm.Opcodes._ import asm.tree._ import scala.collection.convert.decorateAsScala._ @@ -687,9 +688,67 @@ class Inliner[BT <: BTypes](val btypes: BT) { } } - case ivd: InvokeDynamicInsnNode => - // TODO @lry check necessary conditions to inline an indy, instead of giving up - Right(false) + case indy: InvokeDynamicInsnNode => + // an indy instr points to a "call site specifier" (CSP) [1] + // - a reference to a bootstrap method [2] + // - bootstrap method name + // - references to constant arguments, which can be: + // - constant (string, long, int, float, double) + // - class + // - method type (without name) + // - method handle + // - a method name+type + // + // execution [3] + // - resolve the CSP, yielding the boostrap method handle, the static args and the name+type + // - resolution entails accessibility checking [4] + // - execute the `invoke` method of the boostrap method handle (which is signature polymorphic, check its javadoc) + // - the descriptor for the call is made up from the actual arguments on the stack: + // - the first parameters are "MethodHandles.Lookup, String, MethodType", then the types of the constant arguments, + // - the return type is CallSite + // - the values for the call are + // - the bootstrap method handle of the CSP is the receiver + // - the Lookup object for the class in which the callsite occurs (obtained as through calling MethodHandles.lookup()) + // - the method name of the CSP + // - the method type of the CSP + // - the constants of the CSP (primitives are not boxed) + // - the resulting `CallSite` object + // - has as `type` the method type of the CSP + // - is popped from the operand stack + // - the `invokeExact` method (signature polymorphic!) of the `target` method handle of the CallSite is invoked + // - the method descriptor is that of the CSP + // - the receiver is the target of the CallSite + // - the other argument values are those that were on the operand stack at the indy instruction (indyLambda: the captured values) + // + // [1] http://docs.oracle.com/javase/specs/jvms/se8/html/jvms-4.html#jvms-4.4.10 + // [2] http://docs.oracle.com/javase/specs/jvms/se8/html/jvms-4.html#jvms-4.7.23 + // [3] http://docs.oracle.com/javase/specs/jvms/se8/html/jvms-6.html#jvms-6.5.invokedynamic + // [4] http://docs.oracle.com/javase/specs/jvms/se8/html/jvms-5.html#jvms-5.4.3 + + // We cannot generically check if an `invokedynamic` instruction can be safely inlined into + // a different class, that depends on the bootstrap method. The Lookup object passed to the + // bootstrap method is a capability to access private members of the callsite class. We can + // only move the invokedynamic to a new class if we know that the bootstrap method doesn't + // use this capability for otherwise non-accessible members. + // In the case of indyLambda, it depends on the visibility of the implMethod handle. If + // the implMethod is public, lambdaMetaFactory doesn't use the Lookup object's extended + // capability, and we can safely inline the instruction into a different class. + + if (destinationClass == calleeDeclarationClass) { + Right(true) // within the same class, any indy instruction can be inlined + } else if (closureOptimizer.isClosureInstantiation(indy)) { + val implMethod = indy.bsmArgs(1).asInstanceOf[Handle] // safe, checked in isClosureInstantiation + val methodRefClass = classBTypeFromParsedClassfile(implMethod.getOwner) + for { + (methodNode, methodDeclClassNode) <- byteCodeRepository.methodNode(methodRefClass.internalName, implMethod.getName, implMethod.getDesc): Either[OptimizerWarning, (MethodNode, InternalName)] + methodDeclClass = classBTypeFromParsedClassfile(methodDeclClassNode) + res <- memberIsAccessible(methodNode.access, methodDeclClass, methodRefClass, destinationClass) + } yield { + res + } + } else { + Left(UnknownInvokeDynamicInstruction) + } case ci: LdcInsnNode => ci.cst match { case t: asm.Type => classIsAccessible(bTypeForDescriptorOrInternalNameFromClassfile(t.getInternalName), destinationClass) diff --git a/src/partest-extras/scala/tools/partest/ASMConverters.scala b/src/partest-extras/scala/tools/partest/ASMConverters.scala index 90f314428b..b4c686473b 100644 --- a/src/partest-extras/scala/tools/partest/ASMConverters.scala +++ b/src/partest-extras/scala/tools/partest/ASMConverters.scala @@ -58,21 +58,24 @@ object ASMConverters { case class Method(instructions: List[Instruction], handlers: List[ExceptionHandler], localVars: List[LocalVariable]) - case class Field (opcode: Int, owner: String, name: String, desc: String) extends Instruction - case class Incr (opcode: Int, `var`: Int, incr: Int) extends Instruction - case class Op (opcode: Int) extends Instruction - case class IntOp (opcode: Int, operand: Int) extends Instruction - case class Jump (opcode: Int, label: Label) extends Instruction - case class Ldc (opcode: Int, cst: Any) extends Instruction - case class LookupSwitch(opcode: Int, dflt: Label, keys: List[Int], labels: List[Label]) extends Instruction - case class TableSwitch (opcode: Int, min: Int, max: Int, dflt: Label, labels: List[Label]) extends Instruction - case class Invoke (opcode: Int, owner: String, name: String, desc: String, itf: Boolean) extends Instruction - case class NewArray (opcode: Int, desc: String, dims: Int) extends Instruction - case class TypeOp (opcode: Int, desc: String) extends Instruction - case class VarOp (opcode: Int, `var`: Int) extends Instruction - case class Label (offset: Int) extends Instruction { def opcode: Int = -1 } - case class FrameEntry (`type`: Int, local: List[Any], stack: List[Any]) extends Instruction { def opcode: Int = -1 } - case class LineNumber (line: Int, start: Label) extends Instruction { def opcode: Int = -1 } + case class Field (opcode: Int, owner: String, name: String, desc: String) extends Instruction + case class Incr (opcode: Int, `var`: Int, incr: Int) extends Instruction + case class Op (opcode: Int) extends Instruction + case class IntOp (opcode: Int, operand: Int) extends Instruction + case class Jump (opcode: Int, label: Label) extends Instruction + case class Ldc (opcode: Int, cst: Any) extends Instruction + case class LookupSwitch (opcode: Int, dflt: Label, keys: List[Int], labels: List[Label]) extends Instruction + case class TableSwitch (opcode: Int, min: Int, max: Int, dflt: Label, labels: List[Label]) extends Instruction + case class Invoke (opcode: Int, owner: String, name: String, desc: String, itf: Boolean) extends Instruction + case class InvokeDynamic(opcode: Int, name: String, desc: String, bsm: MethodHandle, bsmArgs: List[AnyRef]) extends Instruction + case class NewArray (opcode: Int, desc: String, dims: Int) extends Instruction + case class TypeOp (opcode: Int, desc: String) extends Instruction + case class VarOp (opcode: Int, `var`: Int) extends Instruction + case class Label (offset: Int) extends Instruction { def opcode: Int = -1 } + case class FrameEntry (`type`: Int, local: List[Any], stack: List[Any]) extends Instruction { def opcode: Int = -1 } + case class LineNumber (line: Int, start: Label) extends Instruction { def opcode: Int = -1 } + + case class MethodHandle(tag: Int, owner: String, name: String, desc: String) case class ExceptionHandler(start: Label, end: Label, handler: Label, desc: Option[String]) case class LocalVariable(name: String, desc: String, signature: Option[String], start: Label, end: Label, index: Int) @@ -111,6 +114,7 @@ object ASMConverters { case i: t.LookupSwitchInsnNode => LookupSwitch (op(i), applyLabel(i.dflt), lst(i.keys) map (x => x: Int), lst(i.labels) map applyLabel) case i: t.TableSwitchInsnNode => TableSwitch (op(i), i.min, i.max, applyLabel(i.dflt), lst(i.labels) map applyLabel) case i: t.MethodInsnNode => Invoke (op(i), i.owner, i.name, i.desc, i.itf) + case i: t.InvokeDynamicInsnNode => InvokeDynamic(op(i), i.name, i.desc, convertMethodHandle(i.bsm), convertBsmArgs(i.bsmArgs)) case i: t.MultiANewArrayInsnNode => NewArray (op(i), i.desc, i.dims) case i: t.TypeInsnNode => TypeOp (op(i), i.desc) case i: t.VarInsnNode => VarOp (op(i), i.`var`) @@ -119,6 +123,13 @@ object ASMConverters { case i: t.LineNumberNode => LineNumber (i.line, applyLabel(i.start)) } + private def convertBsmArgs(a: Array[Object]): List[Object] = a.map({ + case h: asm.Handle => convertMethodHandle(h) + case _ => a // can be: Class, method Type, primitive constant + })(collection.breakOut) + + private def convertMethodHandle(h: asm.Handle): MethodHandle = MethodHandle(h.getTag, h.getOwner, h.getName, h.getDesc) + private def convertHandlers(method: t.MethodNode): List[ExceptionHandler] = { method.tryCatchBlocks.asScala.map(h => ExceptionHandler(applyLabel(h.start), applyLabel(h.end), applyLabel(h.handler), Option(h.`type`)))(collection.breakOut) } @@ -197,21 +208,28 @@ object ASMConverters { case x => x.asInstanceOf[Object] } + def unconvertMethodHandle(h: MethodHandle): asm.Handle = new asm.Handle(h.tag, h.owner, h.name, h.desc) + def unconvertBsmArgs(a: List[Object]): Array[Object] = a.map({ + case h: MethodHandle => unconvertMethodHandle(h) + case o => o + })(collection.breakOut) + private def visitMethod(method: t.MethodNode, instruction: Instruction, asmLabel: Map[Label, asm.Label]): Unit = instruction match { - case Field(op, owner, name, desc) => method.visitFieldInsn(op, owner, name, desc) - case Incr(op, vr, incr) => method.visitIincInsn(vr, incr) - case Op(op) => method.visitInsn(op) - case IntOp(op, operand) => method.visitIntInsn(op, operand) - case Jump(op, label) => method.visitJumpInsn(op, asmLabel(label)) - case Ldc(op, cst) => method.visitLdcInsn(cst) - case LookupSwitch(op, dflt, keys, labels) => method.visitLookupSwitchInsn(asmLabel(dflt), keys.toArray, (labels map asmLabel).toArray) - case TableSwitch(op, min, max, dflt, labels) => method.visitTableSwitchInsn(min, max, asmLabel(dflt), (labels map asmLabel).toArray: _*) - case Invoke(op, owner, name, desc, itf) => method.visitMethodInsn(op, owner, name, desc, itf) - case NewArray(op, desc, dims) => method.visitMultiANewArrayInsn(desc, dims) - case TypeOp(op, desc) => method.visitTypeInsn(op, desc) - case VarOp(op, vr) => method.visitVarInsn(op, vr) - case l: Label => method.visitLabel(asmLabel(l)) - case FrameEntry(tp, local, stack) => method.visitFrame(tp, local.length, frameTypesToAsm(local, asmLabel).toArray, stack.length, frameTypesToAsm(stack, asmLabel).toArray) - case LineNumber(line, start) => method.visitLineNumber(line, asmLabel(start)) + case Field(op, owner, name, desc) => method.visitFieldInsn(op, owner, name, desc) + case Incr(op, vr, incr) => method.visitIincInsn(vr, incr) + case Op(op) => method.visitInsn(op) + case IntOp(op, operand) => method.visitIntInsn(op, operand) + case Jump(op, label) => method.visitJumpInsn(op, asmLabel(label)) + case Ldc(op, cst) => method.visitLdcInsn(cst) + case LookupSwitch(op, dflt, keys, labels) => method.visitLookupSwitchInsn(asmLabel(dflt), keys.toArray, (labels map asmLabel).toArray) + case TableSwitch(op, min, max, dflt, labels) => method.visitTableSwitchInsn(min, max, asmLabel(dflt), (labels map asmLabel).toArray: _*) + case Invoke(op, owner, name, desc, itf) => method.visitMethodInsn(op, owner, name, desc, itf) + case InvokeDynamic(op, name, desc, bsm, bsmArgs) => method.visitInvokeDynamicInsn(name, desc, unconvertMethodHandle(bsm), unconvertBsmArgs(bsmArgs)) + case NewArray(op, desc, dims) => method.visitMultiANewArrayInsn(desc, dims) + case TypeOp(op, desc) => method.visitTypeInsn(op, desc) + case VarOp(op, vr) => method.visitVarInsn(op, vr) + case l: Label => method.visitLabel(asmLabel(l)) + case FrameEntry(tp, local, stack) => method.visitFrame(tp, local.length, frameTypesToAsm(local, asmLabel).toArray, stack.length, frameTypesToAsm(stack, asmLabel).toArray) + case LineNumber(line, start) => method.visitLineNumber(line, asmLabel(start)) } } -- cgit v1.2.3 From fc1cda21183436181effc87e2df176ddc2d65be9 Mon Sep 17 00:00:00 2001 From: Lukas Rytz Date: Tue, 7 Jul 2015 14:34:11 +0200 Subject: [backport] Small refactoring to the closure optimizer Introduces an extractor `LMFInvokeDynamic` that matches InvokeDynamic instructions that are LambdaMetaFactory calls. The case class `LambdaMetaFactoryCall` holds such an InvokeDynamic instruction. It also holds the bootstrap arguments (samMethodType, implMethod, instantiatedMethodType) so that they can be accessed without casting the indy.bsmArgs. The `closureInstantiations` map in the call graph now stores ClosureInstantiation objects instead of a tuple. This simplifies some code and gets rid of a few casts. --- .../tools/nsc/backend/jvm/opt/CallGraph.scala | 101 ++++++++++++- .../nsc/backend/jvm/opt/ClosureOptimizer.scala | 158 ++++++--------------- .../scala/tools/nsc/backend/jvm/opt/Inliner.scala | 31 ++-- 3 files changed, 151 insertions(+), 139 deletions(-) diff --git a/src/compiler/scala/tools/nsc/backend/jvm/opt/CallGraph.scala b/src/compiler/scala/tools/nsc/backend/jvm/opt/CallGraph.scala index 8abecdb261..8c1673b4f2 100644 --- a/src/compiler/scala/tools/nsc/backend/jvm/opt/CallGraph.scala +++ b/src/compiler/scala/tools/nsc/backend/jvm/opt/CallGraph.scala @@ -9,7 +9,7 @@ package opt import scala.reflect.internal.util.{NoPosition, Position} import scala.tools.asm.tree.analysis.{Value, Analyzer, BasicInterpreter} -import scala.tools.asm.{Opcodes, Type} +import scala.tools.asm.{Opcodes, Type, Handle} import scala.tools.asm.tree._ import scala.collection.concurrent import scala.collection.convert.decorateAsScala._ @@ -24,7 +24,7 @@ class CallGraph[BT <: BTypes](val btypes: BT) { val callsites: concurrent.Map[MethodInsnNode, Callsite] = recordPerRunCache(concurrent.TrieMap.empty) - val closureInstantiations: concurrent.Map[InvokeDynamicInsnNode, (MethodNode, ClassBType)] = recordPerRunCache(concurrent.TrieMap.empty) + val closureInstantiations: concurrent.Map[InvokeDynamicInsnNode, ClosureInstantiation] = recordPerRunCache(concurrent.TrieMap.empty) def addClass(classNode: ClassNode): Unit = { val classType = classBTypeFromClassNode(classNode) @@ -33,14 +33,14 @@ class CallGraph[BT <: BTypes](val btypes: BT) { (calls, closureInits) = analyzeCallsites(m, classType) } { calls foreach (callsite => callsites(callsite.callsiteInstruction) = callsite) - closureInits foreach (indy => closureInstantiations(indy) = (m, classType)) + closureInits foreach (lmf => closureInstantiations(lmf.indy) = ClosureInstantiation(lmf, m, classType)) } } /** * Returns a list of callsites in the method, plus a list of closure instantiation indy instructions. */ - def analyzeCallsites(methodNode: MethodNode, definingClass: ClassBType): (List[Callsite], List[InvokeDynamicInsnNode]) = { + def analyzeCallsites(methodNode: MethodNode, definingClass: ClassBType): (List[Callsite], List[LambdaMetaFactoryCall]) = { case class CallsiteInfo(safeToInline: Boolean, safeToRewrite: Boolean, annotatedInline: Boolean, annotatedNoInline: Boolean, @@ -129,7 +129,7 @@ class CallGraph[BT <: BTypes](val btypes: BT) { } val callsites = new collection.mutable.ListBuffer[Callsite] - val closureInstantiations = new collection.mutable.ListBuffer[InvokeDynamicInsnNode] + val closureInstantiations = new collection.mutable.ListBuffer[LambdaMetaFactoryCall] methodNode.instructions.iterator.asScala foreach { case call: MethodInsnNode => @@ -173,8 +173,8 @@ class CallGraph[BT <: BTypes](val btypes: BT) { callsitePosition = callsitePositions.getOrElse(call, NoPosition) ) - case indy: InvokeDynamicInsnNode => - if (closureOptimizer.isClosureInstantiation(indy)) closureInstantiations += indy + case LMFInvokeDynamic(lmf) => + closureInstantiations += lmf case _ => } @@ -236,4 +236,91 @@ class CallGraph[BT <: BTypes](val btypes: BT) { calleeInfoWarning: Option[CalleeInfoWarning]) { assert(!(safeToInline && safeToRewrite), s"A callee of ${callee.name} can be either safeToInline or safeToRewrite, but not both.") } + + final case class ClosureInstantiation(lambdaMetaFactoryCall: LambdaMetaFactoryCall, ownerMethod: MethodNode, ownerClass: ClassBType) { + override def toString = s"ClosureInstantiation($lambdaMetaFactoryCall, ${ownerMethod.name + ownerMethod.desc}, $ownerClass)" + } + final case class LambdaMetaFactoryCall(indy: InvokeDynamicInsnNode, samMethodType: Type, implMethod: Handle, instantiatedMethodType: Type) + + object LMFInvokeDynamic { + private val lambdaMetaFactoryInternalName: InternalName = "java/lang/invoke/LambdaMetafactory" + + private val metafactoryHandle = { + val metafactoryMethodName: String = "metafactory" + val metafactoryDesc: String = "(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/MethodType;Ljava/lang/invoke/MethodType;Ljava/lang/invoke/MethodHandle;Ljava/lang/invoke/MethodType;)Ljava/lang/invoke/CallSite;" + new Handle(Opcodes.H_INVOKESTATIC, lambdaMetaFactoryInternalName, metafactoryMethodName, metafactoryDesc) + } + + private val altMetafactoryHandle = { + val altMetafactoryMethodName: String = "altMetafactory" + val altMetafactoryDesc: String = "(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/MethodType;[Ljava/lang/Object;)Ljava/lang/invoke/CallSite;" + new Handle(Opcodes.H_INVOKESTATIC, lambdaMetaFactoryInternalName, altMetafactoryMethodName, altMetafactoryDesc) + } + + private def extractLambdaMetaFactoryCall(indy: InvokeDynamicInsnNode) = { + if (indy.bsm == metafactoryHandle || indy.bsm == altMetafactoryHandle) indy.bsmArgs match { + case Array(samMethodType: Type, implMethod: Handle, instantiatedMethodType: Type, xs@_*) => + // LambdaMetaFactory performs a number of automatic adaptations when invoking the lambda + // implementation method (casting, boxing, unboxing, and primitive widening, see Javadoc). + // + // The closure optimizer supports only one of those adaptations: it will cast arguments + // to the correct type when re-writing a closure call to the body method. Example: + // + // val fun: String => String = l => l + // val l = List("") + // fun(l.head) + // + // The samMethodType of Function1 is `(Object)Object`, while the instantiatedMethodType + // is `(String)String`. The return type of `List.head` is `Object`. + // + // The implMethod has the signature `C$anonfun(String)String`. + // + // At the closure callsite, we have an `INVOKEINTERFACE Function1.apply (Object)Object`, + // so the object returned by `List.head` can be directly passed into the call (no cast). + // + // The closure object will cast the object to String before passing it to the implMethod. + // + // When re-writing the closure callsite to the implMethod, we have to insert a cast. + // + // The check below ensures that + // (1) the implMethod type has the expected singature (captured types plus argument types + // from instantiatedMethodType) + // (2) the receiver of the implMethod matches the first captured type + // (3) all parameters that are not the same in samMethodType and instantiatedMethodType + // are reference types, so that we can insert casts to perform the same adaptation + // that the closure object would. + + val isStatic = implMethod.getTag == Opcodes.H_INVOKESTATIC + val indyParamTypes = Type.getArgumentTypes(indy.desc) + val instantiatedMethodArgTypes = instantiatedMethodType.getArgumentTypes + val expectedImplMethodType = { + val paramTypes = (if (isStatic) indyParamTypes else indyParamTypes.tail) ++ instantiatedMethodArgTypes + Type.getMethodType(instantiatedMethodType.getReturnType, paramTypes: _*) + } + + val isIndyLambda = { + Type.getType(implMethod.getDesc) == expectedImplMethodType // (1) + } && { + isStatic || implMethod.getOwner == indyParamTypes(0).getInternalName // (2) + } && { + def isReference(t: Type) = t.getSort == Type.OBJECT || t.getSort == Type.ARRAY + (samMethodType.getArgumentTypes, instantiatedMethodArgTypes).zipped forall { + case (samArgType, instArgType) => + samArgType == instArgType || isReference(samArgType) && isReference(instArgType) // (3) + } + } + + if (isIndyLambda) Some(LambdaMetaFactoryCall(indy, samMethodType, implMethod, instantiatedMethodType)) + else None + + case _ => None + } + else None + } + + def unapply(insn: AbstractInsnNode): Option[LambdaMetaFactoryCall] = insn match { + case indy: InvokeDynamicInsnNode => extractLambdaMetaFactoryCall(indy) + case _ => None + } + } } diff --git a/src/compiler/scala/tools/nsc/backend/jvm/opt/ClosureOptimizer.scala b/src/compiler/scala/tools/nsc/backend/jvm/opt/ClosureOptimizer.scala index 8da209b269..86536ff0d2 100644 --- a/src/compiler/scala/tools/nsc/backend/jvm/opt/ClosureOptimizer.scala +++ b/src/compiler/scala/tools/nsc/backend/jvm/opt/ClosureOptimizer.scala @@ -8,6 +8,7 @@ package backend.jvm package opt import scala.annotation.switch +import scala.collection.mutable import scala.reflect.internal.util.NoPosition import scala.tools.asm.{Handle, Type, Opcodes} import scala.tools.asm.tree._ @@ -24,106 +25,28 @@ class ClosureOptimizer[BT <: BTypes](val btypes: BT) { import callGraph._ def rewriteClosureApplyInvocations(): Unit = { - implicit object closureInitOrdering extends Ordering[(InvokeDynamicInsnNode, MethodNode, ClassBType)] { - // Note: this code is cleaned up in a future commit, no more tuples. - override def compare(x: (InvokeDynamicInsnNode, MethodNode, ClassBType), y: (InvokeDynamicInsnNode, MethodNode, ClassBType)): Int = { - val cls = x._3.internalName compareTo y._3.internalName + implicit object closureInitOrdering extends Ordering[ClosureInstantiation] { + override def compare(x: ClosureInstantiation, y: ClosureInstantiation): Int = { + val cls = x.ownerClass.internalName compareTo y.ownerClass.internalName if (cls != 0) return cls - val mName = x._2.name compareTo y._2.name + val mName = x.ownerMethod.name compareTo y.ownerMethod.name if (mName != 0) return mName - val mDesc = x._2.desc compareTo y._2.desc + val mDesc = x.ownerMethod.desc compareTo y.ownerMethod.desc if (mDesc != 0) return mDesc - def pos(indy: InvokeDynamicInsnNode) = x._2.instructions.indexOf(indy) - pos(x._1) - pos(y._1) + def pos(inst: ClosureInstantiation) = inst.ownerMethod.instructions.indexOf(inst.lambdaMetaFactoryCall.indy) + pos(x) - pos(y) } } - val sorted = closureInstantiations.iterator.map({ - case (indy, (methodNode, ownerClass)) => (indy, methodNode, ownerClass) - }).to[collection.immutable.TreeSet] + val sorted = mutable.TreeSet.empty[ClosureInstantiation] + sorted ++= closureInstantiations.values - sorted foreach { - case (indy, methodNode, ownerClass) => - val warnings = rewriteClosureApplyInvocations(indy, methodNode, ownerClass) - warnings.foreach(w => backendReporting.inlinerWarning(w.pos, w.toString)) - } - } - - private val lambdaMetaFactoryInternalName: InternalName = "java/lang/invoke/LambdaMetafactory" - - private val metafactoryHandle = { - val metafactoryMethodName: String = "metafactory" - val metafactoryDesc: String = "(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/MethodType;Ljava/lang/invoke/MethodType;Ljava/lang/invoke/MethodHandle;Ljava/lang/invoke/MethodType;)Ljava/lang/invoke/CallSite;" - new Handle(H_INVOKESTATIC, lambdaMetaFactoryInternalName, metafactoryMethodName, metafactoryDesc) - } - - private val altMetafactoryHandle = { - val altMetafactoryMethodName: String = "altMetafactory" - val altMetafactoryDesc: String = "(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/MethodType;[Ljava/lang/Object;)Ljava/lang/invoke/CallSite;" - new Handle(H_INVOKESTATIC, lambdaMetaFactoryInternalName, altMetafactoryMethodName, altMetafactoryDesc) - } - - def isClosureInstantiation(indy: InvokeDynamicInsnNode): Boolean = { - (indy.bsm == metafactoryHandle || indy.bsm == altMetafactoryHandle) && - { - indy.bsmArgs match { - case Array(samMethodType: Type, implMethod: Handle, instantiatedMethodType: Type, xs @ _*) => - // LambdaMetaFactory performs a number of automatic adaptations when invoking the lambda - // implementation method (casting, boxing, unboxing, and primitive widening, see Javadoc). - // - // The closure optimizer supports only one of those adaptations: it will cast arguments - // to the correct type when re-writing a closure call to the body method. Example: - // - // val fun: String => String = l => l - // val l = List("") - // fun(l.head) - // - // The samMethodType of Function1 is `(Object)Object`, while the instantiatedMethodType - // is `(String)String`. The return type of `List.head` is `Object`. - // - // The implMethod has the signature `C$anonfun(String)String`. - // - // At the closure callsite, we have an `INVOKEINTERFACE Function1.apply (Object)Object`, - // so the object returned by `List.head` can be directly passed into the call (no cast). - // - // The closure object will cast the object to String before passing it to the implMethod. - // - // When re-writing the closure callsite to the implMethod, we have to insert a cast. - // - // The check below ensures that - // (1) the implMethod type has the expected singature (captured types plus argument types - // from instantiatedMethodType) - // (2) the receiver of the implMethod matches the first captured type - // (3) all parameters that are not the same in samMethodType and instantiatedMethodType - // are reference types, so that we can insert casts to perform the same adaptation - // that the closure object would. - - val isStatic = implMethod.getTag == H_INVOKESTATIC - val indyParamTypes = Type.getArgumentTypes(indy.desc) - val instantiatedMethodArgTypes = instantiatedMethodType.getArgumentTypes - val expectedImplMethodType = { - val paramTypes = (if (isStatic) indyParamTypes else indyParamTypes.tail) ++ instantiatedMethodArgTypes - Type.getMethodType(instantiatedMethodType.getReturnType, paramTypes: _*) - } - - { - Type.getType(implMethod.getDesc) == expectedImplMethodType // (1) - } && { - isStatic || implMethod.getOwner == indyParamTypes(0).getInternalName // (2) - } && { - def isReference(t: Type) = t.getSort == Type.OBJECT || t.getSort == Type.ARRAY - (samMethodType.getArgumentTypes, instantiatedMethodArgTypes).zipped forall { - case (samArgType, instArgType) => - samArgType == instArgType || isReference(samArgType) && isReference(instArgType) // (3) - } - } - - case _ => - false - } + for (closureInst <- sorted) { + val warnings = rewriteClosureApplyInvocations(closureInst) + warnings.foreach(w => backendReporting.inlinerWarning(w.pos, w.toString)) } } @@ -152,9 +75,10 @@ class ClosureOptimizer[BT <: BTypes](val btypes: BT) { * Stores the values captured by a closure creation into fresh local variables. * Returns the list of locals holding the captured values. */ - private def storeCaptures(indy: InvokeDynamicInsnNode, methodNode: MethodNode): LocalsList = { + private def storeCaptures(closureInit: ClosureInstantiation): LocalsList = { + val indy = closureInit.lambdaMetaFactoryCall.indy val capturedTypes = Type.getArgumentTypes(indy.desc) - val firstCaptureLocal = methodNode.maxLocals + val firstCaptureLocal = closureInit.ownerMethod.maxLocals // This could be optimized: in many cases the captured values are produced by LOAD instructions. // If the variable is not modified within the method, we could avoid introducing yet another @@ -165,10 +89,10 @@ class ClosureOptimizer[BT <: BTypes](val btypes: BT) { // This is checked in `isClosureInstantiation`: the types of the captured variables in the indy // instruction match exactly the corresponding parameter types in the body method. val localsForCaptures = LocalsList.fromTypes(firstCaptureLocal, capturedTypes, castLoadTypes = _ => None) - methodNode.maxLocals = firstCaptureLocal + localsForCaptures.size + closureInit.ownerMethod.maxLocals = firstCaptureLocal + localsForCaptures.size - insertStoreOps(indy, methodNode, localsForCaptures) - insertLoadOps(indy, methodNode, localsForCaptures) + insertStoreOps(indy, closureInit.ownerMethod, localsForCaptures) + insertLoadOps(indy, closureInit.ownerMethod, localsForCaptures) localsForCaptures } @@ -205,22 +129,24 @@ class ClosureOptimizer[BT <: BTypes](val btypes: BT) { } } - def rewriteClosureApplyInvocations(indy: InvokeDynamicInsnNode, methodNode: MethodNode, ownerClass: ClassBType): List[RewriteClosureApplyToClosureBodyFailed] = { - val lambdaBodyHandle = indy.bsmArgs(1).asInstanceOf[Handle] // safe, checked in isClosureInstantiation + def rewriteClosureApplyInvocations(closureInit: ClosureInstantiation): List[RewriteClosureApplyToClosureBodyFailed] = { + val lambdaBodyHandle = closureInit.lambdaMetaFactoryCall.implMethod + val ownerMethod = closureInit.ownerMethod + val ownerClass = closureInit.ownerClass // Kept as a lazy val to make sure the analysis is only computed if it's actually needed. // ProdCons is used to identify closure body invocations (see isSamInvocation), but only if the // callsite has the right name and signature. If the method has no invcation instruction with // the right name and signature, the analysis is not executed. - lazy val prodCons = new ProdConsAnalyzer(methodNode, ownerClass.internalName) + lazy val prodCons = new ProdConsAnalyzer(ownerMethod, ownerClass.internalName) // First collect all callsites without modifying the instructions list yet. // Once we start modifying the instruction list, prodCons becomes unusable. // A list of callsites and stack heights. If the invocation cannot be rewritten, a warning // message is stored in the stack height value. - val invocationsToRewrite: List[(MethodInsnNode, Either[RewriteClosureApplyToClosureBodyFailed, Int])] = methodNode.instructions.iterator.asScala.collect({ - case invocation: MethodInsnNode if isSamInvocation(invocation, indy, prodCons) => + val invocationsToRewrite: List[(MethodInsnNode, Either[RewriteClosureApplyToClosureBodyFailed, Int])] = ownerMethod.instructions.iterator.asScala.collect({ + case invocation: MethodInsnNode if isSamInvocation(invocation, closureInit.lambdaMetaFactoryCall.indy, prodCons) => val bodyAccessible: Either[OptimizerWarning, Boolean] = for { (bodyMethodNode, declClass) <- byteCodeRepository.methodNode(lambdaBodyHandle.getOwner, lambdaBodyHandle.getName, lambdaBodyHandle.getDesc): Either[OptimizerWarning, (MethodNode, InternalName)] isAccessible <- inliner.memberIsAccessible(bodyMethodNode.access, classBTypeFromParsedClassfile(declClass), classBTypeFromParsedClassfile(lambdaBodyHandle.getOwner), ownerClass) @@ -243,17 +169,17 @@ class ClosureOptimizer[BT <: BTypes](val btypes: BT) { // lazy val to make sure locals for captures and arguments are only allocated if there's // effectively a callsite to rewrite. lazy val (localsForCapturedValues, argumentLocalsList) = { - val captureLocals = storeCaptures(indy, methodNode) + val captureLocals = storeCaptures(closureInit) // allocate locals for storing the arguments of the closure apply callsites. // if there are multiple callsites, the same locals are re-used. - val argTypes = indy.bsmArgs(0).asInstanceOf[Type].getArgumentTypes // safe, checked in isClosureInstantiation - val firstArgLocal = methodNode.maxLocals + val argTypes = closureInit.lambdaMetaFactoryCall.samMethodType.getArgumentTypes + val firstArgLocal = ownerMethod.maxLocals // The comment in `isClosureInstantiation` explains why we have to introduce casts for // arguments that have different types in samMethodType and instantiatedMethodType. val castLoadTypes = { - val instantiatedMethodType = indy.bsmArgs(2).asInstanceOf[Type] + val instantiatedMethodType = closureInit.lambdaMetaFactoryCall.instantiatedMethodType (argTypes, instantiatedMethodType.getArgumentTypes).zipped map { case (samArgType, instantiatedArgType) if samArgType != instantiatedArgType => // isClosureInstantiation ensures that the two types are reference types, so we don't @@ -264,7 +190,7 @@ class ClosureOptimizer[BT <: BTypes](val btypes: BT) { } } val argLocals = LocalsList.fromTypes(firstArgLocal, argTypes, castLoadTypes) - methodNode.maxLocals = firstArgLocal + argLocals.size + ownerMethod.maxLocals = firstArgLocal + argLocals.size (captureLocals, argLocals) } @@ -274,20 +200,20 @@ class ClosureOptimizer[BT <: BTypes](val btypes: BT) { case (invocation, Right(stackHeight)) => // store arguments - insertStoreOps(invocation, methodNode, argumentLocalsList) + insertStoreOps(invocation, ownerMethod, argumentLocalsList) // drop the closure from the stack - methodNode.instructions.insertBefore(invocation, new InsnNode(POP)) + ownerMethod.instructions.insertBefore(invocation, new InsnNode(POP)) // load captured values and arguments - insertLoadOps(invocation, methodNode, localsForCapturedValues) - insertLoadOps(invocation, methodNode, argumentLocalsList) + insertLoadOps(invocation, ownerMethod, localsForCapturedValues) + insertLoadOps(invocation, ownerMethod, argumentLocalsList) // update maxStack val capturesStackSize = localsForCapturedValues.size val invocationStackHeight = stackHeight + capturesStackSize - 1 // -1 because the closure is gone - if (invocationStackHeight > methodNode.maxStack) - methodNode.maxStack = invocationStackHeight + if (invocationStackHeight > ownerMethod.maxStack) + ownerMethod.maxStack = invocationStackHeight // replace the callsite with a new call to the body method val bodyOpcode = (lambdaBodyHandle.getTag: @switch) match { @@ -296,19 +222,19 @@ class ClosureOptimizer[BT <: BTypes](val btypes: BT) { case H_INVOKESPECIAL => INVOKESPECIAL case H_INVOKEINTERFACE => INVOKEINTERFACE case H_NEWINVOKESPECIAL => - val insns = methodNode.instructions + val insns = ownerMethod.instructions insns.insertBefore(invocation, new TypeInsnNode(NEW, lambdaBodyHandle.getOwner)) insns.insertBefore(invocation, new InsnNode(DUP)) INVOKESPECIAL } val isInterface = bodyOpcode == INVOKEINTERFACE val bodyInvocation = new MethodInsnNode(bodyOpcode, lambdaBodyHandle.getOwner, lambdaBodyHandle.getName, lambdaBodyHandle.getDesc, isInterface) - methodNode.instructions.insertBefore(invocation, bodyInvocation) + ownerMethod.instructions.insertBefore(invocation, bodyInvocation) val returnType = Type.getReturnType(lambdaBodyHandle.getDesc) - fixLoadedNothingOrNullValue(returnType, bodyInvocation, methodNode, btypes) // see comment of that method + fixLoadedNothingOrNullValue(returnType, bodyInvocation, ownerMethod, btypes) // see comment of that method - methodNode.instructions.remove(invocation) + ownerMethod.instructions.remove(invocation) // update the call graph val originalCallsite = callGraph.callsites.remove(invocation) @@ -318,7 +244,7 @@ class ClosureOptimizer[BT <: BTypes](val btypes: BT) { def bodyMethodIsBeingCompiled = byteCodeRepository.classNodeAndSource(lambdaBodyHandle.getOwner).map(_._2 == CompilationUnit).getOrElse(false) val bodyMethodCallsite = Callsite( callsiteInstruction = bodyInvocation, - callsiteMethod = methodNode, + callsiteMethod = ownerMethod, callsiteClass = ownerClass, callee = bodyMethod.map({ case (bodyMethodNode, bodyMethodDeclClass) => Callee( diff --git a/src/compiler/scala/tools/nsc/backend/jvm/opt/Inliner.scala b/src/compiler/scala/tools/nsc/backend/jvm/opt/Inliner.scala index 4a022b7bc4..3794ee9950 100644 --- a/src/compiler/scala/tools/nsc/backend/jvm/opt/Inliner.scala +++ b/src/compiler/scala/tools/nsc/backend/jvm/opt/Inliner.scala @@ -456,9 +456,9 @@ class Inliner[BT <: BTypes](val btypes: BT) { case indy: InvokeDynamicInsnNode => callGraph.closureInstantiations.get(indy) match { - case Some((methodNode, ownerClass)) => + case Some(closureInit) => val newIndy = instructionMap(indy).asInstanceOf[InvokeDynamicInsnNode] - callGraph.closureInstantiations(newIndy) = (callsiteMethod, callsiteClass) + callGraph.closureInstantiations(newIndy) = ClosureInstantiation(closureInit.lambdaMetaFactoryCall.copy(indy = newIndy), callsiteMethod, callsiteClass) case None => } @@ -688,7 +688,7 @@ class Inliner[BT <: BTypes](val btypes: BT) { } } - case indy: InvokeDynamicInsnNode => + case LMFInvokeDynamic(lmf) => // an indy instr points to a "call site specifier" (CSP) [1] // - a reference to a bootstrap method [2] // - bootstrap method name @@ -734,21 +734,20 @@ class Inliner[BT <: BTypes](val btypes: BT) { // the implMethod is public, lambdaMetaFactory doesn't use the Lookup object's extended // capability, and we can safely inline the instruction into a different class. - if (destinationClass == calleeDeclarationClass) { + val methodRefClass = classBTypeFromParsedClassfile(lmf.implMethod.getOwner) + for { + (methodNode, methodDeclClassNode) <- byteCodeRepository.methodNode(methodRefClass.internalName, lmf.implMethod.getName, lmf.implMethod.getDesc): Either[OptimizerWarning, (MethodNode, InternalName)] + methodDeclClass = classBTypeFromParsedClassfile(methodDeclClassNode) + res <- memberIsAccessible(methodNode.access, methodDeclClass, methodRefClass, destinationClass) + } yield { + res + } + + case indy: InvokeDynamicInsnNode => + if (destinationClass == calleeDeclarationClass) Right(true) // within the same class, any indy instruction can be inlined - } else if (closureOptimizer.isClosureInstantiation(indy)) { - val implMethod = indy.bsmArgs(1).asInstanceOf[Handle] // safe, checked in isClosureInstantiation - val methodRefClass = classBTypeFromParsedClassfile(implMethod.getOwner) - for { - (methodNode, methodDeclClassNode) <- byteCodeRepository.methodNode(methodRefClass.internalName, implMethod.getName, implMethod.getDesc): Either[OptimizerWarning, (MethodNode, InternalName)] - methodDeclClass = classBTypeFromParsedClassfile(methodDeclClassNode) - res <- memberIsAccessible(methodNode.access, methodDeclClass, methodRefClass, destinationClass) - } yield { - res - } - } else { + else Left(UnknownInvokeDynamicInstruction) - } case ci: LdcInsnNode => ci.cst match { case t: asm.Type => classIsAccessible(bTypeForDescriptorOrInternalNameFromClassfile(t.getInternalName), destinationClass) -- cgit v1.2.3 From 41b99e25317cc50a6ae6afa1a5527694236528ff Mon Sep 17 00:00:00 2001 From: Adriaan Moors Date: Tue, 7 Jul 2015 16:02:56 -0700 Subject: [backport] Integrate the LMFInvokeDynamic extractor into LambdaMetaFactoryCall --- .../tools/nsc/backend/jvm/opt/BytecodeUtils.scala | 2 + .../tools/nsc/backend/jvm/opt/CallGraph.scala | 115 ++++++++++----------- .../scala/tools/nsc/backend/jvm/opt/Inliner.scala | 17 +-- 3 files changed, 64 insertions(+), 70 deletions(-) diff --git a/src/compiler/scala/tools/nsc/backend/jvm/opt/BytecodeUtils.scala b/src/compiler/scala/tools/nsc/backend/jvm/opt/BytecodeUtils.scala index cd36fd8bba..df8dcc690a 100644 --- a/src/compiler/scala/tools/nsc/backend/jvm/opt/BytecodeUtils.scala +++ b/src/compiler/scala/tools/nsc/backend/jvm/opt/BytecodeUtils.scala @@ -104,6 +104,8 @@ object BytecodeUtils { def isStrictfpMethod(methodNode: MethodNode): Boolean = (methodNode.access & Opcodes.ACC_STRICT) != 0 + def isReference(t: Type) = t.getSort == Type.OBJECT || t.getSort == Type.ARRAY + def nextExecutableInstruction(instruction: AbstractInsnNode, alsoKeep: AbstractInsnNode => Boolean = Set()): Option[AbstractInsnNode] = { var result = instruction do { result = result.getNext } diff --git a/src/compiler/scala/tools/nsc/backend/jvm/opt/CallGraph.scala b/src/compiler/scala/tools/nsc/backend/jvm/opt/CallGraph.scala index 8c1673b4f2..96455c0e38 100644 --- a/src/compiler/scala/tools/nsc/backend/jvm/opt/CallGraph.scala +++ b/src/compiler/scala/tools/nsc/backend/jvm/opt/CallGraph.scala @@ -173,8 +173,8 @@ class CallGraph[BT <: BTypes](val btypes: BT) { callsitePosition = callsitePositions.getOrElse(call, NoPosition) ) - case LMFInvokeDynamic(lmf) => - closureInstantiations += lmf + case LambdaMetaFactoryCall(indy, samMethodType, implMethod, instantiatedMethodType) => + closureInstantiations += LambdaMetaFactoryCall(indy, samMethodType, implMethod, instantiatedMethodType) case _ => } @@ -242,7 +242,7 @@ class CallGraph[BT <: BTypes](val btypes: BT) { } final case class LambdaMetaFactoryCall(indy: InvokeDynamicInsnNode, samMethodType: Type, implMethod: Handle, instantiatedMethodType: Type) - object LMFInvokeDynamic { + object LambdaMetaFactoryCall { private val lambdaMetaFactoryInternalName: InternalName = "java/lang/invoke/LambdaMetafactory" private val metafactoryHandle = { @@ -257,69 +257,60 @@ class CallGraph[BT <: BTypes](val btypes: BT) { new Handle(Opcodes.H_INVOKESTATIC, lambdaMetaFactoryInternalName, altMetafactoryMethodName, altMetafactoryDesc) } - private def extractLambdaMetaFactoryCall(indy: InvokeDynamicInsnNode) = { - if (indy.bsm == metafactoryHandle || indy.bsm == altMetafactoryHandle) indy.bsmArgs match { - case Array(samMethodType: Type, implMethod: Handle, instantiatedMethodType: Type, xs@_*) => - // LambdaMetaFactory performs a number of automatic adaptations when invoking the lambda - // implementation method (casting, boxing, unboxing, and primitive widening, see Javadoc). - // - // The closure optimizer supports only one of those adaptations: it will cast arguments - // to the correct type when re-writing a closure call to the body method. Example: - // - // val fun: String => String = l => l - // val l = List("") - // fun(l.head) - // - // The samMethodType of Function1 is `(Object)Object`, while the instantiatedMethodType - // is `(String)String`. The return type of `List.head` is `Object`. - // - // The implMethod has the signature `C$anonfun(String)String`. - // - // At the closure callsite, we have an `INVOKEINTERFACE Function1.apply (Object)Object`, - // so the object returned by `List.head` can be directly passed into the call (no cast). - // - // The closure object will cast the object to String before passing it to the implMethod. - // - // When re-writing the closure callsite to the implMethod, we have to insert a cast. - // - // The check below ensures that - // (1) the implMethod type has the expected singature (captured types plus argument types - // from instantiatedMethodType) - // (2) the receiver of the implMethod matches the first captured type - // (3) all parameters that are not the same in samMethodType and instantiatedMethodType - // are reference types, so that we can insert casts to perform the same adaptation - // that the closure object would. - - val isStatic = implMethod.getTag == Opcodes.H_INVOKESTATIC - val indyParamTypes = Type.getArgumentTypes(indy.desc) - val instantiatedMethodArgTypes = instantiatedMethodType.getArgumentTypes - val expectedImplMethodType = { - val paramTypes = (if (isStatic) indyParamTypes else indyParamTypes.tail) ++ instantiatedMethodArgTypes - Type.getMethodType(instantiatedMethodType.getReturnType, paramTypes: _*) - } - - val isIndyLambda = { - Type.getType(implMethod.getDesc) == expectedImplMethodType // (1) - } && { - isStatic || implMethod.getOwner == indyParamTypes(0).getInternalName // (2) - } && { - def isReference(t: Type) = t.getSort == Type.OBJECT || t.getSort == Type.ARRAY - (samMethodType.getArgumentTypes, instantiatedMethodArgTypes).zipped forall { - case (samArgType, instArgType) => - samArgType == instArgType || isReference(samArgType) && isReference(instArgType) // (3) + def unapply(insn: AbstractInsnNode): Option[(InvokeDynamicInsnNode, Type, Handle, Type)] = insn match { + case indy: InvokeDynamicInsnNode if indy.bsm == metafactoryHandle || indy.bsm == altMetafactoryHandle => + indy.bsmArgs match { + case Array(samMethodType: Type, implMethod: Handle, instantiatedMethodType: Type, xs@_*) => // xs binding because IntelliJ gets confused about _@_* + // LambdaMetaFactory performs a number of automatic adaptations when invoking the lambda + // implementation method (casting, boxing, unboxing, and primitive widening, see Javadoc). + // + // The closure optimizer supports only one of those adaptations: it will cast arguments + // to the correct type when re-writing a closure call to the body method. Example: + // + // val fun: String => String = l => l + // val l = List("") + // fun(l.head) + // + // The samMethodType of Function1 is `(Object)Object`, while the instantiatedMethodType + // is `(String)String`. The return type of `List.head` is `Object`. + // + // The implMethod has the signature `C$anonfun(String)String`. + // + // At the closure callsite, we have an `INVOKEINTERFACE Function1.apply (Object)Object`, + // so the object returned by `List.head` can be directly passed into the call (no cast). + // + // The closure object will cast the object to String before passing it to the implMethod. + // + // When re-writing the closure callsite to the implMethod, we have to insert a cast. + // + // The check below ensures that + // (1) the implMethod type has the expected singature (captured types plus argument types + // from instantiatedMethodType) + // (2) the receiver of the implMethod matches the first captured type + // (3) all parameters that are not the same in samMethodType and instantiatedMethodType + // are reference types, so that we can insert casts to perform the same adaptation + // that the closure object would. + + val isStatic = implMethod.getTag == Opcodes.H_INVOKESTATIC + val indyParamTypes = Type.getArgumentTypes(indy.desc) + val instantiatedMethodArgTypes = instantiatedMethodType.getArgumentTypes + val expectedImplMethodType = { + val paramTypes = (if (isStatic) indyParamTypes else indyParamTypes.tail) ++ instantiatedMethodArgTypes + Type.getMethodType(instantiatedMethodType.getReturnType, paramTypes: _*) } - } - if (isIndyLambda) Some(LambdaMetaFactoryCall(indy, samMethodType, implMethod, instantiatedMethodType)) - else None + val isIndyLambda = ( + Type.getType(implMethod.getDesc) == expectedImplMethodType // (1) + && (isStatic || implMethod.getOwner == indyParamTypes(0).getInternalName) // (2) + && samMethodType.getArgumentTypes.corresponds(instantiatedMethodArgTypes)((samArgType, instArgType) => + samArgType == instArgType || isReference(samArgType) && isReference(instArgType)) // (3) + ) - case _ => None - } - else None - } + if (isIndyLambda) Some((indy, samMethodType, implMethod, instantiatedMethodType)) + else None - def unapply(insn: AbstractInsnNode): Option[LambdaMetaFactoryCall] = insn match { - case indy: InvokeDynamicInsnNode => extractLambdaMetaFactoryCall(indy) + case _ => None + } case _ => None } } diff --git a/src/compiler/scala/tools/nsc/backend/jvm/opt/Inliner.scala b/src/compiler/scala/tools/nsc/backend/jvm/opt/Inliner.scala index 3794ee9950..8477f5461a 100644 --- a/src/compiler/scala/tools/nsc/backend/jvm/opt/Inliner.scala +++ b/src/compiler/scala/tools/nsc/backend/jvm/opt/Inliner.scala @@ -688,7 +688,12 @@ class Inliner[BT <: BTypes](val btypes: BT) { } } - case LMFInvokeDynamic(lmf) => + case _: InvokeDynamicInsnNode if destinationClass == calleeDeclarationClass => + // within the same class, any indy instruction can be inlined + Right(true) + + // does the InvokeDynamicInsnNode call LambdaMetaFactory? + case LambdaMetaFactoryCall(_, _, implMethod, _) => // an indy instr points to a "call site specifier" (CSP) [1] // - a reference to a bootstrap method [2] // - bootstrap method name @@ -734,20 +739,16 @@ class Inliner[BT <: BTypes](val btypes: BT) { // the implMethod is public, lambdaMetaFactory doesn't use the Lookup object's extended // capability, and we can safely inline the instruction into a different class. - val methodRefClass = classBTypeFromParsedClassfile(lmf.implMethod.getOwner) + val methodRefClass = classBTypeFromParsedClassfile(implMethod.getOwner) for { - (methodNode, methodDeclClassNode) <- byteCodeRepository.methodNode(methodRefClass.internalName, lmf.implMethod.getName, lmf.implMethod.getDesc): Either[OptimizerWarning, (MethodNode, InternalName)] + (methodNode, methodDeclClassNode) <- byteCodeRepository.methodNode(methodRefClass.internalName, implMethod.getName, implMethod.getDesc): Either[OptimizerWarning, (MethodNode, InternalName)] methodDeclClass = classBTypeFromParsedClassfile(methodDeclClassNode) res <- memberIsAccessible(methodNode.access, methodDeclClass, methodRefClass, destinationClass) } yield { res } - case indy: InvokeDynamicInsnNode => - if (destinationClass == calleeDeclarationClass) - Right(true) // within the same class, any indy instruction can be inlined - else - Left(UnknownInvokeDynamicInstruction) + case _: InvokeDynamicInsnNode => Left(UnknownInvokeDynamicInstruction) case ci: LdcInsnNode => ci.cst match { case t: asm.Type => classIsAccessible(bTypeForDescriptorOrInternalNameFromClassfile(t.getInternalName), destinationClass) -- cgit v1.2.3 From f5e72765f2705c9bad2d87c4222ee64a2500f2fe Mon Sep 17 00:00:00 2001 From: Jason Zaugg Date: Thu, 9 Jul 2015 14:24:30 +1000 Subject: [backport] SI-9387 Fix VerifyError introduced by indylambda As with regular `Apply`-s, we should compute the generated type based on the function's type, rather than the expected type. In the test case, the expected type was void. Now, we correctly use the generated type of `scala/Function1`, which is enough to generate a subsequent POP instruction. The tree shape involved was: ``` arg0 = { { $anonfun() }; scala.runtime.BoxedUnit.UNIT } ``` --- .../tools/nsc/backend/jvm/BCodeBodyBuilder.scala | 3 ++- test/files/run/t9387.scala | 20 ++++++++++++++++++++ test/files/run/t9387b.check | 1 + test/files/run/t9387b.scala | 16 ++++++++++++++++ 4 files changed, 39 insertions(+), 1 deletion(-) create mode 100644 test/files/run/t9387.scala create mode 100644 test/files/run/t9387b.check create mode 100644 test/files/run/t9387b.scala diff --git a/src/compiler/scala/tools/nsc/backend/jvm/BCodeBodyBuilder.scala b/src/compiler/scala/tools/nsc/backend/jvm/BCodeBodyBuilder.scala index 40ba0c010b..67fc7923ea 100644 --- a/src/compiler/scala/tools/nsc/backend/jvm/BCodeBodyBuilder.scala +++ b/src/compiler/scala/tools/nsc/backend/jvm/BCodeBodyBuilder.scala @@ -632,10 +632,11 @@ abstract class BCodeBodyBuilder extends BCodeSkelBuilder { case _ => abort(s"Cannot instantiate $tpt of kind: $generatedType") } - case Apply(_, args) if app.hasAttachment[delambdafy.LambdaMetaFactoryCapable] => + case Apply(fun, args) if app.hasAttachment[delambdafy.LambdaMetaFactoryCapable] => val attachment = app.attachments.get[delambdafy.LambdaMetaFactoryCapable].get genLoadArguments(args, paramTKs(app)) genInvokeDynamicLambda(attachment.target, attachment.arity, attachment.functionalInterface) + generatedType = asmMethodType(fun.symbol).returnType case Apply(fun @ _, List(expr)) if currentRun.runDefinitions.isBox(fun.symbol) => val nativeKind = tpeTK(expr) diff --git a/test/files/run/t9387.scala b/test/files/run/t9387.scala new file mode 100644 index 0000000000..3e33d19fd2 --- /dev/null +++ b/test/files/run/t9387.scala @@ -0,0 +1,20 @@ +class G[T] +object G { + def v[T](x: T): G[T] = null +} + +class A[T] +object A { + def apply[T](x: => G[T]): A[T] = null +} + +object T { + A[Unit](G.v(() => ())) // Was VerifyError +} + +object Test { + def main(args: Array[String]): Unit = { + T + } + +} \ No newline at end of file diff --git a/test/files/run/t9387b.check b/test/files/run/t9387b.check new file mode 100644 index 0000000000..6a452c185a --- /dev/null +++ b/test/files/run/t9387b.check @@ -0,0 +1 @@ +() diff --git a/test/files/run/t9387b.scala b/test/files/run/t9387b.scala new file mode 100644 index 0000000000..6339f4caba --- /dev/null +++ b/test/files/run/t9387b.scala @@ -0,0 +1,16 @@ +object T { + val f: Unit = () => () + println(f) +} + +object U { + def f[T](t: T): T = t + f[Unit](() => ()) +} + +object Test { + def main(args: Array[String]): Unit = { + T + U + } +} -- cgit v1.2.3 From a1d471f7ba93f5a2e3a66a77596a41f2677ca9ee Mon Sep 17 00:00:00 2001 From: Lukas Rytz Date: Wed, 8 Jul 2015 22:59:56 +0200 Subject: [backport] Refactor the ClosureOptimizer, run ProdCons only once per method Refactor and clean up the ClosureOptimzier. The goal of this change is to run the ProdCons analyzer only once per method, instead of once per closure instantiation. Bootstrapping scala with `-Yopt-inline-heuristics:everything` revealed that ProdCons can take a very long time on large methods, for example ``` [quick.compiler] scala/tools/nsc/backend/jvm/BCodeBodyBuilder$PlainBodyBuilder#genArithmeticOp - analysis: 1 spans, 17755ms [quick.compiler] scala/tools/nsc/typechecker/SuperAccessors$SuperAccTransformer#transform - analysis: 1 spans, 28024ms [quick.compiler] scala/tools/nsc/backend/jvm/BCodeBodyBuilder$PlainBodyBuilder#genInvokeDynamicLambda - analysis: 1 spans, 22100ms ``` With this change and enough time and space (-Xmx6000m), bootstrapping scala succeeds in this test mode. --- .../tools/nsc/backend/jvm/BackendReporting.scala | 2 +- .../backend/jvm/analysis/ProdConsAnalyzer.scala | 20 ++ .../nsc/backend/jvm/opt/ClosureOptimizer.scala | 367 ++++++++++++--------- 3 files changed, 231 insertions(+), 158 deletions(-) diff --git a/src/compiler/scala/tools/nsc/backend/jvm/BackendReporting.scala b/src/compiler/scala/tools/nsc/backend/jvm/BackendReporting.scala index 4eb24d13e3..b41d0de92f 100644 --- a/src/compiler/scala/tools/nsc/backend/jvm/BackendReporting.scala +++ b/src/compiler/scala/tools/nsc/backend/jvm/BackendReporting.scala @@ -255,7 +255,7 @@ object BackendReporting { * Used in `rewriteClosureApplyInvocations` when a closure apply callsite cannot be rewritten * to the closure body method. */ - trait RewriteClosureApplyToClosureBodyFailed extends OptimizerWarning { + sealed trait RewriteClosureApplyToClosureBodyFailed extends OptimizerWarning { def pos: Position override def emitWarning(settings: ScalaSettings): Boolean = this match { diff --git a/src/compiler/scala/tools/nsc/backend/jvm/analysis/ProdConsAnalyzer.scala b/src/compiler/scala/tools/nsc/backend/jvm/analysis/ProdConsAnalyzer.scala index cf63bf54a4..700b2f2f6c 100644 --- a/src/compiler/scala/tools/nsc/backend/jvm/analysis/ProdConsAnalyzer.scala +++ b/src/compiler/scala/tools/nsc/backend/jvm/analysis/ProdConsAnalyzer.scala @@ -57,8 +57,20 @@ import scala.collection.convert.decorateAsScala._ * copying operations. */ class ProdConsAnalyzer(methodNode: MethodNode, classInternalName: InternalName) { + + /* Timers for benchmarking ProdCons + import scala.reflect.internal.util.Statistics._ + import ProdConsAnalyzer._ + val analyzerTimer = newSubTimer(classInternalName + "#" + methodNode.name + " - analysis", prodConsAnalyzerTimer) + val consumersTimer = newSubTimer(classInternalName + "#" + methodNode.name + " - consumers", prodConsAnalyzerTimer) + */ + val analyzer = new Analyzer(new InitialProducerSourceInterpreter) + +// val start = analyzerTimer.start() analyzer.analyze(classInternalName, methodNode) +// analyzerTimer.stop(start) +// println(analyzerTimer.line) def frameAt(insn: AbstractInsnNode) = analyzer.frameAt(insn, methodNode) @@ -392,6 +404,7 @@ class ProdConsAnalyzer(methodNode: MethodNode, classInternalName: InternalName) /** For each instruction, a set of potential consumers of the produced values. */ private lazy val _consumersOfOutputsFrom: Map[AbstractInsnNode, Vector[Set[AbstractInsnNode]]] = { +// val start = consumersTimer.start() var res = Map.empty[AbstractInsnNode, Vector[Set[AbstractInsnNode]]] for { insn <- methodNode.instructions.iterator.asScala @@ -404,6 +417,8 @@ class ProdConsAnalyzer(methodNode: MethodNode, classInternalName: InternalName) val outputIndex = producedSlots.indexOf(i) res = res.updated(producer, currentConsumers.updated(outputIndex, currentConsumers(outputIndex) + insn)) } +// consumersTimer.stop(start) +// println(consumersTimer.line) res } @@ -411,6 +426,11 @@ class ProdConsAnalyzer(methodNode: MethodNode, classInternalName: InternalName) private val _ultimateConsumersCache: mutable.AnyRefMap[(AbstractInsnNode, Int), Set[AbstractInsnNode]] = mutable.AnyRefMap.empty } +object ProdConsAnalyzer { + import scala.reflect.internal.util.Statistics._ + val prodConsAnalyzerTimer = newTimer("Time in ProdConsAnalyzer", "jvm") +} + /** * A class for pseudo-instructions representing the initial producers of local values that have * no producer instruction in the method: diff --git a/src/compiler/scala/tools/nsc/backend/jvm/opt/ClosureOptimizer.scala b/src/compiler/scala/tools/nsc/backend/jvm/opt/ClosureOptimizer.scala index 86536ff0d2..58f0813bd8 100644 --- a/src/compiler/scala/tools/nsc/backend/jvm/opt/ClosureOptimizer.scala +++ b/src/compiler/scala/tools/nsc/backend/jvm/opt/ClosureOptimizer.scala @@ -8,9 +8,9 @@ package backend.jvm package opt import scala.annotation.switch -import scala.collection.mutable +import scala.collection.immutable import scala.reflect.internal.util.NoPosition -import scala.tools.asm.{Handle, Type, Opcodes} +import scala.tools.asm.{Type, Opcodes} import scala.tools.asm.tree._ import scala.tools.nsc.backend.jvm.BTypes.InternalName import scala.tools.nsc.backend.jvm.analysis.ProdConsAnalyzer @@ -24,6 +24,35 @@ class ClosureOptimizer[BT <: BTypes](val btypes: BT) { import btypes._ import callGraph._ + /** + * If a closure is allocated and invoked within the same method, re-write the invocation to the + * closure body method. + * + * Note that the closure body method (generated by delambdafy:method) takes additional parameters + * for the values captured by the closure. The bytecode is transformed from + * + * [generate captured values] + * [closure init, capturing values] + * [...] + * [load closure object] + * [generate closure invocation arguments] + * [invoke closure.apply] + * + * to + * + * [generate captured values] + * [store captured values into new locals] + * [load the captured values from locals] // a future optimization will eliminate the closure + * [closure init, capturing values] // instantiation if the closure object becomes unused + * [...] + * [load closure object] + * [generate closure invocation arguments] + * [store argument values into new locals] + * [drop the closure object] + * [load captured values from locals] + * [load argument values from locals] + * [invoke the closure body method] + */ def rewriteClosureApplyInvocations(): Unit = { implicit object closureInitOrdering extends Ordering[ClosureInstantiation] { override def compare(x: ClosureInstantiation, y: ClosureInstantiation): Int = { @@ -41,16 +70,111 @@ class ClosureOptimizer[BT <: BTypes](val btypes: BT) { } } - val sorted = mutable.TreeSet.empty[ClosureInstantiation] - sorted ++= closureInstantiations.values + // Group the closure instantiations by method allows running the ProdConsAnalyzer only once per method. + // Also sort the instantiations: If there are multiple closure instantiations in a method, closure + // invocations need to be re-written in a consistent order for bytecode stability. The local variable + // slots for storing captured values depends on the order of rewriting. + val closureInstantiationsByMethod: Map[MethodNode, immutable.TreeSet[ClosureInstantiation]] = { + closureInstantiations.values.groupBy(_.ownerMethod).mapValues(immutable.TreeSet.empty ++ _) + } - for (closureInst <- sorted) { - val warnings = rewriteClosureApplyInvocations(closureInst) - warnings.foreach(w => backendReporting.inlinerWarning(w.pos, w.toString)) + // For each closure instantiation, a list of callsites of the closure that can be re-written + // If a callsite cannot be rewritten, for example because the lambda body method is not accessible, + // a warning is returned instead. + val callsitesToRewrite: Map[ClosureInstantiation, List[Either[RewriteClosureApplyToClosureBodyFailed, (MethodInsnNode, Int)]]] = { + closureInstantiationsByMethod flatMap { + case (methodNode, closureInits) => + // A lazy val to ensure the analysis only runs if necessary (the value is passed by name to `closureCallsites`) + lazy val prodCons = new ProdConsAnalyzer(methodNode, closureInits.head.ownerClass.internalName) + closureInits.map(init => (init, closureCallsites(init, prodCons))) + } + } + + // Rewrite all closure callsites (or issue inliner warnings for those that cannot be rewritten) + for ((closureInit, callsites) <- callsitesToRewrite) { + // Local variables that hold the captured values and the closure invocation arguments. + // They are lazy vals to ensure that locals for captured values are only allocated if there's + // actually a callsite to rewrite (an not only warnings to be issued). + lazy val (localsForCapturedValues, argumentLocalsList) = localsForClosureRewrite(closureInit) + for (callsite <- callsites) callsite match { + case Left(warning) => + backendReporting.inlinerWarning(warning.pos, warning.toString) + + case Right((invocation, stackHeight)) => + rewriteClosureApplyInvocation(closureInit, invocation, stackHeight, localsForCapturedValues, argumentLocalsList) + } } } - def isSamInvocation(invocation: MethodInsnNode, indy: InvokeDynamicInsnNode, prodCons: => ProdConsAnalyzer): Boolean = { + /** + * Insert instructions to store the values captured by a closure instantiation into local variables, + * and load the values back to the stack. + * + * Returns the list of locals holding those captured values, and a list of locals that should be + * used at the closure invocation callsite to store the arguments passed to the closure invocation. + */ + private def localsForClosureRewrite(closureInit: ClosureInstantiation): (LocalsList, LocalsList) = { + val ownerMethod = closureInit.ownerMethod + val captureLocals = storeCaptures(closureInit) + + // allocate locals for storing the arguments of the closure apply callsites. + // if there are multiple callsites, the same locals are re-used. + val argTypes = closureInit.lambdaMetaFactoryCall.samMethodType.getArgumentTypes + val firstArgLocal = ownerMethod.maxLocals + + // The comment in the unapply method of `LambdaMetaFactoryCall` explains why we have to introduce + // casts for arguments that have different types in samMethodType and instantiatedMethodType. + val castLoadTypes = { + val instantiatedMethodType = closureInit.lambdaMetaFactoryCall.instantiatedMethodType + (argTypes, instantiatedMethodType.getArgumentTypes).zipped map { + case (samArgType, instantiatedArgType) if samArgType != instantiatedArgType => + // the LambdaMetaFactoryCall extractor ensures that the two types are reference types, + // so we don't end up casting primitive values. + Some(instantiatedArgType) + case _ => + None + } + } + val argLocals = LocalsList.fromTypes(firstArgLocal, argTypes, castLoadTypes) + ownerMethod.maxLocals = firstArgLocal + argLocals.size + + (captureLocals, argLocals) + } + + /** + * Find all callsites of a closure within the method where the closure is allocated. + */ + private def closureCallsites(closureInit: ClosureInstantiation, prodCons: => ProdConsAnalyzer): List[Either[RewriteClosureApplyToClosureBodyFailed, (MethodInsnNode, Int)]] = { + val ownerMethod = closureInit.ownerMethod + val ownerClass = closureInit.ownerClass + val lambdaBodyHandle = closureInit.lambdaMetaFactoryCall.implMethod + + ownerMethod.instructions.iterator.asScala.collect({ + case invocation: MethodInsnNode if isSamInvocation(invocation, closureInit, prodCons) => + // TODO: This is maybe over-cautious. + // We are checking if the closure body method is accessible at the closure callsite. + // If the closure allocation has access to the body method, then the callsite (in the same + // method as the alloction) should have access too. + val bodyAccessible: Either[OptimizerWarning, Boolean] = for { + (bodyMethodNode, declClass) <- byteCodeRepository.methodNode(lambdaBodyHandle.getOwner, lambdaBodyHandle.getName, lambdaBodyHandle.getDesc): Either[OptimizerWarning, (MethodNode, InternalName)] + isAccessible <- inliner.memberIsAccessible(bodyMethodNode.access, classBTypeFromParsedClassfile(declClass), classBTypeFromParsedClassfile(lambdaBodyHandle.getOwner), ownerClass) + } yield { + isAccessible + } + + def pos = callGraph.callsites.get(invocation).map(_.callsitePosition).getOrElse(NoPosition) + val stackSize: Either[RewriteClosureApplyToClosureBodyFailed, Int] = bodyAccessible match { + case Left(w) => Left(RewriteClosureAccessCheckFailed(pos, w)) + case Right(false) => Left(RewriteClosureIllegalAccess(pos, ownerClass.internalName)) + case _ => Right(prodCons.frameAt(invocation).getStackSize) + } + + stackSize.right.map((invocation, _)) + }).toList + } + + private def isSamInvocation(invocation: MethodInsnNode, closureInit: ClosureInstantiation, prodCons: => ProdConsAnalyzer): Boolean = { + val indy = closureInit.lambdaMetaFactoryCall.indy if (invocation.getOpcode == INVOKESTATIC) false else { def closureIsReceiver = { @@ -64,16 +188,90 @@ class ClosureOptimizer[BT <: BTypes](val btypes: BT) { } invocation.name == indy.name && { - val indySamMethodDesc = indy.bsmArgs(0).asInstanceOf[Type].getDescriptor // safe, checked in isClosureInstantiation + val indySamMethodDesc = closureInit.lambdaMetaFactoryCall.samMethodType.getDescriptor indySamMethodDesc == invocation.desc } && - closureIsReceiver // most expensive check last + closureIsReceiver // most expensive check last } } + private def rewriteClosureApplyInvocation(closureInit: ClosureInstantiation, invocation: MethodInsnNode, stackHeight: Int, localsForCapturedValues: LocalsList, argumentLocalsList: LocalsList): Unit = { + val ownerMethod = closureInit.ownerMethod + val lambdaBodyHandle = closureInit.lambdaMetaFactoryCall.implMethod + + // store arguments + insertStoreOps(invocation, ownerMethod, argumentLocalsList) + + // drop the closure from the stack + ownerMethod.instructions.insertBefore(invocation, new InsnNode(POP)) + + // load captured values and arguments + insertLoadOps(invocation, ownerMethod, localsForCapturedValues) + insertLoadOps(invocation, ownerMethod, argumentLocalsList) + + // update maxStack + val capturesStackSize = localsForCapturedValues.size + val invocationStackHeight = stackHeight + capturesStackSize - 1 // -1 because the closure is gone + if (invocationStackHeight > ownerMethod.maxStack) + ownerMethod.maxStack = invocationStackHeight + + // replace the callsite with a new call to the body method + val bodyOpcode = (lambdaBodyHandle.getTag: @switch) match { + case H_INVOKEVIRTUAL => INVOKEVIRTUAL + case H_INVOKESTATIC => INVOKESTATIC + case H_INVOKESPECIAL => INVOKESPECIAL + case H_INVOKEINTERFACE => INVOKEINTERFACE + case H_NEWINVOKESPECIAL => + val insns = ownerMethod.instructions + insns.insertBefore(invocation, new TypeInsnNode(NEW, lambdaBodyHandle.getOwner)) + insns.insertBefore(invocation, new InsnNode(DUP)) + INVOKESPECIAL + } + val isInterface = bodyOpcode == INVOKEINTERFACE + val bodyInvocation = new MethodInsnNode(bodyOpcode, lambdaBodyHandle.getOwner, lambdaBodyHandle.getName, lambdaBodyHandle.getDesc, isInterface) + ownerMethod.instructions.insertBefore(invocation, bodyInvocation) + + val returnType = Type.getReturnType(lambdaBodyHandle.getDesc) + fixLoadedNothingOrNullValue(returnType, bodyInvocation, ownerMethod, btypes) // see comment of that method + + ownerMethod.instructions.remove(invocation) + + // update the call graph + val originalCallsite = callGraph.callsites.remove(invocation) + + // the method node is needed for building the call graph entry + val bodyMethod = byteCodeRepository.methodNode(lambdaBodyHandle.getOwner, lambdaBodyHandle.getName, lambdaBodyHandle.getDesc) + def bodyMethodIsBeingCompiled = byteCodeRepository.classNodeAndSource(lambdaBodyHandle.getOwner).map(_._2 == CompilationUnit).getOrElse(false) + val bodyMethodCallsite = Callsite( + callsiteInstruction = bodyInvocation, + callsiteMethod = ownerMethod, + callsiteClass = closureInit.ownerClass, + callee = bodyMethod.map({ + case (bodyMethodNode, bodyMethodDeclClass) => Callee( + callee = bodyMethodNode, + calleeDeclarationClass = classBTypeFromParsedClassfile(bodyMethodDeclClass), + safeToInline = compilerSettings.YoptInlineGlobal || bodyMethodIsBeingCompiled, + safeToRewrite = false, // the lambda body method is not a trait interface method + annotatedInline = false, + annotatedNoInline = false, + calleeInfoWarning = None) + }), + argInfos = Nil, + callsiteStackHeight = invocationStackHeight, + receiverKnownNotNull = true, // see below (*) + callsitePosition = originalCallsite.map(_.callsitePosition).getOrElse(NoPosition) + ) + // (*) The documentation in class LambdaMetafactory says: + // "if implMethod corresponds to an instance method, the first capture argument + // (corresponding to the receiver) must be non-null" + // Explanation: If the lambda body method is non-static, the receiver is a captured + // value. It can only be captured within some instance method, so we know it's non-null. + callGraph.callsites(bodyInvocation) = bodyMethodCallsite + } + /** - * Stores the values captured by a closure creation into fresh local variables. - * Returns the list of locals holding the captured values. + * Stores the values captured by a closure creation into fresh local variables, and loads the + * values back onto the stack. Returns the list of locals holding the captured values. */ private def storeCaptures(closureInit: ClosureInstantiation): LocalsList = { val indy = closureInit.lambdaMetaFactoryCall.indy @@ -129,151 +327,6 @@ class ClosureOptimizer[BT <: BTypes](val btypes: BT) { } } - def rewriteClosureApplyInvocations(closureInit: ClosureInstantiation): List[RewriteClosureApplyToClosureBodyFailed] = { - val lambdaBodyHandle = closureInit.lambdaMetaFactoryCall.implMethod - val ownerMethod = closureInit.ownerMethod - val ownerClass = closureInit.ownerClass - - // Kept as a lazy val to make sure the analysis is only computed if it's actually needed. - // ProdCons is used to identify closure body invocations (see isSamInvocation), but only if the - // callsite has the right name and signature. If the method has no invcation instruction with - // the right name and signature, the analysis is not executed. - lazy val prodCons = new ProdConsAnalyzer(ownerMethod, ownerClass.internalName) - - // First collect all callsites without modifying the instructions list yet. - // Once we start modifying the instruction list, prodCons becomes unusable. - - // A list of callsites and stack heights. If the invocation cannot be rewritten, a warning - // message is stored in the stack height value. - val invocationsToRewrite: List[(MethodInsnNode, Either[RewriteClosureApplyToClosureBodyFailed, Int])] = ownerMethod.instructions.iterator.asScala.collect({ - case invocation: MethodInsnNode if isSamInvocation(invocation, closureInit.lambdaMetaFactoryCall.indy, prodCons) => - val bodyAccessible: Either[OptimizerWarning, Boolean] = for { - (bodyMethodNode, declClass) <- byteCodeRepository.methodNode(lambdaBodyHandle.getOwner, lambdaBodyHandle.getName, lambdaBodyHandle.getDesc): Either[OptimizerWarning, (MethodNode, InternalName)] - isAccessible <- inliner.memberIsAccessible(bodyMethodNode.access, classBTypeFromParsedClassfile(declClass), classBTypeFromParsedClassfile(lambdaBodyHandle.getOwner), ownerClass) - } yield { - isAccessible - } - - def pos = callGraph.callsites.get(invocation).map(_.callsitePosition).getOrElse(NoPosition) - val stackSize: Either[RewriteClosureApplyToClosureBodyFailed, Int] = bodyAccessible match { - case Left(w) => Left(RewriteClosureAccessCheckFailed(pos, w)) - case Right(false) => Left(RewriteClosureIllegalAccess(pos, ownerClass.internalName)) - case _ => Right(prodCons.frameAt(invocation).getStackSize) - } - - (invocation, stackSize) - }).toList - - if (invocationsToRewrite.isEmpty) Nil - else { - // lazy val to make sure locals for captures and arguments are only allocated if there's - // effectively a callsite to rewrite. - lazy val (localsForCapturedValues, argumentLocalsList) = { - val captureLocals = storeCaptures(closureInit) - - // allocate locals for storing the arguments of the closure apply callsites. - // if there are multiple callsites, the same locals are re-used. - val argTypes = closureInit.lambdaMetaFactoryCall.samMethodType.getArgumentTypes - val firstArgLocal = ownerMethod.maxLocals - - // The comment in `isClosureInstantiation` explains why we have to introduce casts for - // arguments that have different types in samMethodType and instantiatedMethodType. - val castLoadTypes = { - val instantiatedMethodType = closureInit.lambdaMetaFactoryCall.instantiatedMethodType - (argTypes, instantiatedMethodType.getArgumentTypes).zipped map { - case (samArgType, instantiatedArgType) if samArgType != instantiatedArgType => - // isClosureInstantiation ensures that the two types are reference types, so we don't - // end up casting primitive values. - Some(instantiatedArgType) - case _ => - None - } - } - val argLocals = LocalsList.fromTypes(firstArgLocal, argTypes, castLoadTypes) - ownerMethod.maxLocals = firstArgLocal + argLocals.size - - (captureLocals, argLocals) - } - - val warnings = invocationsToRewrite flatMap { - case (invocation, Left(warning)) => Some(warning) - - case (invocation, Right(stackHeight)) => - // store arguments - insertStoreOps(invocation, ownerMethod, argumentLocalsList) - - // drop the closure from the stack - ownerMethod.instructions.insertBefore(invocation, new InsnNode(POP)) - - // load captured values and arguments - insertLoadOps(invocation, ownerMethod, localsForCapturedValues) - insertLoadOps(invocation, ownerMethod, argumentLocalsList) - - // update maxStack - val capturesStackSize = localsForCapturedValues.size - val invocationStackHeight = stackHeight + capturesStackSize - 1 // -1 because the closure is gone - if (invocationStackHeight > ownerMethod.maxStack) - ownerMethod.maxStack = invocationStackHeight - - // replace the callsite with a new call to the body method - val bodyOpcode = (lambdaBodyHandle.getTag: @switch) match { - case H_INVOKEVIRTUAL => INVOKEVIRTUAL - case H_INVOKESTATIC => INVOKESTATIC - case H_INVOKESPECIAL => INVOKESPECIAL - case H_INVOKEINTERFACE => INVOKEINTERFACE - case H_NEWINVOKESPECIAL => - val insns = ownerMethod.instructions - insns.insertBefore(invocation, new TypeInsnNode(NEW, lambdaBodyHandle.getOwner)) - insns.insertBefore(invocation, new InsnNode(DUP)) - INVOKESPECIAL - } - val isInterface = bodyOpcode == INVOKEINTERFACE - val bodyInvocation = new MethodInsnNode(bodyOpcode, lambdaBodyHandle.getOwner, lambdaBodyHandle.getName, lambdaBodyHandle.getDesc, isInterface) - ownerMethod.instructions.insertBefore(invocation, bodyInvocation) - - val returnType = Type.getReturnType(lambdaBodyHandle.getDesc) - fixLoadedNothingOrNullValue(returnType, bodyInvocation, ownerMethod, btypes) // see comment of that method - - ownerMethod.instructions.remove(invocation) - - // update the call graph - val originalCallsite = callGraph.callsites.remove(invocation) - - // the method node is needed for building the call graph entry - val bodyMethod = byteCodeRepository.methodNode(lambdaBodyHandle.getOwner, lambdaBodyHandle.getName, lambdaBodyHandle.getDesc) - def bodyMethodIsBeingCompiled = byteCodeRepository.classNodeAndSource(lambdaBodyHandle.getOwner).map(_._2 == CompilationUnit).getOrElse(false) - val bodyMethodCallsite = Callsite( - callsiteInstruction = bodyInvocation, - callsiteMethod = ownerMethod, - callsiteClass = ownerClass, - callee = bodyMethod.map({ - case (bodyMethodNode, bodyMethodDeclClass) => Callee( - callee = bodyMethodNode, - calleeDeclarationClass = classBTypeFromParsedClassfile(bodyMethodDeclClass), - safeToInline = compilerSettings.YoptInlineGlobal || bodyMethodIsBeingCompiled, - safeToRewrite = false, // the lambda body method is not a trait interface method - annotatedInline = false, - annotatedNoInline = false, - calleeInfoWarning = None) - }), - argInfos = Nil, - callsiteStackHeight = invocationStackHeight, - receiverKnownNotNull = true, // see below (*) - callsitePosition = originalCallsite.map(_.callsitePosition).getOrElse(NoPosition) - ) - // (*) The documentation in class LambdaMetafactory says: - // "if implMethod corresponds to an instance method, the first capture argument - // (corresponding to the receiver) must be non-null" - // Explanation: If the lambda body method is non-static, the receiver is a captured - // value. It can only be captured within some instance method, so we know it's non-null. - callGraph.callsites(bodyInvocation) = bodyMethodCallsite - None - } - - warnings.toList - } - } - /** * A list of local variables. Each local stores information about its type, see class [[Local]]. */ -- cgit v1.2.3 From 6177cb44812b92b4059ec621a34cc26ddbe532c1 Mon Sep 17 00:00:00 2001 From: Jason Zaugg Date: Sun, 12 Jul 2015 19:23:26 +1000 Subject: [backport] SI-9393 Temporarily disable two assertions in GenBCode These cause a crash in the build of Play. We should try to bring these back once we have suitable annotation awareness. Perhaps they should only be `devWarning`-s, though. --- .../scala/tools/nsc/backend/jvm/BTypes.scala | 21 +++++++++++---------- test/files/pos/t9393/Named.java | 3 +++ test/files/pos/t9393/NamedImpl.java | 15 +++++++++++++++ test/files/pos/t9393/test.scala | 3 +++ 4 files changed, 32 insertions(+), 10 deletions(-) create mode 100644 test/files/pos/t9393/Named.java create mode 100644 test/files/pos/t9393/NamedImpl.java create mode 100644 test/files/pos/t9393/test.scala diff --git a/src/compiler/scala/tools/nsc/backend/jvm/BTypes.scala b/src/compiler/scala/tools/nsc/backend/jvm/BTypes.scala index 8720da84e8..9bae63f1fc 100644 --- a/src/compiler/scala/tools/nsc/backend/jvm/BTypes.scala +++ b/src/compiler/scala/tools/nsc/backend/jvm/BTypes.scala @@ -841,16 +841,17 @@ abstract class BTypes { assert(!ClassBType.isInternalPhantomType(internalName), s"Cannot create ClassBType for phantom type $this") - assert( - if (info.get.superClass.isEmpty) { isJLO(this) || (isCompilingPrimitive && ClassBType.hasNoSuper(internalName)) } - else if (isInterface.get) isJLO(info.get.superClass.get) - else !isJLO(this) && ifInit(info.get.superClass.get)(!_.isInterface.get), - s"Invalid superClass in $this: ${info.get.superClass}" - ) - assert( - info.get.interfaces.forall(c => ifInit(c)(_.isInterface.get)), - s"Invalid interfaces in $this: ${info.get.interfaces}" - ) + // TODO bring these back in a way that doesn't trip pos/t9393 + // assert( + // if (info.get.superClass.isEmpty) { isJLO(this) || (isCompilingPrimitive && ClassBType.hasNoSuper(internalName)) } + // else if (isInterface.get) isJLO(info.get.superClass.get) + // else !isJLO(this) && ifInit(info.get.superClass.get)(!_.isInterface.get), + // s"Invalid superClass in $this: ${info.get.superClass}" + // ) + // assert( + // info.get.interfaces.forall(c => ifInit(c)(_.isInterface.get)), + // s"Invalid interfaces in $this: ${info.get.interfaces}" + // ) assert(info.get.nestedClasses.forall(c => ifInit(c)(_.isNestedClass.get)), info.get.nestedClasses) } diff --git a/test/files/pos/t9393/Named.java b/test/files/pos/t9393/Named.java new file mode 100644 index 0000000000..144ddbf26e --- /dev/null +++ b/test/files/pos/t9393/Named.java @@ -0,0 +1,3 @@ +package bug; + +public @interface Named {} diff --git a/test/files/pos/t9393/NamedImpl.java b/test/files/pos/t9393/NamedImpl.java new file mode 100644 index 0000000000..7918739c2b --- /dev/null +++ b/test/files/pos/t9393/NamedImpl.java @@ -0,0 +1,15 @@ +/* + * Copyright (C) 2009-2015 Typesafe Inc. + */ +package bug; + +import bug.Named; +import java.io.Serializable; +import java.lang.annotation.Annotation; + +public class NamedImpl implements Named { + + public Class annotationType() { + return null; + } +} diff --git a/test/files/pos/t9393/test.scala b/test/files/pos/t9393/test.scala new file mode 100644 index 0000000000..4df0476c98 --- /dev/null +++ b/test/files/pos/t9393/test.scala @@ -0,0 +1,3 @@ +class C { + new bug.NamedImpl +} -- cgit v1.2.3 From 091c1e6ed8254fbd75a21843e2b9d331cbabf9d8 Mon Sep 17 00:00:00 2001 From: Jason Zaugg Date: Mon, 13 Jul 2015 10:13:23 +1000 Subject: [backport] SI-9392 Avoid crash in GenBCode for incoherent trees A macro in shapeless was generating a tree of the form: ``` { class C#2 new C#2 }.setType(C#1) ``` This happened due to an error in the macro; it used untypecheck to try to fix the owner-chain consistency problem, but kept a reference to the previous version of the block-local class symbol `C` and used this in the resulting tree. This commit detects the particular situation we encountered, and avoids the crash by not creating the `NestedInfo` for the `BType` corresponding to `C#1`. The code comment discusses why I think this is safe, and suggests a refactoring that would mean we only ever try to construct `NestedInfo` when we are going to need them. --- .../scala/tools/nsc/backend/jvm/BTypesFromSymbols.scala | 8 ++++++++ test/files/pos/t9392/client_2.scala | 4 ++++ test/files/pos/t9392/macro_1.scala | 16 ++++++++++++++++ 3 files changed, 28 insertions(+) create mode 100644 test/files/pos/t9392/client_2.scala create mode 100644 test/files/pos/t9392/macro_1.scala diff --git a/src/compiler/scala/tools/nsc/backend/jvm/BTypesFromSymbols.scala b/src/compiler/scala/tools/nsc/backend/jvm/BTypesFromSymbols.scala index cf29c8090b..7b238e56eb 100644 --- a/src/compiler/scala/tools/nsc/backend/jvm/BTypesFromSymbols.scala +++ b/src/compiler/scala/tools/nsc/backend/jvm/BTypesFromSymbols.scala @@ -351,6 +351,14 @@ class BTypesFromSymbols[G <: Global](val global: G) extends BTypes { val isTopLevel = innerClassSym.rawowner.isPackageClass // impl classes are considered top-level, see comment in BTypes if (isTopLevel || considerAsTopLevelImplementationArtifact(innerClassSym)) None + else if (innerClassSym.rawowner.isTerm) + // SI-9392 An errant macro might leave a reference to a local class symbol that no longer exists in the tree, + // this avoids an assertion error in that case. AFAICT, we don't actually need the `NestedInfo` for all BTypes, + // only for ones that describe classes defined in the trees that reach the backend, so this is safe enough. + // + // TODO Can we avoid creating `NestedInfo` for each type that is referred to, and instead only create if for + // symbols of ClassDefs? + None else { // See comment in BTypes, when is a class marked static in the InnerClass table. val isStaticNestedClass = isOriginallyStaticOwner(innerClassSym.originalOwner) diff --git a/test/files/pos/t9392/client_2.scala b/test/files/pos/t9392/client_2.scala new file mode 100644 index 0000000000..6b706fea12 --- /dev/null +++ b/test/files/pos/t9392/client_2.scala @@ -0,0 +1,4 @@ +class Client { + Macro() +} + diff --git a/test/files/pos/t9392/macro_1.scala b/test/files/pos/t9392/macro_1.scala new file mode 100644 index 0000000000..3f67ac17b2 --- /dev/null +++ b/test/files/pos/t9392/macro_1.scala @@ -0,0 +1,16 @@ +import language.experimental.macros + + +object Macro { + + import reflect.macros.blackbox.Context + def impl(c: Context)(): c.Tree = { + import c.universe._ + val tree = q"""class C; new C""" + val tree1 = c.typecheck(tree) + val tpe = tree1.tpe + val tree2 = c.typecheck(c.untypecheck(tree1)) + q"""$tree2.asInstanceOf[$tpe]""" + } + def apply(): Any = macro impl +} -- cgit v1.2.3 From 3b6b2bfe9fdd7995bf9f90a5fafc30613a101179 Mon Sep 17 00:00:00 2001 From: Lukas Rytz Date: Wed, 15 Jul 2015 10:19:42 +0200 Subject: [backport] SI-9392 Clarify the workaround comment and introduce a devWarning --- .../scala/tools/nsc/backend/jvm/BTypesFromSymbols.scala | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/compiler/scala/tools/nsc/backend/jvm/BTypesFromSymbols.scala b/src/compiler/scala/tools/nsc/backend/jvm/BTypesFromSymbols.scala index 7b238e56eb..9b4451d492 100644 --- a/src/compiler/scala/tools/nsc/backend/jvm/BTypesFromSymbols.scala +++ b/src/compiler/scala/tools/nsc/backend/jvm/BTypesFromSymbols.scala @@ -351,15 +351,15 @@ class BTypesFromSymbols[G <: Global](val global: G) extends BTypes { val isTopLevel = innerClassSym.rawowner.isPackageClass // impl classes are considered top-level, see comment in BTypes if (isTopLevel || considerAsTopLevelImplementationArtifact(innerClassSym)) None - else if (innerClassSym.rawowner.isTerm) - // SI-9392 An errant macro might leave a reference to a local class symbol that no longer exists in the tree, - // this avoids an assertion error in that case. AFAICT, we don't actually need the `NestedInfo` for all BTypes, - // only for ones that describe classes defined in the trees that reach the backend, so this is safe enough. - // - // TODO Can we avoid creating `NestedInfo` for each type that is referred to, and instead only create if for - // symbols of ClassDefs? + else if (innerClassSym.rawowner.isTerm) { + // This case should never be reached: the lambdalift phase mutates the rawowner field of all + // classes to be the enclosing class. SI-9392 shows an errant macro that leaves a reference + // to a local class symbol that no longer exists, which is not updated by lambdalift. + devWarning(innerClassSym.pos, + s"""The class symbol $innerClassSym with the term symbol ${innerClassSym.rawowner} as `rawowner` reached the backend. + |Most likely this indicates a stale reference to a non-existing class introduced by a macro, see SI-9392.""".stripMargin) None - else { + } else { // See comment in BTypes, when is a class marked static in the InnerClass table. val isStaticNestedClass = isOriginallyStaticOwner(innerClassSym.originalOwner) -- cgit v1.2.3 From 8946d60bd27a021591818defb6b4f82ab014d4d0 Mon Sep 17 00:00:00 2001 From: Lukas Rytz Date: Thu, 16 Jul 2015 14:17:46 +0200 Subject: [backport] Fix bytecode stability when running the closure optimizer Fixes the stability regression introduced by #4619. --- .../tools/nsc/backend/jvm/opt/ClosureOptimizer.scala | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/compiler/scala/tools/nsc/backend/jvm/opt/ClosureOptimizer.scala b/src/compiler/scala/tools/nsc/backend/jvm/opt/ClosureOptimizer.scala index 58f0813bd8..92b9b34006 100644 --- a/src/compiler/scala/tools/nsc/backend/jvm/opt/ClosureOptimizer.scala +++ b/src/compiler/scala/tools/nsc/backend/jvm/opt/ClosureOptimizer.scala @@ -70,10 +70,10 @@ class ClosureOptimizer[BT <: BTypes](val btypes: BT) { } } - // Group the closure instantiations by method allows running the ProdConsAnalyzer only once per method. - // Also sort the instantiations: If there are multiple closure instantiations in a method, closure - // invocations need to be re-written in a consistent order for bytecode stability. The local variable - // slots for storing captured values depends on the order of rewriting. + // Grouping the closure instantiations by method allows running the ProdConsAnalyzer only once per + // method. Also sort the instantiations: If there are multiple closure instantiations in a method, + // closure invocations need to be re-written in a consistent order for bytecode stability. The local + // variable slots for storing captured values depends on the order of rewriting. val closureInstantiationsByMethod: Map[MethodNode, immutable.TreeSet[ClosureInstantiation]] = { closureInstantiations.values.groupBy(_.ownerMethod).mapValues(immutable.TreeSet.empty ++ _) } @@ -81,13 +81,13 @@ class ClosureOptimizer[BT <: BTypes](val btypes: BT) { // For each closure instantiation, a list of callsites of the closure that can be re-written // If a callsite cannot be rewritten, for example because the lambda body method is not accessible, // a warning is returned instead. - val callsitesToRewrite: Map[ClosureInstantiation, List[Either[RewriteClosureApplyToClosureBodyFailed, (MethodInsnNode, Int)]]] = { - closureInstantiationsByMethod flatMap { + val callsitesToRewrite: List[(ClosureInstantiation, List[Either[RewriteClosureApplyToClosureBodyFailed, (MethodInsnNode, Int)]])] = { + closureInstantiationsByMethod.iterator.flatMap({ case (methodNode, closureInits) => // A lazy val to ensure the analysis only runs if necessary (the value is passed by name to `closureCallsites`) lazy val prodCons = new ProdConsAnalyzer(methodNode, closureInits.head.ownerClass.internalName) - closureInits.map(init => (init, closureCallsites(init, prodCons))) - } + closureInits.iterator.map(init => (init, closureCallsites(init, prodCons))) + }).toList // mapping to a list (not a map) to keep the sorting of closureInstantiationsByMethod } // Rewrite all closure callsites (or issue inliner warnings for those that cannot be rewritten) -- cgit v1.2.3 From 7a7f9927c3fd9919133d12619ce83ac2481848ce Mon Sep 17 00:00:00 2001 From: Lukas Rytz Date: Wed, 15 Jul 2015 19:59:57 +0200 Subject: SI-9393 fix modifiers of ClassBTypes for Java annotations The Scala classfile and java source parsers make Java annotation classes (which are actually interfaces at the classfile level) look like Scala annotation classes: - the INTERFACE / ABSTRACT flags are not added - scala.annotation.Annotation is added as superclass - scala.annotation.ClassfileAnnotation is added as interface This makes type-checking @Annot uniform, whether it is defined in Java or Scala. This is a hack that leads to various bugs (SI-9393, SI-9400). Instead the type-checking should be special-cased for Java annotations. This commit fixes SI-9393 and a part of SI-9400, but it's still easy to generate invalid classfiles. Restores the assertions that were disabled in #4621. I'd like to leave these assertions in: they are valuable and helped uncovering the issue being fixed here. A new flag JAVA_ANNOTATION is introduced for Java annotation ClassSymbols, similar to the existing ENUM flag. When building ClassBTypes for Java annotations, the flags, superclass and interfaces are recovered to represent the situation in the classfile. Cleans up and documents the flags space in the area of "late" and "anti" flags. The test for SI-9393 is extended to test both the classfile and the java source parser. --- .../tools/nsc/backend/jvm/BCodeAsmCommon.scala | 20 ++++- .../tools/nsc/backend/jvm/BCodeSkelBuilder.scala | 6 +- .../scala/tools/nsc/backend/jvm/BTypes.scala | 21 +++-- .../tools/nsc/backend/jvm/BTypesFromSymbols.scala | 13 +++- .../scala/tools/nsc/javac/JavaParsers.scala | 2 +- .../reflect/internal/ClassfileConstants.scala | 2 + src/reflect/scala/reflect/internal/Flags.scala | 33 ++++++-- src/reflect/scala/reflect/internal/HasFlags.scala | 1 + src/reflect/scala/reflect/internal/Symbols.scala | 2 +- test/files/jvm/innerClassAttribute/Test.scala | 4 +- test/files/pos/t9393/Named.java | 3 - test/files/pos/t9393/NamedImpl.java | 15 ---- test/files/pos/t9393/NamedImpl_1.java | 15 ++++ test/files/pos/t9393/NamedImpl_2.java | 15 ++++ test/files/pos/t9393/Named_1.java | 3 + test/files/pos/t9393/Named_2.java | 3 + test/files/pos/t9393/test.scala | 3 - test/files/pos/t9393/test_2.scala | 4 + test/junit/scala/tools/nsc/symtab/FlagsTest.scala | 89 ++++++++++++++++++++++ 19 files changed, 201 insertions(+), 53 deletions(-) delete mode 100644 test/files/pos/t9393/Named.java delete mode 100644 test/files/pos/t9393/NamedImpl.java create mode 100644 test/files/pos/t9393/NamedImpl_1.java create mode 100644 test/files/pos/t9393/NamedImpl_2.java create mode 100644 test/files/pos/t9393/Named_1.java create mode 100644 test/files/pos/t9393/Named_2.java delete mode 100644 test/files/pos/t9393/test.scala create mode 100644 test/files/pos/t9393/test_2.scala create mode 100644 test/junit/scala/tools/nsc/symtab/FlagsTest.scala diff --git a/src/compiler/scala/tools/nsc/backend/jvm/BCodeAsmCommon.scala b/src/compiler/scala/tools/nsc/backend/jvm/BCodeAsmCommon.scala index daa31c5dfe..23cb170b38 100644 --- a/src/compiler/scala/tools/nsc/backend/jvm/BCodeAsmCommon.scala +++ b/src/compiler/scala/tools/nsc/backend/jvm/BCodeAsmCommon.scala @@ -256,6 +256,9 @@ final class BCodeAsmCommon[G <: Global](val global: G) { if (hasAbstractMethod) ACC_ABSTRACT else 0 } GenBCode.mkFlags( + // SI-9393: the classfile / java source parser make java annotation symbols look like classes. + // here we recover the actual classfile flags. + if (classSym.hasJavaAnnotationFlag) ACC_ANNOTATION | ACC_INTERFACE | ACC_ABSTRACT else 0, if (classSym.isPublic) ACC_PUBLIC else 0, if (classSym.isFinal) ACC_FINAL else 0, // see the link above. javac does the same: ACC_SUPER for all classes, but not interfaces. @@ -310,10 +313,10 @@ final class BCodeAsmCommon[G <: Global](val global: G) { } private def retentionPolicyOf(annot: AnnotationInfo): Symbol = - annot.atp.typeSymbol.getAnnotation(AnnotationRetentionAttr).map(_.assocs).map(assoc => + annot.atp.typeSymbol.getAnnotation(AnnotationRetentionAttr).map(_.assocs).flatMap(assoc => assoc.collectFirst { case (`nme`.value, LiteralAnnotArg(Constant(value: Symbol))) => value - }).flatten.getOrElse(AnnotationRetentionPolicyClassValue) + }).getOrElse(AnnotationRetentionPolicyClassValue) def implementedInterfaces(classSym: Symbol): List[Symbol] = { // Additional interface parents based on annotations and other cues @@ -322,9 +325,18 @@ final class BCodeAsmCommon[G <: Global](val global: G) { case _ => None } - def isInterfaceOrTrait(sym: Symbol) = sym.isInterface || sym.isTrait + // SI-9393: java annotations are interfaces, but the classfile / java source parsers make them look like classes. + def isInterfaceOrTrait(sym: Symbol) = sym.isInterface || sym.isTrait || sym.hasJavaAnnotationFlag - val allParents = classSym.info.parents ++ classSym.annotations.flatMap(newParentForAnnotation) + val classParents = { + val parents = classSym.info.parents + // SI-9393: the classfile / java source parsers add Annotation and ClassfileAnnotation to the + // parents of a java annotations. undo this for the backend (where we need classfile-level information). + if (classSym.hasJavaAnnotationFlag) parents.filterNot(c => c.typeSymbol == ClassfileAnnotationClass || c.typeSymbol == AnnotationClass) + else parents + } + + val allParents = classParents ++ classSym.annotations.flatMap(newParentForAnnotation) // We keep the superClass when computing minimizeParents to eliminate more interfaces. // Example: T can be eliminated from D diff --git a/src/compiler/scala/tools/nsc/backend/jvm/BCodeSkelBuilder.scala b/src/compiler/scala/tools/nsc/backend/jvm/BCodeSkelBuilder.scala index c7c2e43f82..a9b6a312e9 100644 --- a/src/compiler/scala/tools/nsc/backend/jvm/BCodeSkelBuilder.scala +++ b/src/compiler/scala/tools/nsc/backend/jvm/BCodeSkelBuilder.scala @@ -153,9 +153,9 @@ abstract class BCodeSkelBuilder extends BCodeHelpers { */ private def initJClass(jclass: asm.ClassVisitor) { - val ps = claszSymbol.info.parents - val superClass: String = if (ps.isEmpty) ObjectReference.internalName else internalName(ps.head.typeSymbol) - val interfaceNames = classBTypeFromSymbol(claszSymbol).info.get.interfaces map { + val bType = classBTypeFromSymbol(claszSymbol) + val superClass = bType.info.get.superClass.getOrElse(ObjectReference).internalName + val interfaceNames = bType.info.get.interfaces map { case classBType => if (classBType.isNestedClass.get) { innerClassBufferASM += classBType } classBType.internalName diff --git a/src/compiler/scala/tools/nsc/backend/jvm/BTypes.scala b/src/compiler/scala/tools/nsc/backend/jvm/BTypes.scala index 9bae63f1fc..8720da84e8 100644 --- a/src/compiler/scala/tools/nsc/backend/jvm/BTypes.scala +++ b/src/compiler/scala/tools/nsc/backend/jvm/BTypes.scala @@ -841,17 +841,16 @@ abstract class BTypes { assert(!ClassBType.isInternalPhantomType(internalName), s"Cannot create ClassBType for phantom type $this") - // TODO bring these back in a way that doesn't trip pos/t9393 - // assert( - // if (info.get.superClass.isEmpty) { isJLO(this) || (isCompilingPrimitive && ClassBType.hasNoSuper(internalName)) } - // else if (isInterface.get) isJLO(info.get.superClass.get) - // else !isJLO(this) && ifInit(info.get.superClass.get)(!_.isInterface.get), - // s"Invalid superClass in $this: ${info.get.superClass}" - // ) - // assert( - // info.get.interfaces.forall(c => ifInit(c)(_.isInterface.get)), - // s"Invalid interfaces in $this: ${info.get.interfaces}" - // ) + assert( + if (info.get.superClass.isEmpty) { isJLO(this) || (isCompilingPrimitive && ClassBType.hasNoSuper(internalName)) } + else if (isInterface.get) isJLO(info.get.superClass.get) + else !isJLO(this) && ifInit(info.get.superClass.get)(!_.isInterface.get), + s"Invalid superClass in $this: ${info.get.superClass}" + ) + assert( + info.get.interfaces.forall(c => ifInit(c)(_.isInterface.get)), + s"Invalid interfaces in $this: ${info.get.interfaces}" + ) assert(info.get.nestedClasses.forall(c => ifInit(c)(_.isNestedClass.get)), info.get.nestedClasses) } diff --git a/src/compiler/scala/tools/nsc/backend/jvm/BTypesFromSymbols.scala b/src/compiler/scala/tools/nsc/backend/jvm/BTypesFromSymbols.scala index 9b4451d492..8ded58d3d9 100644 --- a/src/compiler/scala/tools/nsc/backend/jvm/BTypesFromSymbols.scala +++ b/src/compiler/scala/tools/nsc/backend/jvm/BTypesFromSymbols.scala @@ -216,7 +216,18 @@ class BTypesFromSymbols[G <: Global](val global: G) extends BTypes { } private def setClassInfo(classSym: Symbol, classBType: ClassBType): ClassBType = { - val superClassSym = if (classSym.isImplClass) ObjectClass else classSym.superClass + // Check for isImplClass: trait implementation classes have NoSymbol as superClass + // Check for hasAnnotationFlag for SI-9393: the classfile / java source parsers add + // scala.annotation.Annotation as superclass to java annotations. In reality, java + // annotation classfiles have superclass Object (like any interface classfile). + val superClassSym = if (classSym.isImplClass || classSym.hasJavaAnnotationFlag) ObjectClass else { + val sc = classSym.superClass + // SI-9393: Java annotation classes don't have the ABSTRACT/INTERFACE flag, so they appear + // (wrongly) as superclasses. Fix this for BTypes: the java annotation will appear as interface + // (handled by method implementedInterfaces), the superclass is set to Object. + if (sc.hasJavaAnnotationFlag) ObjectClass + else sc + } assert( if (classSym == ObjectClass) superClassSym == NoSymbol diff --git a/src/compiler/scala/tools/nsc/javac/JavaParsers.scala b/src/compiler/scala/tools/nsc/javac/JavaParsers.scala index 03f0236734..67921303b9 100644 --- a/src/compiler/scala/tools/nsc/javac/JavaParsers.scala +++ b/src/compiler/scala/tools/nsc/javac/JavaParsers.scala @@ -751,7 +751,7 @@ trait JavaParsers extends ast.parser.ParsersCommon with JavaScanners { val (statics, body) = typeBody(AT, name) val templ = makeTemplate(annotationParents, body) addCompanionObject(statics, atPos(pos) { - ClassDef(mods, name, List(), templ) + ClassDef(mods | Flags.JAVA_ANNOTATION, name, List(), templ) }) } diff --git a/src/reflect/scala/reflect/internal/ClassfileConstants.scala b/src/reflect/scala/reflect/internal/ClassfileConstants.scala index 53241fb15b..d04bd3d3c8 100644 --- a/src/reflect/scala/reflect/internal/ClassfileConstants.scala +++ b/src/reflect/scala/reflect/internal/ClassfileConstants.scala @@ -345,6 +345,7 @@ object ClassfileConstants { case JAVA_ACC_ABSTRACT => if (isAnnotation) 0L else if (isClass) ABSTRACT else DEFERRED case JAVA_ACC_INTERFACE => if (isAnnotation) 0L else TRAIT | INTERFACE | ABSTRACT case JAVA_ACC_ENUM => ENUM + case JAVA_ACC_ANNOTATION => JAVA_ANNOTATION case _ => 0L } private def translateFlags(jflags: Int, baseFlags: Long, isClass: Boolean): Long = { @@ -360,6 +361,7 @@ object ClassfileConstants { res |= translateFlag0(jflags & JAVA_ACC_ABSTRACT) res |= translateFlag0(jflags & JAVA_ACC_INTERFACE) res |= translateFlag0(jflags & JAVA_ACC_ENUM) + res |= translateFlag0(jflags & JAVA_ACC_ANNOTATION) res } diff --git a/src/reflect/scala/reflect/internal/Flags.scala b/src/reflect/scala/reflect/internal/Flags.scala index 1707061817..ab56020713 100644 --- a/src/reflect/scala/reflect/internal/Flags.scala +++ b/src/reflect/scala/reflect/internal/Flags.scala @@ -64,7 +64,7 @@ import scala.collection.{ mutable, immutable } // 46: ARTIFACT // 47: DEFAULTMETHOD/M // 48: ENUM -// 49: +// 49: JAVA_ANNOTATION // 50: // 51: lateDEFERRED // 52: lateFINAL @@ -120,7 +120,8 @@ class ModifierFlags { final val ARTIFACT = 1L << 46 // symbol should be ignored when typechecking; will be marked ACC_SYNTHETIC in bytecode // to see which symbols are marked as ARTIFACT, see scaladocs for FlagValues.ARTIFACT final val DEFAULTMETHOD = 1L << 47 // symbol is a java default method - final val ENUM = 1L << 48 // symbol is an enum + final val ENUM = 1L << 48 // symbol is a java enum + final val JAVA_ANNOTATION = 1L << 49 // symbol is a java annotation // Overridden. def flagToString(flag: Long): String = "" @@ -172,12 +173,28 @@ class Flags extends ModifierFlags { final val SYNCHRONIZED = 1L << 45 // symbol is a method which should be marked ACC_SYNCHRONIZED // ------- shift definitions ------------------------------------------------------- + // + // Flags from 1L to (1L << 50) are normal flags. + // + // The flags DEFERRED (1L << 4) to MODULE (1L << 8) have a `late` counterpart. Late flags change + // their counterpart from 0 to 1 after a specific phase (see below). The first late flag + // (lateDEFERRED) is at (1L << 51), i.e., late flags are shifted by 47. The last one is (1L << 55). + // + // The flags PROTECTED (1L) to PRIVATE (1L << 2) have a `not` counterpart. Negated flags change + // their counterpart from 1 to 0 after a specific phase (see below). They are shifted by 56, i.e., + // the first negated flag (notPROTECTED) is at (1L << 56), the last at (1L << 58). + // + // Late and negative flags are only enabled after certain phases, implemented by the phaseNewFlags + // method of the SubComponent, so they implement a bit of a flag history. + // + // The flags (1L << 59) to (1L << 63) are currently unused. If added to the InitialFlags mask, + // they could be used as normal flags. - final val InitialFlags = 0x0001FFFFFFFFFFFFL // flags that are enabled from phase 1. - final val LateFlags = 0x00FE000000000000L // flags that override flags in 0x1FC. - final val AntiFlags = 0x7F00000000000000L // flags that cancel flags in 0x07F - final val LateShift = 47L - final val AntiShift = 56L + final val InitialFlags = 0x0007FFFFFFFFFFFFL // normal flags, enabled from the first phase: 1L to (1L << 50) + final val LateFlags = 0x00F8000000000000L // flags that override flags in (1L << 4) to (1L << 8): DEFERRED, FINAL, INTERFACE, METHOD, MODULE + final val AntiFlags = 0x0700000000000000L // flags that cancel flags in 1L to (1L << 2): PROTECTED, OVERRIDE, PRIVATE + final val LateShift = 47 + final val AntiShift = 56 // Flags which sketchily share the same slot // 16: BYNAMEPARAM/M CAPTURED COVARIANT/M @@ -436,7 +453,7 @@ class Flags extends ModifierFlags { case ARTIFACT => "" // (1L << 46) case DEFAULTMETHOD => "" // (1L << 47) case ENUM => "" // (1L << 48) - case 0x2000000000000L => "" // (1L << 49) + case JAVA_ANNOTATION => "" // (1L << 49) case 0x4000000000000L => "" // (1L << 50) case `lateDEFERRED` => "" // (1L << 51) case `lateFINAL` => "" // (1L << 52) diff --git a/src/reflect/scala/reflect/internal/HasFlags.scala b/src/reflect/scala/reflect/internal/HasFlags.scala index aa8f4c532e..17f28ab1ca 100644 --- a/src/reflect/scala/reflect/internal/HasFlags.scala +++ b/src/reflect/scala/reflect/internal/HasFlags.scala @@ -83,6 +83,7 @@ trait HasFlags { def hasAccessorFlag = hasFlag(ACCESSOR) def hasDefault = hasFlag(DEFAULTPARAM) && hasFlag(METHOD | PARAM) // Second condition disambiguates with TRAIT def hasEnumFlag = hasFlag(ENUM) + def hasJavaAnnotationFlag = hasFlag(JAVA_ANNOTATION) @deprecated("Use isLocalToThis instead", "2.11.0") def hasLocalFlag = hasFlag(LOCAL) def isLocalToThis = hasFlag(LOCAL) diff --git a/src/reflect/scala/reflect/internal/Symbols.scala b/src/reflect/scala/reflect/internal/Symbols.scala index 2329b30638..56fb1181fe 100644 --- a/src/reflect/scala/reflect/internal/Symbols.scala +++ b/src/reflect/scala/reflect/internal/Symbols.scala @@ -732,7 +732,7 @@ trait Symbols extends api.Symbols { self: SymbolTable => final def flags: Long = { if (Statistics.hotEnabled) Statistics.incCounter(flagsCount) val fs = _rawflags & phase.flagMask - (fs | ((fs & LateFlags) >>> LateShift)) & ~(fs >>> AntiShift) + (fs | ((fs & LateFlags) >>> LateShift)) & ~((fs & AntiFlags) >>> AntiShift) } def flags_=(fs: Long) = _rawflags = fs def rawflags_=(x: Long) { _rawflags = x } diff --git a/test/files/jvm/innerClassAttribute/Test.scala b/test/files/jvm/innerClassAttribute/Test.scala index 376b3c895b..3a6737ca46 100644 --- a/test/files/jvm/innerClassAttribute/Test.scala +++ b/test/files/jvm/innerClassAttribute/Test.scala @@ -149,9 +149,7 @@ object Test extends BytecodeTest { def testA11() = { val List(ann) = innerClassNodes("A11") - // in the java class file, the INNERCLASS attribute has more flags (public | static | abstract | interface | annotation) - // the scala compiler has its own interpretation of java annotations ant their flags.. it only emits publicStatic. - assertMember(ann, "JavaAnnot_1", "Ann", flags = publicStatic) + assertMember(ann, "JavaAnnot_1", "Ann", flags = publicAbstractInterface | Flags.ACC_STATIC | Flags.ACC_ANNOTATION) } def testA13() = { diff --git a/test/files/pos/t9393/Named.java b/test/files/pos/t9393/Named.java deleted file mode 100644 index 144ddbf26e..0000000000 --- a/test/files/pos/t9393/Named.java +++ /dev/null @@ -1,3 +0,0 @@ -package bug; - -public @interface Named {} diff --git a/test/files/pos/t9393/NamedImpl.java b/test/files/pos/t9393/NamedImpl.java deleted file mode 100644 index 7918739c2b..0000000000 --- a/test/files/pos/t9393/NamedImpl.java +++ /dev/null @@ -1,15 +0,0 @@ -/* - * Copyright (C) 2009-2015 Typesafe Inc. - */ -package bug; - -import bug.Named; -import java.io.Serializable; -import java.lang.annotation.Annotation; - -public class NamedImpl implements Named { - - public Class annotationType() { - return null; - } -} diff --git a/test/files/pos/t9393/NamedImpl_1.java b/test/files/pos/t9393/NamedImpl_1.java new file mode 100644 index 0000000000..02ec9b4671 --- /dev/null +++ b/test/files/pos/t9393/NamedImpl_1.java @@ -0,0 +1,15 @@ +/* + * Copyright (C) 2009-2015 Typesafe Inc. + */ +package bug; + +import bug.Named_1; +import java.io.Serializable; +import java.lang.annotation.Annotation; + +public class NamedImpl_1 implements Named_1 { + + public Class annotationType() { + return null; + } +} diff --git a/test/files/pos/t9393/NamedImpl_2.java b/test/files/pos/t9393/NamedImpl_2.java new file mode 100644 index 0000000000..c87e94016d --- /dev/null +++ b/test/files/pos/t9393/NamedImpl_2.java @@ -0,0 +1,15 @@ +/* + * Copyright (C) 2009-2015 Typesafe Inc. + */ +package bug; + +import bug.Named_2; +import java.io.Serializable; +import java.lang.annotation.Annotation; + +public class NamedImpl_2 implements Named_2 { + + public Class annotationType() { + return null; + } +} diff --git a/test/files/pos/t9393/Named_1.java b/test/files/pos/t9393/Named_1.java new file mode 100644 index 0000000000..30a6c9839a --- /dev/null +++ b/test/files/pos/t9393/Named_1.java @@ -0,0 +1,3 @@ +package bug; + +public @interface Named_1 {} diff --git a/test/files/pos/t9393/Named_2.java b/test/files/pos/t9393/Named_2.java new file mode 100644 index 0000000000..3210fb636a --- /dev/null +++ b/test/files/pos/t9393/Named_2.java @@ -0,0 +1,3 @@ +package bug; + +public @interface Named_2 {} diff --git a/test/files/pos/t9393/test.scala b/test/files/pos/t9393/test.scala deleted file mode 100644 index 4df0476c98..0000000000 --- a/test/files/pos/t9393/test.scala +++ /dev/null @@ -1,3 +0,0 @@ -class C { - new bug.NamedImpl -} diff --git a/test/files/pos/t9393/test_2.scala b/test/files/pos/t9393/test_2.scala new file mode 100644 index 0000000000..8ea346129d --- /dev/null +++ b/test/files/pos/t9393/test_2.scala @@ -0,0 +1,4 @@ +class C { + new bug.NamedImpl_1 // separate compilation, testing the classfile parser + new bug.NamedImpl_2 // mixed compilation, testing the java source parser +} diff --git a/test/junit/scala/tools/nsc/symtab/FlagsTest.scala b/test/junit/scala/tools/nsc/symtab/FlagsTest.scala new file mode 100644 index 0000000000..fc0e8b0f6b --- /dev/null +++ b/test/junit/scala/tools/nsc/symtab/FlagsTest.scala @@ -0,0 +1,89 @@ +package scala.tools.nsc +package symtab + +import org.junit.Assert._ +import scala.tools.testing.AssertUtil._ +import org.junit.Test +import org.junit.runner.RunWith +import org.junit.runners.JUnit4 + +@RunWith(classOf[JUnit4]) +class FlagsTest { + object symbolTable extends SymbolTableForUnitTesting + import symbolTable._ + import Flags._ + + def sym = NoSymbol.newTermSymbol(nme.EMPTY) + + def withFlagMask[A](mask: Long)(body: => A): A = enteringPhase(new Phase(NoPhase) { + override def flagMask = mask + def name = "" + def run() = () + })(body) + + def testTimedFlag(flag: Long, test: Symbol => Boolean, enabling: Boolean) = { + assertEquals(withFlagMask(InitialFlags)(test(sym.setFlag(flag))), !enabling) + assertEquals(withFlagMask(InitialFlags | flag)(test(sym.setFlag(flag))), enabling) + } + + def testLate(flag: Long, test: Symbol => Boolean) = testTimedFlag(flag, test, enabling = true) + def testNot(flag: Long, test: Symbol => Boolean) = testTimedFlag(flag, test, enabling = false) + + @Test + def testTimedFlags(): Unit = { + testLate(lateDEFERRED, _.isDeferred) + testLate(lateFINAL, _.isFinal) + testLate(lateINTERFACE, _.isInterface) + testLate(lateMETHOD, _.isMethod) + testLate(lateMODULE, _.isModule) + testNot(PROTECTED | notPROTECTED, _.isProtected) + testNot(OVERRIDE | notOVERRIDE, _.isOverride) + testNot(PRIVATE | notPRIVATE, _.isPrivate) + + assertFalse(withFlagMask(AllFlags)(sym.setFlag(PRIVATE | notPRIVATE).isPrivate)) + + assertEquals(withFlagMask(InitialFlags)(sym.setFlag(PRIVATE | notPRIVATE).flags & PRIVATE), PRIVATE) + assertEquals(withFlagMask(AllFlags)(sym.setFlag(PRIVATE | notPRIVATE).flags & PRIVATE), 0) + } + + @Test + def normalLateOverlap(): Unit = { + // late flags are shifted by LateShift == 47. + // however, the first late flag is lateDEFERRED, which is DEFERRED << 47 == (1 << 4) << 47 == 1 << 51 + // the flags from 1 << 47 to 1 << 50 are not late flags. this is ensured by the LateFlags mask. + + for (i <- 0 to 3) { + val f = 1L << i + assertEquals(withFlagMask(AllFlags)(sym.setFlag(f << LateShift).flags & f), 0) // not treated as late flag + } + for (i <- 4 to 8) { + val f = 1L << i + assertEquals(withFlagMask(AllFlags)(sym.setFlag(f << LateShift).flags & f), f) // treated as late flag + } + } + + @Test + def normalAnti(): Unit = { + for (i <- 0 to 2) { + val f = 1L << i + assertEquals(withFlagMask(AllFlags)(sym.setFlag(f | (f << AntiShift)).flags & f), 0) // negated flags + } + for (i <- 3 to 7) { + val f = 1L << i + assertEquals(withFlagMask(AllFlags)(sym.setFlag(f | (f << AntiShift)).flags & f), f) // not negated + } + } + + @Test + def lateAntiCrossCheck(): Unit = { + val allButNegatable = AllFlags & ~(PROTECTED | OVERRIDE | PRIVATE) + val lateable = 0L | DEFERRED | FINAL | INTERFACE | METHOD | MODULE + val lateFlags = lateable << LateShift + val allButLateable = AllFlags & ~lateable + + assertEquals(withFlagMask(AllFlags)(sym.setFlag(AllFlags).flags), allButNegatable) + assertEquals(withFlagMask(AllFlags)(sym.setFlag(allButLateable).flags), allButNegatable) + + assertEquals(withFlagMask(AllFlags)(sym.setFlag(lateFlags).flags), lateFlags | lateable) + } +} -- cgit v1.2.3 From 241bb9ac192f13868436d9a4e2e7ae29a2e22bed Mon Sep 17 00:00:00 2001 From: Lukas Rytz Date: Tue, 21 Jul 2015 22:13:25 +0200 Subject: Rename the ENUM / DEFAULTMETHOD flags to include JAVA_ Similar to the new JAVA_ANNOTATION flag, be more explicit about flags for java entities. --- .../tools/nsc/backend/jvm/BCodeAsmCommon.scala | 14 +-- .../tools/nsc/backend/jvm/BTypesFromSymbols.scala | 2 +- .../scala/tools/nsc/backend/jvm/GenASM.scala | 2 +- .../scala/tools/nsc/javac/JavaParsers.scala | 8 +- .../nsc/symtab/classfile/ClassfileParser.scala | 2 +- .../scala/tools/nsc/typechecker/Namers.scala | 4 +- .../scala/tools/nsc/typechecker/RefChecks.scala | 4 +- .../reflect/internal/ClassfileConstants.scala | 2 +- .../scala/reflect/internal/Definitions.scala | 2 +- src/reflect/scala/reflect/internal/FlagSets.scala | 2 +- src/reflect/scala/reflect/internal/Flags.scala | 126 ++++++++++----------- src/reflect/scala/reflect/internal/HasFlags.scala | 82 +++++++------- 12 files changed, 125 insertions(+), 125 deletions(-) diff --git a/src/compiler/scala/tools/nsc/backend/jvm/BCodeAsmCommon.scala b/src/compiler/scala/tools/nsc/backend/jvm/BCodeAsmCommon.scala index 23cb170b38..93f5159f89 100644 --- a/src/compiler/scala/tools/nsc/backend/jvm/BCodeAsmCommon.scala +++ b/src/compiler/scala/tools/nsc/backend/jvm/BCodeAsmCommon.scala @@ -258,15 +258,15 @@ final class BCodeAsmCommon[G <: Global](val global: G) { GenBCode.mkFlags( // SI-9393: the classfile / java source parser make java annotation symbols look like classes. // here we recover the actual classfile flags. - if (classSym.hasJavaAnnotationFlag) ACC_ANNOTATION | ACC_INTERFACE | ACC_ABSTRACT else 0, - if (classSym.isPublic) ACC_PUBLIC else 0, - if (classSym.isFinal) ACC_FINAL else 0, + if (classSym.hasJavaAnnotationFlag) ACC_ANNOTATION | ACC_INTERFACE | ACC_ABSTRACT else 0, + if (classSym.isPublic) ACC_PUBLIC else 0, + if (classSym.isFinal) ACC_FINAL else 0, // see the link above. javac does the same: ACC_SUPER for all classes, but not interfaces. - if (classSym.isInterface) ACC_INTERFACE else ACC_SUPER, + if (classSym.isInterface) ACC_INTERFACE else ACC_SUPER, // for Java enums, we cannot trust `hasAbstractFlag` (see comment in enumFlags) - if (!classSym.hasEnumFlag && classSym.hasAbstractFlag) ACC_ABSTRACT else 0, - if (classSym.isArtifact) ACC_SYNTHETIC else 0, - if (classSym.hasEnumFlag) enumFlags else 0 + if (!classSym.hasJavaEnumFlag && classSym.hasAbstractFlag) ACC_ABSTRACT else 0, + if (classSym.isArtifact) ACC_SYNTHETIC else 0, + if (classSym.hasJavaEnumFlag) enumFlags else 0 ) } diff --git a/src/compiler/scala/tools/nsc/backend/jvm/BTypesFromSymbols.scala b/src/compiler/scala/tools/nsc/backend/jvm/BTypesFromSymbols.scala index 8ded58d3d9..45d9cc3ff3 100644 --- a/src/compiler/scala/tools/nsc/backend/jvm/BTypesFromSymbols.scala +++ b/src/compiler/scala/tools/nsc/backend/jvm/BTypesFromSymbols.scala @@ -578,7 +578,7 @@ class BTypesFromSymbols[G <: Global](val global: G) extends BTypes { if (sym.isBridge) ACC_BRIDGE | ACC_SYNTHETIC else 0, if (sym.isArtifact) ACC_SYNTHETIC else 0, if (sym.isClass && !sym.isInterface) ACC_SUPER else 0, - if (sym.hasEnumFlag) ACC_ENUM else 0, + if (sym.hasJavaEnumFlag) ACC_ENUM else 0, if (sym.isVarargsMethod) ACC_VARARGS else 0, if (sym.hasFlag(symtab.Flags.SYNCHRONIZED)) ACC_SYNCHRONIZED else 0, if (sym.isDeprecated) asm.Opcodes.ACC_DEPRECATED else 0 diff --git a/src/compiler/scala/tools/nsc/backend/jvm/GenASM.scala b/src/compiler/scala/tools/nsc/backend/jvm/GenASM.scala index 2f1cd732da..a34ab914ef 100644 --- a/src/compiler/scala/tools/nsc/backend/jvm/GenASM.scala +++ b/src/compiler/scala/tools/nsc/backend/jvm/GenASM.scala @@ -307,7 +307,7 @@ abstract class GenASM extends SubComponent with BytecodeWriters { self => if (sym.isBridge) ACC_BRIDGE | ACC_SYNTHETIC else 0, if (sym.isArtifact) ACC_SYNTHETIC else 0, if (sym.isClass && !sym.isInterface) ACC_SUPER else 0, - if (sym.hasEnumFlag) ACC_ENUM else 0, + if (sym.hasJavaEnumFlag) ACC_ENUM else 0, if (sym.isVarargsMethod) ACC_VARARGS else 0, if (sym.hasFlag(Flags.SYNCHRONIZED)) ACC_SYNCHRONIZED else 0 ) diff --git a/src/compiler/scala/tools/nsc/javac/JavaParsers.scala b/src/compiler/scala/tools/nsc/javac/JavaParsers.scala index 67921303b9..eb25eb6e06 100644 --- a/src/compiler/scala/tools/nsc/javac/JavaParsers.scala +++ b/src/compiler/scala/tools/nsc/javac/JavaParsers.scala @@ -370,7 +370,7 @@ trait JavaParsers extends ast.parser.ParsersCommon with JavaScanners { flags |= Flags.FINAL in.nextToken() case DEFAULT => - flags |= Flags.DEFAULTMETHOD + flags |= Flags.JAVA_DEFAULTMETHOD in.nextToken() case NATIVE => addAnnot(NativeAttr) @@ -489,7 +489,7 @@ trait JavaParsers extends ast.parser.ParsersCommon with JavaScanners { val vparams = formalParams() if (!isVoid) rtpt = optArrayBrackets(rtpt) optThrows() - val isConcreteInterfaceMethod = !inInterface || (mods hasFlag Flags.DEFAULTMETHOD) || (mods hasFlag Flags.STATIC) + val isConcreteInterfaceMethod = !inInterface || (mods hasFlag Flags.JAVA_DEFAULTMETHOD) || (mods hasFlag Flags.STATIC) val bodyOk = !(mods1 hasFlag Flags.DEFERRED) && isConcreteInterfaceMethod val body = if (bodyOk && in.token == LBRACE) { @@ -809,7 +809,7 @@ trait JavaParsers extends ast.parser.ParsersCommon with JavaScanners { if (hasAbstractMember) Flags.ABSTRACT else 0l } addCompanionObject(consts ::: statics ::: predefs, atPos(pos) { - ClassDef(mods | Flags.ENUM | finalFlag | abstractFlag, name, List(), + ClassDef(mods | Flags.JAVA_ENUM | finalFlag | abstractFlag, name, List(), makeTemplate(superclazz :: interfaces, body)) }) } @@ -830,7 +830,7 @@ trait JavaParsers extends ast.parser.ParsersCommon with JavaScanners { skipAhead() accept(RBRACE) } - ValDef(Modifiers(Flags.ENUM | Flags.STABLE | Flags.JAVA | Flags.STATIC), name.toTermName, enumType, blankExpr) + ValDef(Modifiers(Flags.JAVA_ENUM | Flags.STABLE | Flags.JAVA | Flags.STATIC), name.toTermName, enumType, blankExpr) } (res, hasClassBody) } diff --git a/src/compiler/scala/tools/nsc/symtab/classfile/ClassfileParser.scala b/src/compiler/scala/tools/nsc/symtab/classfile/ClassfileParser.scala index 91355693ee..06a0299d2a 100644 --- a/src/compiler/scala/tools/nsc/symtab/classfile/ClassfileParser.scala +++ b/src/compiler/scala/tools/nsc/symtab/classfile/ClassfileParser.scala @@ -862,7 +862,7 @@ abstract class ClassfileParser { srcfile0 = settings.outputDirs.srcFilesFor(in.file, srcpath).find(_.exists) case tpnme.CodeATTR => if (sym.owner.isInterface) { - sym setFlag DEFAULTMETHOD + sym setFlag JAVA_DEFAULTMETHOD log(s"$sym in ${sym.owner} is a java8+ default method.") } in.skip(attrLen) diff --git a/src/compiler/scala/tools/nsc/typechecker/Namers.scala b/src/compiler/scala/tools/nsc/typechecker/Namers.scala index c1655467e9..4ad81b60ae 100644 --- a/src/compiler/scala/tools/nsc/typechecker/Namers.scala +++ b/src/compiler/scala/tools/nsc/typechecker/Namers.scala @@ -145,8 +145,8 @@ trait Namers extends MethodSynthesis { // while Scala's enum constants live directly in the class. // We don't check for clazz.superClass == JavaEnumClass, because this causes a illegal // cyclic reference error. See the commit message for details. - if (context.unit.isJava) owner.companionClass.hasEnumFlag else owner.hasEnumFlag - vd.mods.hasAllFlags(ENUM | STABLE | STATIC) && ownerHasEnumFlag + if (context.unit.isJava) owner.companionClass.hasJavaEnumFlag else owner.hasJavaEnumFlag + vd.mods.hasAllFlags(JAVA_ENUM | STABLE | STATIC) && ownerHasEnumFlag } def setPrivateWithin[T <: Symbol](tree: Tree, sym: T, mods: Modifiers): T = diff --git a/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala b/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala index 36423fa2aa..0198529ef7 100644 --- a/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala +++ b/src/compiler/scala/tools/nsc/typechecker/RefChecks.scala @@ -421,7 +421,7 @@ abstract class RefChecks extends InfoTransform with scala.reflect.internal.trans overrideError("cannot be used here - classes can only override abstract types") } else if (other.isEffectivelyFinal) { // (1.2) overrideError("cannot override final member") - } else if (!other.isDeferredOrDefault && !other.hasFlag(DEFAULTMETHOD) && !member.isAnyOverride && !member.isSynthetic) { // (*) + } else if (!other.isDeferredOrJavaDefault && !other.hasFlag(JAVA_DEFAULTMETHOD) && !member.isAnyOverride && !member.isSynthetic) { // (*) // (*) Synthetic exclusion for (at least) default getters, fixes SI-5178. We cannot assign the OVERRIDE flag to // the default getter: one default getter might sometimes override, sometimes not. Example in comment on ticket. if (isNeitherInClass && !(other.owner isSubClass member.owner)) @@ -604,7 +604,7 @@ abstract class RefChecks extends InfoTransform with scala.reflect.internal.trans def checkNoAbstractMembers(): Unit = { // Avoid spurious duplicates: first gather any missing members. def memberList = clazz.info.nonPrivateMembersAdmitting(VBRIDGE) - val (missing, rest) = memberList partition (m => m.isDeferredNotDefault && !ignoreDeferred(m)) + val (missing, rest) = memberList partition (m => m.isDeferredNotJavaDefault && !ignoreDeferred(m)) // Group missing members by the name of the underlying symbol, // to consolidate getters and setters. val grouped = missing groupBy (sym => analyzer.underlyingSymbol(sym).name) diff --git a/src/reflect/scala/reflect/internal/ClassfileConstants.scala b/src/reflect/scala/reflect/internal/ClassfileConstants.scala index d04bd3d3c8..e5d97e8959 100644 --- a/src/reflect/scala/reflect/internal/ClassfileConstants.scala +++ b/src/reflect/scala/reflect/internal/ClassfileConstants.scala @@ -344,7 +344,7 @@ object ClassfileConstants { case JAVA_ACC_STATIC => STATIC case JAVA_ACC_ABSTRACT => if (isAnnotation) 0L else if (isClass) ABSTRACT else DEFERRED case JAVA_ACC_INTERFACE => if (isAnnotation) 0L else TRAIT | INTERFACE | ABSTRACT - case JAVA_ACC_ENUM => ENUM + case JAVA_ACC_ENUM => JAVA_ENUM case JAVA_ACC_ANNOTATION => JAVA_ANNOTATION case _ => 0L } diff --git a/src/reflect/scala/reflect/internal/Definitions.scala b/src/reflect/scala/reflect/internal/Definitions.scala index 4e0fc1e36e..0bdf5b4647 100644 --- a/src/reflect/scala/reflect/internal/Definitions.scala +++ b/src/reflect/scala/reflect/internal/Definitions.scala @@ -815,7 +815,7 @@ trait Definitions extends api.StandardDefinitions { // must filter out "universal" members (getClass is deferred for some reason) val deferredMembers = ( tp membersBasedOnFlags (excludedFlags = BridgeAndPrivateFlags, requiredFlags = METHOD) - filter (mem => mem.isDeferredNotDefault && !isUniversalMember(mem)) // TODO: test + filter (mem => mem.isDeferredNotJavaDefault && !isUniversalMember(mem)) // TODO: test ) // if there is only one, it's monomorphic and has a single argument list diff --git a/src/reflect/scala/reflect/internal/FlagSets.scala b/src/reflect/scala/reflect/internal/FlagSets.scala index ef9c77878f..b6521634fb 100644 --- a/src/reflect/scala/reflect/internal/FlagSets.scala +++ b/src/reflect/scala/reflect/internal/FlagSets.scala @@ -42,7 +42,7 @@ trait FlagSets extends api.FlagSets { self: SymbolTable => val DEFAULTPARAM : FlagSet = Flags.DEFAULTPARAM val PRESUPER : FlagSet = Flags.PRESUPER val DEFAULTINIT : FlagSet = Flags.DEFAULTINIT - val ENUM : FlagSet = Flags.ENUM + val ENUM : FlagSet = Flags.JAVA_ENUM val PARAMACCESSOR : FlagSet = Flags.PARAMACCESSOR val CASEACCESSOR : FlagSet = Flags.CASEACCESSOR val SYNTHETIC : FlagSet = Flags.SYNTHETIC diff --git a/src/reflect/scala/reflect/internal/Flags.scala b/src/reflect/scala/reflect/internal/Flags.scala index ab56020713..754b96a9dd 100644 --- a/src/reflect/scala/reflect/internal/Flags.scala +++ b/src/reflect/scala/reflect/internal/Flags.scala @@ -15,65 +15,65 @@ import scala.collection.{ mutable, immutable } // // Generated by mkFlagsTable() at Thu Feb 02 20:31:52 PST 2012 // -// 0: PROTECTED/M -// 1: OVERRIDE/M -// 2: PRIVATE/M -// 3: ABSTRACT/M -// 4: DEFERRED/M -// 5: FINAL/M -// 6: METHOD -// 7: INTERFACE/M -// 8: MODULE -// 9: IMPLICIT/M -// 10: SEALED/M -// 11: CASE/M -// 12: MUTABLE/M -// 13: PARAM/M -// 14: PACKAGE -// 15: MACRO/M -// 16: BYNAMEPARAM/M CAPTURED COVARIANT/M -// 17: CONTRAVARIANT/M INCONSTRUCTOR LABEL -// 18: ABSOVERRIDE/M -// 19: LOCAL/M -// 20: JAVA/M -// 21: SYNTHETIC -// 22: STABLE -// 23: STATIC/M -// 24: CASEACCESSOR/M -// 25: DEFAULTPARAM/M TRAIT/M -// 26: BRIDGE -// 27: ACCESSOR -// 28: SUPERACCESSOR -// 29: PARAMACCESSOR/M -// 30: MODULEVAR -// 31: LAZY/M -// 32: IS_ERROR -// 33: OVERLOADED -// 34: LIFTED -// 35: EXISTENTIAL MIXEDIN -// 36: EXPANDEDNAME -// 37: IMPLCLASS PRESUPER/M -// 38: TRANS_FLAG -// 39: LOCKED -// 40: SPECIALIZED -// 41: DEFAULTINIT/M -// 42: VBRIDGE -// 43: VARARGS -// 44: TRIEDCOOKING -// 45: SYNCHRONIZED/M -// 46: ARTIFACT -// 47: DEFAULTMETHOD/M -// 48: ENUM +// 0: PROTECTED/M +// 1: OVERRIDE/M +// 2: PRIVATE/M +// 3: ABSTRACT/M +// 4: DEFERRED/M +// 5: FINAL/M +// 6: METHOD +// 7: INTERFACE/M +// 8: MODULE +// 9: IMPLICIT/M +// 10: SEALED/M +// 11: CASE/M +// 12: MUTABLE/M +// 13: PARAM/M +// 14: PACKAGE +// 15: MACRO/M +// 16: BYNAMEPARAM/M CAPTURED COVARIANT/M +// 17: CONTRAVARIANT/M INCONSTRUCTOR LABEL +// 18: ABSOVERRIDE/M +// 19: LOCAL/M +// 20: JAVA/M +// 21: SYNTHETIC +// 22: STABLE +// 23: STATIC/M +// 24: CASEACCESSOR/M +// 25: DEFAULTPARAM/M TRAIT/M +// 26: BRIDGE +// 27: ACCESSOR +// 28: SUPERACCESSOR +// 29: PARAMACCESSOR/M +// 30: MODULEVAR +// 31: LAZY/M +// 32: IS_ERROR +// 33: OVERLOADED +// 34: LIFTED +// 35: EXISTENTIAL MIXEDIN +// 36: EXPANDEDNAME +// 37: IMPLCLASS PRESUPER/M +// 38: TRANS_FLAG +// 39: LOCKED +// 40: SPECIALIZED +// 41: DEFAULTINIT/M +// 42: VBRIDGE +// 43: VARARGS +// 44: TRIEDCOOKING +// 45: SYNCHRONIZED/M +// 46: ARTIFACT +// 47: JAVA_DEFAULTMETHOD/M +// 48: JAVA_ENUM // 49: JAVA_ANNOTATION // 50: -// 51: lateDEFERRED -// 52: lateFINAL -// 53: lateMETHOD -// 54: lateINTERFACE -// 55: lateMODULE -// 56: notPROTECTED -// 57: notOVERRIDE -// 58: notPRIVATE +// 51: lateDEFERRED +// 52: lateFINAL +// 53: lateMETHOD +// 54: lateINTERFACE +// 55: lateMODULE +// 56: notPROTECTED +// 57: notOVERRIDE +// 58: notPRIVATE // 59: // 60: // 61: @@ -119,9 +119,9 @@ class ModifierFlags { final val DEFAULTINIT = 1L << 41 // symbol is initialized to the default value: used by -Xcheckinit final val ARTIFACT = 1L << 46 // symbol should be ignored when typechecking; will be marked ACC_SYNTHETIC in bytecode // to see which symbols are marked as ARTIFACT, see scaladocs for FlagValues.ARTIFACT - final val DEFAULTMETHOD = 1L << 47 // symbol is a java default method - final val ENUM = 1L << 48 // symbol is a java enum - final val JAVA_ANNOTATION = 1L << 49 // symbol is a java annotation + final val JAVA_DEFAULTMETHOD = 1L << 47 // symbol is a java default method + final val JAVA_ENUM = 1L << 48 // symbol is a java enum + final val JAVA_ANNOTATION = 1L << 49 // symbol is a java annotation // Overridden. def flagToString(flag: Long): String = "" @@ -260,7 +260,7 @@ class Flags extends ModifierFlags { */ final val ExplicitFlags = PRIVATE | PROTECTED | ABSTRACT | FINAL | SEALED | - OVERRIDE | CASE | IMPLICIT | ABSOVERRIDE | LAZY | DEFAULTMETHOD + OVERRIDE | CASE | IMPLICIT | ABSOVERRIDE | LAZY | JAVA_DEFAULTMETHOD /** The two bridge flags */ final val BridgeFlags = BRIDGE | VBRIDGE @@ -451,8 +451,8 @@ class Flags extends ModifierFlags { case TRIEDCOOKING => "" // (1L << 44) case SYNCHRONIZED => "" // (1L << 45) case ARTIFACT => "" // (1L << 46) - case DEFAULTMETHOD => "" // (1L << 47) - case ENUM => "" // (1L << 48) + case JAVA_DEFAULTMETHOD => "" // (1L << 47) + case JAVA_ENUM => "" // (1L << 48) case JAVA_ANNOTATION => "" // (1L << 49) case 0x4000000000000L => "" // (1L << 50) case `lateDEFERRED` => "" // (1L << 51) diff --git a/src/reflect/scala/reflect/internal/HasFlags.scala b/src/reflect/scala/reflect/internal/HasFlags.scala index 17f28ab1ca..5162b15206 100644 --- a/src/reflect/scala/reflect/internal/HasFlags.scala +++ b/src/reflect/scala/reflect/internal/HasFlags.scala @@ -79,50 +79,50 @@ trait HasFlags { // Tests which come through cleanly: both Symbol and Modifiers use these // identically, testing for a single flag. - def hasAbstractFlag = hasFlag(ABSTRACT) - def hasAccessorFlag = hasFlag(ACCESSOR) - def hasDefault = hasFlag(DEFAULTPARAM) && hasFlag(METHOD | PARAM) // Second condition disambiguates with TRAIT - def hasEnumFlag = hasFlag(ENUM) + def hasAbstractFlag = hasFlag(ABSTRACT) + def hasAccessorFlag = hasFlag(ACCESSOR) + def hasDefault = hasFlag(DEFAULTPARAM) && hasFlag(METHOD | PARAM) // Second condition disambiguates with TRAIT + def hasJavaEnumFlag = hasFlag(JAVA_ENUM) def hasJavaAnnotationFlag = hasFlag(JAVA_ANNOTATION) @deprecated("Use isLocalToThis instead", "2.11.0") - def hasLocalFlag = hasFlag(LOCAL) - def isLocalToThis = hasFlag(LOCAL) - def hasModuleFlag = hasFlag(MODULE) - def hasPackageFlag = hasFlag(PACKAGE) - def hasStableFlag = hasFlag(STABLE) - def hasStaticFlag = hasFlag(STATIC) - def isAbstractOverride = hasFlag(ABSOVERRIDE) - def isAnyOverride = hasFlag(OVERRIDE | ABSOVERRIDE) - def isCase = hasFlag(CASE) - def isCaseAccessor = hasFlag(CASEACCESSOR) - def isDeferred = hasFlag(DEFERRED) - def isFinal = hasFlag(FINAL) - def isArtifact = hasFlag(ARTIFACT) - def isImplicit = hasFlag(IMPLICIT) - def isInterface = hasFlag(INTERFACE) - def isJavaDefined = hasFlag(JAVA) - def isLabel = hasAllFlags(LABEL | METHOD) && !hasAccessorFlag - def isLazy = hasFlag(LAZY) - def isLifted = hasFlag(LIFTED) - def isMacro = hasFlag(MACRO) - def isMutable = hasFlag(MUTABLE) - def isOverride = hasFlag(OVERRIDE) - def isParamAccessor = hasFlag(PARAMACCESSOR) - def isPrivate = hasFlag(PRIVATE) + def hasLocalFlag = hasFlag(LOCAL) + def isLocalToThis = hasFlag(LOCAL) + def hasModuleFlag = hasFlag(MODULE) + def hasPackageFlag = hasFlag(PACKAGE) + def hasStableFlag = hasFlag(STABLE) + def hasStaticFlag = hasFlag(STATIC) + def isAbstractOverride = hasFlag(ABSOVERRIDE) + def isAnyOverride = hasFlag(OVERRIDE | ABSOVERRIDE) + def isCase = hasFlag(CASE) + def isCaseAccessor = hasFlag(CASEACCESSOR) + def isDeferred = hasFlag(DEFERRED) + def isFinal = hasFlag(FINAL) + def isArtifact = hasFlag(ARTIFACT) + def isImplicit = hasFlag(IMPLICIT) + def isInterface = hasFlag(INTERFACE) + def isJavaDefined = hasFlag(JAVA) + def isLabel = hasAllFlags(LABEL | METHOD) && !hasAccessorFlag + def isLazy = hasFlag(LAZY) + def isLifted = hasFlag(LIFTED) + def isMacro = hasFlag(MACRO) + def isMutable = hasFlag(MUTABLE) + def isOverride = hasFlag(OVERRIDE) + def isParamAccessor = hasFlag(PARAMACCESSOR) + def isPrivate = hasFlag(PRIVATE) @deprecated ("Use `hasPackageFlag` instead", "2.11.0") - def isPackage = hasFlag(PACKAGE) - def isPrivateLocal = hasAllFlags(PrivateLocal) - def isProtected = hasFlag(PROTECTED) - def isProtectedLocal = hasAllFlags(ProtectedLocal) - def isPublic = hasNoFlags(PRIVATE | PROTECTED) && !hasAccessBoundary - def isSealed = hasFlag(SEALED) - def isSpecialized = hasFlag(SPECIALIZED) - def isSuperAccessor = hasFlag(SUPERACCESSOR) - def isSynthetic = hasFlag(SYNTHETIC) - def isTrait = hasFlag(TRAIT) && !hasFlag(PARAM) - - def isDeferredOrDefault = hasFlag(DEFERRED | DEFAULTMETHOD) - def isDeferredNotDefault = isDeferred && !hasFlag(DEFAULTMETHOD) + def isPackage = hasFlag(PACKAGE) + def isPrivateLocal = hasAllFlags(PrivateLocal) + def isProtected = hasFlag(PROTECTED) + def isProtectedLocal = hasAllFlags(ProtectedLocal) + def isPublic = hasNoFlags(PRIVATE | PROTECTED) && !hasAccessBoundary + def isSealed = hasFlag(SEALED) + def isSpecialized = hasFlag(SPECIALIZED) + def isSuperAccessor = hasFlag(SUPERACCESSOR) + def isSynthetic = hasFlag(SYNTHETIC) + def isTrait = hasFlag(TRAIT) && !hasFlag(PARAM) + + def isDeferredOrJavaDefault = hasFlag(DEFERRED | JAVA_DEFAULTMETHOD) + def isDeferredNotJavaDefault = isDeferred && !hasFlag(JAVA_DEFAULTMETHOD) def flagBitsToString(bits: Long): String = { // Fast path for common case -- cgit v1.2.3 From 2678d349b2b2738d9db38d890199f32aa39d8c3e Mon Sep 17 00:00:00 2001 From: Lukas Rytz Date: Wed, 22 Jul 2015 20:51:05 +0200 Subject: SI-9403 fix ICodeReader for negative BIPUSH / SIPUSH values The byte value of a BIPUSH instruction and the (byte1 << 8) | byte2 value of a SIPUSH instruction are signed, see [1] and [2]. Similar for the increment value of IINC [3]. [1] https://docs.oracle.com/javase/specs/jvms/se8/html/jvms-6.html#jvms-6.5.bipush [2] https://docs.oracle.com/javase/specs/jvms/se8/html/jvms-6.html#jvms-6.5.sipush [3] https://docs.oracle.com/javase/specs/jvms/se8/html/jvms-6.html#jvms-6.5.iinc --- .../nsc/symtab/classfile/ClassfileParser.scala | 3 +++ .../tools/nsc/symtab/classfile/ICodeReader.scala | 6 ++--- test/files/run/t9403.flags | 1 + test/files/run/t9403/C_1.scala | 5 ++++ test/files/run/t9403/Test_2.scala | 29 ++++++++++++++++++++++ 5 files changed, 41 insertions(+), 3 deletions(-) create mode 100644 test/files/run/t9403.flags create mode 100644 test/files/run/t9403/C_1.scala create mode 100644 test/files/run/t9403/Test_2.scala diff --git a/src/compiler/scala/tools/nsc/symtab/classfile/ClassfileParser.scala b/src/compiler/scala/tools/nsc/symtab/classfile/ClassfileParser.scala index 660028eab8..74b531a0d9 100644 --- a/src/compiler/scala/tools/nsc/symtab/classfile/ClassfileParser.scala +++ b/src/compiler/scala/tools/nsc/symtab/classfile/ClassfileParser.scala @@ -84,6 +84,9 @@ abstract class ClassfileParser { protected final def u2(): Int = in.nextChar.toInt protected final def u4(): Int = in.nextInt + protected final def s1(): Int = in.nextByte.toInt // sign-extend the byte to int + protected final def s2(): Int = (in.nextByte.toInt << 8) | u1 // sign-extend and shift the first byte, or with the unsigned second byte + private def readInnerClassFlags() = readClassFlags() private def readClassFlags() = JavaAccFlags classFlags u2 private def readMethodFlags() = JavaAccFlags methodFlags u2 diff --git a/src/compiler/scala/tools/nsc/symtab/classfile/ICodeReader.scala b/src/compiler/scala/tools/nsc/symtab/classfile/ICodeReader.scala index 438a71061e..b2f5a4119d 100644 --- a/src/compiler/scala/tools/nsc/symtab/classfile/ICodeReader.scala +++ b/src/compiler/scala/tools/nsc/symtab/classfile/ICodeReader.scala @@ -326,8 +326,8 @@ abstract class ICodeReader extends ClassfileParser { case JVM.dconst_0 => code emit CONSTANT(Constant(0.0)) case JVM.dconst_1 => code emit CONSTANT(Constant(1.0)) - case JVM.bipush => code.emit(CONSTANT(Constant(u1))); size += 1 - case JVM.sipush => code.emit(CONSTANT(Constant(u2))); size += 2 + case JVM.bipush => code.emit(CONSTANT(Constant(s1))); size += 1 + case JVM.sipush => code.emit(CONSTANT(Constant(s2))); size += 2 case JVM.ldc => code.emit(CONSTANT(pool.getConstant(u1))); size += 1 case JVM.ldc_w => code.emit(CONSTANT(pool.getConstant(u2))); size += 2 case JVM.ldc2_w => code.emit(CONSTANT(pool.getConstant(u2))); size += 2 @@ -466,7 +466,7 @@ abstract class ICodeReader extends ClassfileParser { size += 2 val local = code.getLocal(u1, INT) code.emit(LOAD_LOCAL(local)) - code.emit(CONSTANT(Constant(u1))) + code.emit(CONSTANT(Constant(s1))) code.emit(CALL_PRIMITIVE(Arithmetic(ADD, INT))) code.emit(STORE_LOCAL(local)) diff --git a/test/files/run/t9403.flags b/test/files/run/t9403.flags new file mode 100644 index 0000000000..307668060c --- /dev/null +++ b/test/files/run/t9403.flags @@ -0,0 +1 @@ +-Ybackend:GenASM -optimize diff --git a/test/files/run/t9403/C_1.scala b/test/files/run/t9403/C_1.scala new file mode 100644 index 0000000000..439af1a386 --- /dev/null +++ b/test/files/run/t9403/C_1.scala @@ -0,0 +1,5 @@ +package p +class C { + @inline final def f(x: Int): Long = 10L / (if (x < 0) -2 else 2) + @inline final def g(x: Int): Long = 3000L / (if (x < 0) -300 else 300) +} diff --git a/test/files/run/t9403/Test_2.scala b/test/files/run/t9403/Test_2.scala new file mode 100644 index 0000000000..fb2777b9a8 --- /dev/null +++ b/test/files/run/t9403/Test_2.scala @@ -0,0 +1,29 @@ +import p.C +import scala.tools.asm.Opcodes +import scala.tools.partest.BytecodeTest +import scala.tools.partest.ASMConverters._ + + +object Test extends BytecodeTest { + def foo(c: C, x: Int) = c.f(x) + def goo(c: C, x: Int) = c.g(x) + + def has(i: Instruction, c: String, m: String) = { + val cls = loadClassNode(c) + val mth = convertMethod(getMethod(cls, m)) + assert(mth.instructions.contains(i)) + } + + def show(): Unit = { + assert(foo(new C, -2) == -5L) + assert(goo(new C, -2) == -10L) + + val bipush2 = IntOp(Opcodes.BIPUSH, -2) + has(bipush2, "p.C", "f") + has(bipush2, "Test$", "foo") + + val sipush300 = IntOp(Opcodes.SIPUSH, -300) + has(sipush300, "p.C", "g") + has(sipush300, "Test$", "goo") + } +} -- cgit v1.2.3 From 0c99742c51706ee4b60a56a8f5babb13682f9b10 Mon Sep 17 00:00:00 2001 From: Jason Zaugg Date: Mon, 27 Jul 2015 15:33:30 +1000 Subject: SI-9365 Don't null out dependencies of transient lazy vals As per Iulian's analysis: > When lazy vals are transient, the optimization in SI-720 is invalid, > leading to NPEs. These NPEs appear when recomputing the lazy val when deserializaing the object. This commit disables the field nulling if the lazy val is marked transient. The post-mixin tree in the enclosed test changes as follows: ``` --- sandbox/old.log 2015-07-27 15:48:03.000000000 +1000 +++ sandbox/new.log 2015-07-27 15:47:56.000000000 +1000 @@ -1,61 +1,54 @@ [[syntax trees at end of mixin]] // t9365.scala package { class Test extends Object with Serializable { @transient @volatile private[this] var bitmap$trans$0: Boolean = false; private def foo$lzycompute(): Object = { { Test.this.synchronized({ if (Test.this.bitmap$trans$0.unary_!()) { Test.this.foo = Test.this.x.apply(); Test.this.bitmap$trans$0 = true; () }; scala.runtime.BoxedUnit.UNIT }); - Test.this.x = null + () }; Test.this.foo }; ``` In addition to the test from the ticket, I've added a reflection based test that directly tests the nulling. This complements the test added in 449f2a7473, the fix for SI-720, which passes by virtue of not exhausting the heap. --- src/compiler/scala/tools/nsc/transform/Mixin.scala | 2 +- test/files/run/t720.scala | 48 ++++++++++++++++++++++ test/files/run/t9365.check | 2 + test/files/run/t9365.scala | 18 ++++++++ 4 files changed, 69 insertions(+), 1 deletion(-) create mode 100644 test/files/run/t720.scala create mode 100644 test/files/run/t9365.check create mode 100644 test/files/run/t9365.scala diff --git a/src/compiler/scala/tools/nsc/transform/Mixin.scala b/src/compiler/scala/tools/nsc/transform/Mixin.scala index 25d45cc819..a079a76ce7 100644 --- a/src/compiler/scala/tools/nsc/transform/Mixin.scala +++ b/src/compiler/scala/tools/nsc/transform/Mixin.scala @@ -1122,7 +1122,7 @@ abstract class Mixin extends InfoTransform with ast.TreeDSL { if (scope exists (_.isLazy)) { val map = mutable.Map[Symbol, Set[Symbol]]() withDefaultValue Set() // check what fields can be nulled for - for ((field, users) <- singleUseFields(templ); lazyFld <- users) + for ((field, users) <- singleUseFields(templ); lazyFld <- users if !lazyFld.accessed.hasAnnotation(TransientAttr)) map(lazyFld) += field map.toMap diff --git a/test/files/run/t720.scala b/test/files/run/t720.scala new file mode 100644 index 0000000000..a5cb2495cf --- /dev/null +++ b/test/files/run/t720.scala @@ -0,0 +1,48 @@ +class Lazy(f: => Int) { + lazy val get: Int = f +} + +class UsedLater(f: => Int) { + lazy val get: Int = f + def other = f +} + +class TransientLazy(f: => Int) { + @transient + lazy val get: Int = f +} + +object Test { + def main(args: Array[String]): Unit = { + testLazy() + testUsedLater() + } + + def testLazy() { + val o = new Lazy("".length) + val f = classOf[Lazy].getDeclaredField("f") + f.setAccessible(true) + assert(f.get(o) != null) + o.get + assert(f.get(o) == null) + } + + def testUsedLater() { + val o = new UsedLater("".length) + val f = classOf[UsedLater].getDeclaredField("f") + f.setAccessible(true) + assert(f.get(o) != null) + o.get + assert(f.get(o) != null) + } + + def testTransientLazy() { + val o = new TransientLazy("".length) + val f = classOf[TransientLazy].getDeclaredField("f") + f.setAccessible(true) + assert(f.get(o) != null) + o.get + assert(f.get(o) != null) // SI-9365 + } +} + diff --git a/test/files/run/t9365.check b/test/files/run/t9365.check new file mode 100644 index 0000000000..0d55bed3a3 --- /dev/null +++ b/test/files/run/t9365.check @@ -0,0 +1,2 @@ +foo +foo diff --git a/test/files/run/t9365.scala b/test/files/run/t9365.scala new file mode 100644 index 0000000000..0c4477dda9 --- /dev/null +++ b/test/files/run/t9365.scala @@ -0,0 +1,18 @@ +class Test(x: => Object) extends Serializable { + @transient lazy val foo = x +} + +object Test { + def main(args: Array[String]): Unit = { + import java.io._ + val t = new Test("foo") + println(t.foo) + val baos = new ByteArrayOutputStream + val dos = new ObjectOutputStream(baos) + dos.writeObject(t) + dos.close() + val dis = new ObjectInputStream(new ByteArrayInputStream(baos.toByteArray())) + val t1 = dis.readObject().asInstanceOf[Test] + println(t1.foo) // was NPE + } +} -- cgit v1.2.3 From 173a6fad95c85a2b6d2c773d273431667d836016 Mon Sep 17 00:00:00 2001 From: Janek Bogucki Date: Mon, 27 Jul 2015 08:51:26 +0100 Subject: Remove redundant 'val' from case class params. --- src/compiler/scala/tools/nsc/typechecker/Macros.scala | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/compiler/scala/tools/nsc/typechecker/Macros.scala b/src/compiler/scala/tools/nsc/typechecker/Macros.scala index 10aefae20b..99dd81c7e2 100644 --- a/src/compiler/scala/tools/nsc/typechecker/Macros.scala +++ b/src/compiler/scala/tools/nsc/typechecker/Macros.scala @@ -85,9 +85,9 @@ trait Macros extends MacroRuntimes with Traces with Helpers { */ case class MacroImplBinding( // Is this macro impl a bundle (a trait extending *box.Macro) or a vanilla def? - val isBundle: Boolean, + isBundle: Boolean, // Is this macro impl blackbox (i.e. having blackbox.Context in its signature)? - val isBlackbox: Boolean, + isBlackbox: Boolean, // Java class name of the class that contains the macro implementation // is used to load the corresponding object with Java reflection className: String, -- cgit v1.2.3 From ed5098dbc482f79140300acfab942dad2491a9b0 Mon Sep 17 00:00:00 2001 From: Seth Tisue Date: Mon, 27 Jul 2015 12:44:08 -0400 Subject: merge two reviewers lists in readme --- README.md | 32 +++++++++----------------------- 1 file changed, 9 insertions(+), 23 deletions(-) diff --git a/README.md b/README.md index 45138a281b..2655de43de 100644 --- a/README.md +++ b/README.md @@ -13,32 +13,18 @@ If you need some help with your PR at any time, please feel free to @-mention an | username | talk to me about... | --------------------------------------------------------------------------------------------------|----------------------------------------------------------------|---------------------------------------------------| - | [`@adriaanm`](https://github.com/adriaanm) | how we can help // type checker, pattern matcher, infrastructure | - | [`@SethTisue`](https://github.com/SethTisue) | back-end, library, improving the *welcome to Scala* experience | - | [`@retronym`](https://github.com/retronym) | Java 8 lambdas, tricky bug detective work | - | [`@Ichoran`](https://github.com/Ichoran) | the collections library, performance | + | [`@adriaanm`](https://github.com/adriaanm) | type checker, pattern matcher, infrastructure, language spec | + | [`@SethTisue`](https://github.com/SethTisue) | back-end, library, the welcome-to-Scala experience, build | + | [`@retronym`](https://github.com/retronym) | compiler performance, weird compiler bugs, Java 8 lambdas, REPL | + | [`@Ichoran`](https://github.com/Ichoran) | collections library, performance | | [`@lrytz`](https://github.com/lrytz) | optimizer, named & default arguments | - | [`@VladUreche`](https://github.com/VladUreche) | specialization & the scaladoc tool | + | [`@VladUreche`](https://github.com/VladUreche) | specialization, Scaladoc tool | | [`@densh`](https://github.com/densh) | quasiquotes, parser, string interpolators, macros in standard library | | [`@xeno-by`](https://github.com/xeno-by) | macros and reflection | - | [`@dickwall`](https://github.com/dickwall) | process & community | - -additional reviewer suggestions: - -* library: @axel22 (Aleksander Prokopec -- concurrent & collection) -* specialisation: @axel22 (Aleksander Prokopec), @vladureche (Vlad Ureche), @dragos (Iulian Dragos) -* named / default args, annotations, plugins: @lrytz (Lukas Rytz) -* macros, reflection, manifests, string interpolation: @xeno-by (Eugene Burmako) -* type checker, inference: @adriaanm (Adriaan Moors) -* language specification: @adriaanm (Adriaan Moors) -* new pattern matcher, implicit search: @adriaanm (Adriaan Moors) -* backend: @lrytz (Lukas Rytz), @retronym (Jason Zaugg), @dragos (Iulian Dragos) -* repl, compiler performance: @retronym (Jason Zaugg) -* scaladoc: @dickwall (Dick Wall), @vladureche (Vlad Ureche) -* optimizer: @vladureche (Vlad Ureche), @lrytz (Lukas Rytz) -* build: @SethTisue -* random compiler bugs: @retronym, @lrytz, @adriaanm -* documentation: @heathermiller (Heather Miller), @dickwall (Dick Wall) + | [`@heathermiller`](https://github.com/heathermiller) | documentation | + | [`@dickwall`](https://github.com/dickwall) | process & community, documentation | + | [`@dragos`](https://github.com/dragos) | specialization, back end | + | [`@axel22`](https://github.com/axel22) | collections, concurrency, specialization | P.S.: If you have some spare time to help out around here, we would be delighted to add your name to this list! -- cgit v1.2.3 From 600fc4e6e1c81b85b049e4841156d9e0f77b4e08 Mon Sep 17 00:00:00 2001 From: Seth Tisue Date: Mon, 27 Jul 2015 13:14:58 -0400 Subject: fix readme for death of typesafe.artifactoryonline.com there's no need for a link here anyway --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index e722c88e41..7ae407a121 100644 --- a/README.md +++ b/README.md @@ -158,7 +158,7 @@ ideas and coordinate your effort with others. Here are some common commands. Most ant targets offer a `-opt` variant that runs under `-optimise` (CI runs the -optimize variant). - - `./pull-binary-libs.sh` [downloads](http://typesafe.artifactoryonline.com/typesafe) all binary artifacts associated with this commit. + - `./pull-binary-libs.sh` downloads all binary artifacts associated with this commit. - `ant -p` prints out information about the commonly used ant targets. - `ant` or `ant build`: A quick compilation (to build/quick) of your changes using the locker compiler. -- cgit v1.2.3 From cdc55c29d02258aac57bd0d6e6d2d4e87bd392f5 Mon Sep 17 00:00:00 2001 From: Lukas Rytz Date: Tue, 28 Jul 2015 10:21:10 +0200 Subject: Upgrade scala-asm to 5.0.4-scala-3 Fixes a crash in the backend when the bytecode of a method is too large (https://github.com/scala/scala-asm/issues/8). --- versions.properties | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/versions.properties b/versions.properties index 529365c9f9..c3270736d8 100644 --- a/versions.properties +++ b/versions.properties @@ -33,7 +33,7 @@ scala-swing.version.number=1.0.2 akka-actor.version.number=2.3.10 actors-migration.version.number=1.1.0 jline.version=2.12.1 -scala-asm.version=5.0.4-scala-2 +scala-asm.version=5.0.4-scala-3 # external modules, used internally (not shipped) partest.version.number=1.0.7 -- cgit v1.2.3 From 69c2c106fedd60f5c29a4aad696dec1de5e85200 Mon Sep 17 00:00:00 2001 From: Janek Bogucki Date: Tue, 28 Jul 2015 19:13:44 +0100 Subject: ScalaDoc fixes for library and library-aux --- src/library-aux/scala/AnyRef.scala | 4 ++-- src/library/scala/StringContext.scala | 2 +- src/library/scala/collection/GenSeqLike.scala | 2 +- src/library/scala/collection/immutable/Stream.scala | 2 +- src/library/scala/compat/Platform.scala | 2 +- src/library/scala/math/BigDecimal.scala | 2 +- src/library/scala/reflect/Manifest.scala | 4 ++-- test/scaladoc/resources/doc-root/AnyRef.scala | 4 ++-- 8 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/library-aux/scala/AnyRef.scala b/src/library-aux/scala/AnyRef.scala index 8c1862e729..7217499da7 100644 --- a/src/library-aux/scala/AnyRef.scala +++ b/src/library-aux/scala/AnyRef.scala @@ -45,7 +45,7 @@ trait AnyRef extends Any { */ def synchronized[T](body: => T): T - /** Tests whether the argument (`arg0`) is a reference to the receiver object (`this`). + /** Tests whether the argument (`that`) is a reference to the receiver object (`this`). * * The `eq` method implements an [[http://en.wikipedia.org/wiki/Equivalence_relation equivalence relation]] on * non-null instances of `AnyRef`, and has three additional properties: @@ -73,7 +73,7 @@ trait AnyRef extends Any { /** The expression `x == that` is equivalent to `if (x eq null) that eq null else x.equals(that)`. * - * @param arg0 the object to compare against this object for equality. + * @param that the object to compare against this object for equality. * @return `true` if the receiver object is equivalent to the argument; `false` otherwise. */ final def ==(that: Any): Boolean = diff --git a/src/library/scala/StringContext.scala b/src/library/scala/StringContext.scala index e60fa2f290..69533c12da 100644 --- a/src/library/scala/StringContext.scala +++ b/src/library/scala/StringContext.scala @@ -173,7 +173,7 @@ object StringContext { /** An exception that is thrown if a string contains a backslash (`\`) character * that does not start a valid escape sequence. * @param str The offending string - * @param idx The index of the offending backslash character in `str`. + * @param index The index of the offending backslash character in `str`. */ class InvalidEscapeException(str: String, @deprecatedName('idx) val index: Int) extends IllegalArgumentException( s"""invalid escape ${ diff --git a/src/library/scala/collection/GenSeqLike.scala b/src/library/scala/collection/GenSeqLike.scala index f786293822..be1da1660a 100644 --- a/src/library/scala/collection/GenSeqLike.scala +++ b/src/library/scala/collection/GenSeqLike.scala @@ -274,7 +274,7 @@ trait GenSeqLike[+A, +Repr] extends Any with GenIterableLike[A, Repr] with Equal * @tparam B the element type of the returned $coll. * @tparam That $thatinfo * @param bf $bfinfo - * @return a new $coll` which is a copy of this $coll with the element at position `index` replaced by `elem`. + * @return a new $coll which is a copy of this $coll with the element at position `index` replaced by `elem`. * @throws IndexOutOfBoundsException if `index` does not satisfy `0 <= index < length`. * * @usecase def updated(index: Int, elem: A): $Coll[A] diff --git a/src/library/scala/collection/immutable/Stream.scala b/src/library/scala/collection/immutable/Stream.scala index 17cf02cce6..d8f0559706 100644 --- a/src/library/scala/collection/immutable/Stream.scala +++ b/src/library/scala/collection/immutable/Stream.scala @@ -1190,7 +1190,7 @@ object Stream extends SeqFactory[Stream] { def #:::(prefix: Stream[A]): Stream[A] = prefix append tl } - /** A wrapper method that adds `#::` for cons and `#::: for concat as operations + /** A wrapper method that adds `#::` for cons and `#:::` for concat as operations * to streams. */ implicit def consWrapper[A](stream: => Stream[A]): ConsWrapper[A] = diff --git a/src/library/scala/compat/Platform.scala b/src/library/scala/compat/Platform.scala index 4c82d6e15b..42dfcbfdde 100644 --- a/src/library/scala/compat/Platform.scala +++ b/src/library/scala/compat/Platform.scala @@ -41,7 +41,7 @@ object Platform { * @throws java.lang.ArrayStoreException If either `src` or `dest` are not of type * [java.lang.Array]; or if the element type of `src` is not * compatible with that of `dest`. - * @throws java.lang.IndexOutOfBoundsException If either srcPos` or `destPos` are + * @throws java.lang.IndexOutOfBoundsException If either `srcPos` or `destPos` are * outside of the bounds of their respective arrays; or if `length` * is negative; or if there are less than `length` elements available * after `srcPos` or `destPos` in `src` and `dest` respectively. diff --git a/src/library/scala/math/BigDecimal.scala b/src/library/scala/math/BigDecimal.scala index 6bb35606a6..bb337e7a1d 100644 --- a/src/library/scala/math/BigDecimal.scala +++ b/src/library/scala/math/BigDecimal.scala @@ -124,7 +124,7 @@ object BigDecimal { */ def exact(s: String): BigDecimal = exact(new BigDec(s)) - /** Constructs a 'BigDecimal` that exactly represents the number + /** Constructs a `BigDecimal` that exactly represents the number * specified in base 10 in a character array. */ def exact(cs: Array[Char]): BigDecimal = exact(new BigDec(cs)) diff --git a/src/library/scala/reflect/Manifest.scala b/src/library/scala/reflect/Manifest.scala index 2f7643bccf..4ff49c44d0 100644 --- a/src/library/scala/reflect/Manifest.scala +++ b/src/library/scala/reflect/Manifest.scala @@ -248,7 +248,7 @@ object ManifestFactory { def arrayType[T](arg: Manifest[_]): Manifest[Array[T]] = arg.asInstanceOf[Manifest[T]].arrayManifest - /** Manifest for the abstract type `prefix # name'. `upperBound` is not + /** Manifest for the abstract type `prefix # name`. `upperBound` is not * strictly necessary as it could be obtained by reflection. It was * added so that erasure can be calculated without reflection. */ def abstractType[T](prefix: Manifest[_], name: String, upperBound: Predef.Class[_], args: Manifest[_]*): Manifest[T] = @@ -269,7 +269,7 @@ object ManifestFactory { (if (upperBound eq Nothing) "" else " <: "+upperBound) } - /** Manifest for the intersection type `parents_0 with ... with parents_n'. */ + /** Manifest for the intersection type `parents_0 with ... with parents_n`. */ def intersectionType[T](parents: Manifest[_]*): Manifest[T] = new Manifest[T] { def runtimeClass = parents.head.runtimeClass diff --git a/test/scaladoc/resources/doc-root/AnyRef.scala b/test/scaladoc/resources/doc-root/AnyRef.scala index 362fbcf0f5..7cdc3d1ada 100644 --- a/test/scaladoc/resources/doc-root/AnyRef.scala +++ b/test/scaladoc/resources/doc-root/AnyRef.scala @@ -45,7 +45,7 @@ trait AnyRef extends Any { */ def synchronized[T](body: => T): T - /** Tests whether the argument (`arg0`) is a reference to the receiver object (`this`). + /** Tests whether the argument (`that`) is a reference to the receiver object (`this`). * * The `eq` method implements an [[http://en.wikipedia.org/wiki/Equivalence_relation equivalence relation]] on * non-null instances of `AnyRef`, and has three additional properties: @@ -73,7 +73,7 @@ trait AnyRef extends Any { /** The expression `x == that` is equivalent to `if (x eq null) that eq null else x.equals(that)`. * - * @param arg0 the object to compare against this object for equality. + * @param that the object to compare against this object for equality. * @return `true` if the receiver object is equivalent to the argument; `false` otherwise. */ final def ==(that: AnyRef): Boolean = -- cgit v1.2.3 From e206a1837d116136f3e2c93875c1f9ee08166dc1 Mon Sep 17 00:00:00 2001 From: Janek Bogucki Date: Tue, 28 Jul 2015 22:42:24 +0100 Subject: ScalaDoc fixes for reflect --- src/reflect/scala/reflect/api/Trees.scala | 2 +- src/reflect/scala/reflect/api/TypeTags.scala | 2 +- src/reflect/scala/reflect/internal/ExistentialsAndSkolems.scala | 2 +- src/reflect/scala/reflect/internal/SymbolPairs.scala | 2 +- src/reflect/scala/reflect/internal/TreeGen.scala | 7 +++---- src/reflect/scala/reflect/internal/Types.scala | 6 +++--- src/reflect/scala/reflect/internal/pickling/UnPickler.scala | 2 +- .../scala/reflect/internal/util/AbstractFileClassLoader.scala | 2 +- 8 files changed, 12 insertions(+), 13 deletions(-) diff --git a/src/reflect/scala/reflect/api/Trees.scala b/src/reflect/scala/reflect/api/Trees.scala index 2bf407ee19..a43195d9b6 100644 --- a/src/reflect/scala/reflect/api/Trees.scala +++ b/src/reflect/scala/reflect/api/Trees.scala @@ -143,7 +143,7 @@ trait Trees { self: Universe => /** Find all subtrees matching predicate `p`. Same as `withFilter` */ def filter(f: Tree => Boolean): List[Tree] - /** Apply `pf' to each subtree on which the function is defined and collect the results. + /** Apply `pf` to each subtree on which the function is defined and collect the results. */ def collect[T](pf: PartialFunction[Tree, T]): List[T] diff --git a/src/reflect/scala/reflect/api/TypeTags.scala b/src/reflect/scala/reflect/api/TypeTags.scala index 7db375ca61..bc239ca870 100644 --- a/src/reflect/scala/reflect/api/TypeTags.scala +++ b/src/reflect/scala/reflect/api/TypeTags.scala @@ -53,7 +53,7 @@ import java.io.ObjectStreamException * Each of these methods constructs a `TypeTag[T]` or `ClassTag[T]` for the given * type argument `T`. * - * === #2 Using an implicit parameter of type `TypeTag[T]`, `ClassTag[T]`, or `WeakTypeTag[T] + * === #2 Using an implicit parameter of type `TypeTag[T]`, `ClassTag[T]`, or `WeakTypeTag[T]` * * For example: * {{{ diff --git a/src/reflect/scala/reflect/internal/ExistentialsAndSkolems.scala b/src/reflect/scala/reflect/internal/ExistentialsAndSkolems.scala index 0eeca4aace..3e18f88f80 100644 --- a/src/reflect/scala/reflect/internal/ExistentialsAndSkolems.scala +++ b/src/reflect/scala/reflect/internal/ExistentialsAndSkolems.scala @@ -110,7 +110,7 @@ trait ExistentialsAndSkolems { /** * Compute an existential type from hidden symbols `hidden` and type `tp`. * @param hidden The symbols that will be existentially abstracted - * @param hidden The original type + * @param tp The original type * @param rawOwner The owner for Java raw types. */ final def packSymbols(hidden: List[Symbol], tp: Type, rawOwner: Symbol = NoSymbol): Type = diff --git a/src/reflect/scala/reflect/internal/SymbolPairs.scala b/src/reflect/scala/reflect/internal/SymbolPairs.scala index 4763e77a34..a52d2d8510 100644 --- a/src/reflect/scala/reflect/internal/SymbolPairs.scala +++ b/src/reflect/scala/reflect/internal/SymbolPairs.scala @@ -217,7 +217,7 @@ abstract class SymbolPairs { bs(nshifted) |= nmask } - /** Implements `bs1 * bs2 * {0..n} != 0. + /** Implements `bs1 * bs2 * {0..n} != 0`. * Used in hasCommonParentAsSubclass */ private def intersectionContainsElementLeq(bs1: BitSet, bs2: BitSet, n: Int): Boolean = { val nshifted = n >> 5 diff --git a/src/reflect/scala/reflect/internal/TreeGen.scala b/src/reflect/scala/reflect/internal/TreeGen.scala index b2248ad518..201b727ed6 100644 --- a/src/reflect/scala/reflect/internal/TreeGen.scala +++ b/src/reflect/scala/reflect/internal/TreeGen.scala @@ -594,13 +594,12 @@ abstract class TreeGen { * TupleN(x_1, ..., x_N) * } ...) * - * If any of the P_i are variable patterns, the corresponding `x_i @ P_i' is not generated + * If any of the P_i are variable patterns, the corresponding `x_i @ P_i` is not generated * and the variable constituting P_i is used instead of x_i * - * @param mapName The name to be used for maps (either map or foreach) - * @param flatMapName The name to be used for flatMaps (either flatMap or foreach) * @param enums The enumerators in the for expression - * @param body The body of the for expression + * @param sugarBody The body of the for expression + * @param fresh A source of new names */ def mkFor(enums: List[Tree], sugarBody: Tree)(implicit fresh: FreshNameCreator): Tree = { val (mapName, flatMapName, body) = sugarBody match { diff --git a/src/reflect/scala/reflect/internal/Types.scala b/src/reflect/scala/reflect/internal/Types.scala index 8c72405c8d..adc2362e88 100644 --- a/src/reflect/scala/reflect/internal/Types.scala +++ b/src/reflect/scala/reflect/internal/Types.scala @@ -732,7 +732,7 @@ trait Types * * `SubstThisAndSymMap` performs a breadth-first map over this type, which meant that * symbol substitution occurred before `ThisType` substitution. Consequently, in substitution - * of a `SingleType(ThisType(`from`), sym), symbols were rebound to `from` rather than `to`. + * of a `SingleType(ThisType(from), sym)`, symbols were rebound to `from` rather than `to`. */ def substThisAndSym(from: Symbol, to: Type, symsFrom: List[Symbol], symsTo: List[Symbol]): Type = if (symsFrom eq symsTo) substThis(from, to) @@ -756,7 +756,7 @@ trait Types /** Apply `f` to each part of this type */ def foreach(f: Type => Unit) { new ForEachTypeTraverser(f).traverse(this) } - /** Apply `pf' to each part of this type on which the function is defined */ + /** Apply `pf` to each part of this type on which the function is defined */ def collect[T](pf: PartialFunction[Type, T]): List[T] = new CollectTypeCollector(pf).collect(this) /** Apply `f` to each part of this type; children get mapped before their parents */ @@ -2045,7 +2045,7 @@ trait Types /** SI-3731, SI-8177: when prefix is changed to `newPre`, maintain consistency of prefix and sym * (where the symbol refers to a declaration "embedded" in the prefix). * - * @returns newSym so that `newPre` binds `sym.name` to `newSym`, + * @return newSym so that `newPre` binds `sym.name` to `newSym`, * to remain consistent with `pre` previously binding `sym.name` to `sym`. * * `newSym` and `sym` are conceptually the same symbols, but some change to our `prefix` diff --git a/src/reflect/scala/reflect/internal/pickling/UnPickler.scala b/src/reflect/scala/reflect/internal/pickling/UnPickler.scala index 1f643b2b23..c40f5be476 100644 --- a/src/reflect/scala/reflect/internal/pickling/UnPickler.scala +++ b/src/reflect/scala/reflect/internal/pickling/UnPickler.scala @@ -168,7 +168,7 @@ abstract class UnPickler { } /** If entry at `i` is undefined, define it by performing - * operation `op` with `readIndex at start of i'th + * operation `op` with `readIndex` at start of i'th * entry. Restore `readIndex` afterwards. */ protected def at[T <: AnyRef](i: Int, op: () => T): T = { diff --git a/src/reflect/scala/reflect/internal/util/AbstractFileClassLoader.scala b/src/reflect/scala/reflect/internal/util/AbstractFileClassLoader.scala index 30dcbc21ca..5cbdb92664 100644 --- a/src/reflect/scala/reflect/internal/util/AbstractFileClassLoader.scala +++ b/src/reflect/scala/reflect/internal/util/AbstractFileClassLoader.scala @@ -12,7 +12,7 @@ import java.security.cert.Certificate import java.security.{ ProtectionDomain, CodeSource } import java.util.{ Collections => JCollections, Enumeration => JEnumeration } -/** A class loader that loads files from a {@link scala.tools.nsc.io.AbstractFile}. +/** A class loader that loads files from a [[scala.reflect.io.AbstractFile]]. * * @author Lex Spoon */ -- cgit v1.2.3 From ec95e534a213a6ea760aa31c507d122ce449890a Mon Sep 17 00:00:00 2001 From: Jason Zaugg Date: Wed, 29 Jul 2015 13:29:57 +1000 Subject: SI-9422 Fix incorrect constant propagation The ConstantOptimization phase uses abstract interpretation to track what is known about values, and then to use this information to optimize away tests with a statically known result. Constant propagation was added under -optimize in Scala 2.11.0-M3, in PR #2214. For example, we might know that a variable must hold one of a set of values (`Possible`). Or, we could track that it must *not* be of of a set of value (`Impossible`). The test case in the bug report was enough to create comparison: v1 == v2 // where V1 = Possible(Set(true, false)) // V2 = Possible(Set(true, false)) This test was considered to be always true, due to a bug in `Possible#mightNotEqual`. The only time we can be sure that `Possible(p1) mightNotEquals Possible(p2)` is if `p1` and `p2` are the same singleton set. This commit changes this method to implement this logic. The starting assumption for all values is currently `Impossible(Set())`, although it would also be reasonable to represent an unknown boolean variable as `Possible(Set(true, false))`, given the finite and small domain. I tried to change the starting assumption for boolean locals in exactly this manner, and that brings the bug into sharp focus. Under this patch: https://github.com/retronym/scala/commit/e564fe522d This code: def test(a: Boolean, b: Boolean) = a == b Compiles to: public boolean test(boolean, boolean); Code: 0: iconst_1 1: ireturn Note: the enclosed test case does not list `-optimize` in a `.flags` file, I'm relying on that being passed in by the validation build. I've tested locally with that option, though. --- .../scala/tools/nsc/backend/opt/ConstantOptimization.scala | 8 +++++--- test/files/run/t9422.scala | 11 +++++++++++ 2 files changed, 16 insertions(+), 3 deletions(-) create mode 100644 test/files/run/t9422.scala diff --git a/src/compiler/scala/tools/nsc/backend/opt/ConstantOptimization.scala b/src/compiler/scala/tools/nsc/backend/opt/ConstantOptimization.scala index 0e6ee76eb2..fb1799e092 100644 --- a/src/compiler/scala/tools/nsc/backend/opt/ConstantOptimization.scala +++ b/src/compiler/scala/tools/nsc/backend/opt/ConstantOptimization.scala @@ -170,9 +170,11 @@ abstract class ConstantOptimization extends SubComponent { // out all the possibilities case Impossible(possible2) => (possible -- possible2).nonEmpty }) - def mightNotEqual(other: Contents): Boolean = (this ne other) && (other match { - // two Possibles might not be equal if either has possible members that the other doesn't - case Possible(possible2) => (possible -- possible2).nonEmpty || (possible2 -- possible).nonEmpty + def mightNotEqual(other: Contents): Boolean = (other match { + case Possible(possible2) => + // two Possibles must equal if each is known to be of the same, single value + val mustEqual = possible.size == 1 && possible == possible2 + !mustEqual case Impossible(_) => true }) } diff --git a/test/files/run/t9422.scala b/test/files/run/t9422.scala new file mode 100644 index 0000000000..5ca2e8daaa --- /dev/null +++ b/test/files/run/t9422.scala @@ -0,0 +1,11 @@ +class Test(val x: Long) { + def sameDirection(y: Long): Boolean = + (y == 0 || x == 0 || ((y > 0) == (x > 0))) +} + +object Test { + def main(args: Array[String]) { + val b = new Test(1L) + assert(!b.sameDirection(-1L)) + } +} -- cgit v1.2.3 From 58ae3e51f7d0ac0f4a56deaab5f90ab5048350fe Mon Sep 17 00:00:00 2001 From: Janek Bogucki Date: Tue, 4 Aug 2015 08:45:58 +0100 Subject: Delegate null test to Option Option(null) is None while Option(v) is Some(v) which makes the null test redundant. --- .../scala/collection/convert/Wrappers.scala | 41 +++++----------------- src/library/scala/ref/WeakReference.scala | 5 +-- .../scala/reflect/runtime/TwoWayCache.scala | 3 +- .../scala/reflect/runtime/TwoWayCaches.scala | 3 +- 4 files changed, 11 insertions(+), 41 deletions(-) diff --git a/src/library/scala/collection/convert/Wrappers.scala b/src/library/scala/collection/convert/Wrappers.scala index 9f9732c62f..e829a0215b 100644 --- a/src/library/scala/collection/convert/Wrappers.scala +++ b/src/library/scala/collection/convert/Wrappers.scala @@ -265,17 +265,11 @@ private[collection] trait Wrappers { def +=(kv: (A, B)): this.type = { underlying.put(kv._1, kv._2); this } def -=(key: A): this.type = { underlying remove key; this } - override def put(k: A, v: B): Option[B] = { - val r = underlying.put(k, v) - if (r != null) Some(r) else None - } + override def put(k: A, v: B): Option[B] = Option(underlying.put(k, v)) override def update(k: A, v: B) { underlying.put(k, v) } - override def remove(k: A): Option[B] = { - val r = underlying remove k - if (r != null) Some(r) else None - } + override def remove(k: A): Option[B] = Option(underlying remove k) def iterator: Iterator[(A, B)] = new AbstractIterator[(A, B)] { val ui = underlying.entrySet.iterator @@ -326,25 +320,15 @@ private[collection] trait Wrappers { * are not guaranteed to be atomic. */ case class JConcurrentMapWrapper[A, B](underlying: juc.ConcurrentMap[A, B]) extends mutable.AbstractMap[A, B] with JMapWrapperLike[A, B, JConcurrentMapWrapper[A, B]] with concurrent.Map[A, B] { - override def get(k: A) = { - val v = underlying get k - if (v != null) Some(v) - else None - } + override def get(k: A) = Option(underlying get k) override def empty = new JConcurrentMapWrapper(new juc.ConcurrentHashMap[A, B]) - def putIfAbsent(k: A, v: B): Option[B] = { - val r = underlying.putIfAbsent(k, v) - if (r != null) Some(r) else None - } + def putIfAbsent(k: A, v: B): Option[B] = Option(underlying.putIfAbsent(k, v)) def remove(k: A, v: B): Boolean = underlying.remove(k, v) - def replace(k: A, v: B): Option[B] = { - val prev = underlying.replace(k, v) - if (prev != null) Some(prev) else None - } + def replace(k: A, v: B): Option[B] = Option(underlying.replace(k, v)) def replace(k: A, oldvalue: B, newvalue: B): Boolean = underlying.replace(k, oldvalue, newvalue) @@ -380,25 +364,16 @@ private[collection] trait Wrappers { case class JDictionaryWrapper[A, B](underlying: ju.Dictionary[A, B]) extends mutable.AbstractMap[A, B] with mutable.Map[A, B] { override def size: Int = underlying.size - def get(k: A) = { - val v = underlying get k - if (v != null) Some(v) else None - } + def get(k: A) = Option(underlying get k) def +=(kv: (A, B)): this.type = { underlying.put(kv._1, kv._2); this } def -=(key: A): this.type = { underlying remove key; this } - override def put(k: A, v: B): Option[B] = { - val r = underlying.put(k, v) - if (r != null) Some(r) else None - } + override def put(k: A, v: B): Option[B] = Option(underlying.put(k, v)) override def update(k: A, v: B) { underlying.put(k, v) } - override def remove(k: A): Option[B] = { - val r = underlying remove k - if (r != null) Some(r) else None - } + override def remove(k: A): Option[B] = Option(underlying remove k) def iterator = enumerationAsScalaIterator(underlying.keys) map (k => (k, underlying get k)) diff --git a/src/library/scala/ref/WeakReference.scala b/src/library/scala/ref/WeakReference.scala index 6ee40aed5c..9dcc0bbe5f 100644 --- a/src/library/scala/ref/WeakReference.scala +++ b/src/library/scala/ref/WeakReference.scala @@ -28,10 +28,7 @@ object WeakReference { def apply[T <: AnyRef](value: T) = new WeakReference(value) /** Optionally returns the referenced value, or `None` if that value no longer exists */ - def unapply[T <: AnyRef](wr: WeakReference[T]): Option[T] = { - val x = wr.underlying.get - if (x != null) Some(x) else None - } + def unapply[T <: AnyRef](wr: WeakReference[T]): Option[T] = Option(wr.underlying.get) } /** diff --git a/src/reflect/scala/reflect/runtime/TwoWayCache.scala b/src/reflect/scala/reflect/runtime/TwoWayCache.scala index d0fc3dac74..6c1ca5b571 100644 --- a/src/reflect/scala/reflect/runtime/TwoWayCache.scala +++ b/src/reflect/scala/reflect/runtime/TwoWayCache.scala @@ -26,8 +26,7 @@ private[runtime] class TwoWayCache[J, S] { private object SomeRef { def unapply[T](optRef: Option[WeakReference[T]]): Option[T] = if (optRef.nonEmpty) { - val result = optRef.get.get - if (result != null) Some(result) else None + Option(optRef.get.get) } else None } diff --git a/src/reflect/scala/reflect/runtime/TwoWayCaches.scala b/src/reflect/scala/reflect/runtime/TwoWayCaches.scala index 6e2890e536..6ce0c0a728 100644 --- a/src/reflect/scala/reflect/runtime/TwoWayCaches.scala +++ b/src/reflect/scala/reflect/runtime/TwoWayCaches.scala @@ -26,8 +26,7 @@ private[runtime] trait TwoWayCaches { self: SymbolTable => private object SomeRef { def unapply[T](optRef: Option[WeakReference[T]]): Option[T] = if (optRef.nonEmpty) { - val result = optRef.get.get - if (result != null) Some(result) else None + Option(optRef.get.get) } else None } -- cgit v1.2.3 From b763dbf3683793fe07d6b573d088149423e337ac Mon Sep 17 00:00:00 2001 From: Adriaan Moors Date: Tue, 4 Aug 2015 14:52:12 -0700 Subject: Include jline on quick.bin tool path --- build.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/build.xml b/build.xml index ed4f3114d7..f568688c4e 100755 --- a/build.xml +++ b/build.xml @@ -842,7 +842,7 @@ TODO: - + -- cgit v1.2.3