More conservative optimization for unnecessary outer ref checks #4974
Conversation
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
|
/cc @adriaanm |
|
Sorry, I'm really deep in INDY Sammy land. Will emerge before I fly to Old Country land (Wed) with feedback. |
| // if there's an outer accessor, otherwise the condition becomes `true` -- TODO: can we improve needsOuterTest so there's always an outerAccessor? | ||
| val outer = expectedTp.typeSymbol.newMethod(vpmName.outer, newFlags = SYNTHETIC | ARTIFACT) setInfo expectedTp.prefix | ||
|
|
||
| (Select(codegen._asInstanceOf(testedBinder, expectedTp), outer)) OBJ_EQ expectedOuter |
There was a problem hiding this comment.
Are there well-typed matches where a statically known expected type could never match (and thus the test is constant false instead of true). I can't think of any, but it's worth exploring.
There was a problem hiding this comment.
I can't think of any, either, and I think there should not be any in principle. Such a match could never succeed, so if we can detect it at compile-time, it should fail to compile in the first place.
|
Added the test case that @lrytz ran into yesterday |
szeiger
force-pushed
the
wip/patmat-outertest
branch
from
6113021 to
a0a100e
Compare
szeiger
changed the title
|
Squashed and cleaned up. This should be good to go. |
The old algorithm omitted necessary outer ref checks in some places. This new one is more conservative. It only omits outer ref checks when the expected type and the scrutinee type match up, or when the expected type is defined in a static location. For this specific purpose the top level of a method or other code block (which is not a trait or class definition) is also considered static because it does not have a prefix. This change comes with a spec update to clarify the prefix rule for type patterns. The new wording makes it clear that the presence of a prefix is to be interpreted in a *semantic* way, i.e. the existence of a prefix determines the necessity for an outer ref check, no matter if the prefix is actually spelled out *syntactically*. Note that the old outer ref check implementation did not use the alternative interpretation of requiring prefixes to be given syntactically. It never created an outer ref check for a local class `C`, no matter if the pattern was `_: C` or `_: this.C`, thus violating both interpretations of the spec. There is now explicit support for unchecked matches (like `case _: (T @unchecked) =>`) to suppress warnings for unchecked outer refs. `@unchecked` worked before and was used for this purpose in `neg/t7721` but never actually existed as a feature. It was a result of a bug that prevented an outer ref check from being generated in the first place if *any* annotation was used on an expected type in a type pattern. This new version will still generate the outer ref check if an outer ref is available but suppress the warning otherwise. Other annotations on type patterns are ignored. New tests are in `neg/outer-ref-checks`. The expected results of tests `neg/t7171` and `neg/t7171b` have changed because the compiler now tries to generate additional outer ref checks that were not present before (which was a bug).
szeiger
force-pushed
the
wip/patmat-outertest
branch
from
a0a100e to
e7e1c77
Compare
|
LGTM -- good stuff, @szeiger |
adriaanm
added a commit
that referenced
this pull request
Mar 15, 2016
More conservative optimization for unnecessary outer ref checks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Outer ref checks in pattern matches are elided if the tested prefix is the same as the expected prefix or the expected type is defined in a "static" location (i.e. directly in a package or inside a chain of nested objects).
The
@uncheckedannotation can be used to suppress the warning when an outer ref check is required but no outer ref is available (like for nested final classes).Includes a spec update to clarify the use of prefixes in type patterns.
For review only. This needs to be squashed before merging.