Fix handling of arrays in java annotation arguments #8360
Merged
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
smarter
force-pushed
the
fix-array-annot
branch
from
97b1149 to
9eb6b7b
Compare
odersky
requested changes
Feb 29, 2020
compiler/src/dotty/tools/dotc/core/classfile/ClassfileParser.scala
Outdated
Show resolved
Hide resolved
It's not necessary and doing an implicit search after typer does not sound like a good idea.
Previously, the array literal argument `{ "foo", "bar" }` was typed as
an `Array["foo" | "bar"]`, which is not a subtype of the corresponding
parameter type (`Array[String]`). This lead to a crash when forcing
the annotation. This is a serious problem because in Java 9+, JFrame
contains such an annotation, therefore this affects basically any code
using Swing/AWT.
Similary an empty array `{}` was always typed as `Array[Object]` which
does not always match the parameter type, this one is trickier to fix
since the serialized annotation does not keep track of the element type
of an empty array and we don't want to force the serialization at this
point, so we use a WildcardType.
smarter
force-pushed
the
fix-array-annot
branch
from
9eb6b7b to
8dcc774
Compare
|
I've implemented an alternative, cleaner fix: storing annotation arguments as untyped trees. See latest commit. |
We can't properly type annotation arguments which are empty array literals because we would need the corresponding parameter type which would require forcing the annotation constructor early. The previous commit worked around this by using `WildcardType` as the element type but that's a hack. Instead, we can fix the problem by always storing annotation arguments as untyped trees and using `applyOverloaded` to type them when the annotation tree is forced. This requires some refactoring to make `applyOverloaded` work both with typed and untyped arguments (the variants are now accessible using `untpd.applyOverloaded` and `tpd.applyOverloaded`). (the abstract `def FunProto` in Trees that gets implemented by forwarding in untpd/tpd sounds like a good candidate for `export`, except we can't export a class constructor)
smarter
force-pushed
the
fix-array-annot
branch
from
8dcc774 to
610450f
Compare
odersky
requested changes
Mar 22, 2020
| .map(denot => TermRef(receiver.tpe, denot.symbol)) | ||
| .filter(tr => typeParamCount(tr) == targs.length) | ||
| .filter { _.widen match { | ||
| case MethodTpe(_, _, x: MethodType) => !x.isImplicitMethod |
There was a problem hiding this comment.
Why drop only implicit methods that are followed by some other method type? Should this not be done for all implicit methods?
There was a problem hiding this comment.
Note that I didn't write this code, I just moved it from tpd.scala That said, this drops methods whose result type is an implicit method, e.g. if you have def foo(x: A) and def foo(x: A)(implicit y: B), it will pick the first alternative, which seems reasonable to me.
| case MethodTpe(_, _, x: MethodType) => !x.isImplicitMethod | ||
| case _ => true | ||
| }} | ||
| if (targs.isEmpty) allAlts = allAlts.filterNot(_.widen.isInstanceOf[PolyType]) |
There was a problem hiding this comment.
This line looks redundant to me. PolyTypes were already filtered out in line 1537.
There was a problem hiding this comment.
I'll add a follow-up commit to remove the redundant filtering.
Already taken care of by the `typeParamCount(tr) == targs.length` filtering done above.
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.
Also turn off implicit search in applyOverloaded, see commit messages for details.