-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix #23224: Optimize simple tuple extraction #23373
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
6e466de
Optimize simple tuple extraction to avoid creating unnecessary tuple …
noti0na1 0463b76
Fix patterns containing wildcards; enhance test
noti0na1 6841cd1
Try to optimize typed vars
noti0na1 74e2b76
Remove typed vars logic
noti0na1 310e20b
Enhance forallResults to handle Try
noti0na1 5f79048
Add special handling when the selector of a pattern matching has Noth…
noti0na1 568b115
Apply change to all bottom types
noti0na1 545dd69
Add bytecode test to verify absence of Tuple2.apply calls
noti0na1 d614d0b
Remove debug comment
noti0na1 82c068c
Enhance bytecode test to verify absence of NEW instruction in method f1
noti0na1 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,9 @@ | ||
-- [E007] Type Mismatch Error: tests/neg/i7294.scala:7:15 -------------------------------------------------------------- | ||
-- [E007] Type Mismatch Error: tests/neg/i7294.scala:7:18 -------------------------------------------------------------- | ||
7 | case x: T => x.g(10) // error | ||
| ^ | ||
| Found: (x : Nothing) | ||
| Required: ?{ g: ? } | ||
| Note that implicit conversions were not tried because the result of an implicit conversion | ||
| must be more specific than ?{ g: [applied to (10) returning T] } | ||
| ^^^^^^^ | ||
| Found: Any | ||
| Required: T | ||
| | ||
| where: T is a type in given instance f with bounds <: foo.Foo | ||
| | ||
| longer explanation available when compiling with `-explain` |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,43 @@ | ||
|
||
class Test: | ||
noti0na1 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
def f1: (Int, String, AnyRef) = (1, "2", "3") | ||
def f2: (x: Int, y: String) = (0, "y") | ||
|
||
def test1 = | ||
val (a, b, c) = f1 | ||
// Desugared to: | ||
// val $2$: (Int, String, AnyRef) = | ||
// this.f1:(Int, String, AnyRef) @unchecked match | ||
// { | ||
// case $1$ @ Tuple3.unapply[Int, String, Object](_, _, _) => | ||
// $1$:(Int, String, AnyRef) | ||
// } | ||
// val a: Int = $2$._1 | ||
// val b: String = $2$._2 | ||
// val c: AnyRef = $2$._3 | ||
a + b.length() + c.toString.length() | ||
|
||
// This pattern will not be optimized: | ||
// val (a1, b1, c1: String) = f1 | ||
|
||
def test2 = | ||
val (_, b, c) = f1 | ||
b.length() + c.toString.length() | ||
|
||
val (a2, _, c2) = f1 | ||
a2 + c2.toString.length() | ||
|
||
val (a3, _, _) = f1 | ||
a3 + 1 | ||
|
||
def test3 = | ||
val (_, b, _) = f1 | ||
b.length() + 1 | ||
|
||
def test4 = | ||
val (x, y) = f2 | ||
x + y.length() | ||
|
||
def test5 = | ||
val (_, b) = f2 | ||
b.length() + 1 |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you comment on why the bottom-related changes are necessary? I find them suspicious. This change should be nothing but an optimization, so it shouldn't change typing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have tests like:
val (a, b) = ???
. Since we now bind the tuple pattern to a variable, I want to give the bind variable a tuple type instead of a bottom type.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that necessary for correctness? There is really no point in improving the code for such an extraction. By definition, the code will throw before it gets to the tuple extraction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is just a small rule change to bind variable with nothing selector, which is necessary for this PR, and I think it is also useful in general for "holes" (to avoid compiling error).