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
Make "PrimitiveParameterizedClass.default" a poly expression. #369
base: lworld
Are you sure you want to change the base?
Conversation
|
@jespersm This change now passes all automated pre-integration checks. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 20 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@mcimadamore) but any other Committer may sponsor as well.
|
Webrevs
|
Nice work - it's a very nice cleanup of the code base! I like how the code that was previously scattered in several places now can just move where it belongs.
Functionality-wise, I'm not sure this supports poly expressions yet - e.g. I see that you set the polyKind tag correctly, but I'm afraid that this along is not sufficient to support stuff like:
List<String> ls = List.default;
or:
void m(List<String> ls) { ... }
m(List.default)
I think you might be led to believe that the current impl is doing the right thing - but my feeling is that javac might be tricking you: if the expected type is e.g. List<String>
but the qualifier of the default expression is List
, the compiler will check (as per checkId) that List <: List - which is true by subtyping conversion. I don't think there is any inference going on, in other words. You might try debugging and looking at the type of the expression that comes out of checkId.
Anyway, even if this works and I did miss something, I don't see how this approach can scale to default expression passed as method arguments - given that in that case there is no pt()
. To add support for that you need to add another case/node in ArgumentAttr
(see what's been done for anonymous inner classes - this should be simpler as there's no constructor call).
In other words, you want Attr.visitApply to treat your default expression method argument in a special way - by creating what we call a "deferred type" - that is a type that is not fully inferred until after overload resolution (when you DO know the pt()).
One possibility would be to push this change as is - as the refactoring part is really good - and maybe issue raw type warning when generic types are involved, or when default expressions are used in method context. And then in another, separate PR we can refine what happens in truly poly cases.
So, I debugged what happens in case where we have
What I missed is that we create a variable symbol for Counter example:
This example is more complex, for two reasons:
All this stuff needs to work correctly if we want to claim that default expressions are true poly expressions. |
Mailing list message from Jesper Steen M��ller on valhalla-dev: On 19 Mar 2021, at 13.28, Maurizio Cimadamore <mcimadamore at openjdk.java.net> wrote:
Yes, that might make sense. I?ll split the PR up into the two cases.
I get it; this needs to be checked before blindly promoting to the expected type. As for method overload, I?ve come up with this example, which seems to demonstrate the need for the "full poly treatment" using deferred types. class Whatever {
} -Jesper |
@mcimadamore : I've opened https://bugs.openjdk.java.net/browse/JDK-8263900 for the refactoring alone. I'm not totally confident that all the checks in Attr are syntactically possible, and should likely be turned into assertions. So this PR is temporarily on hold, awaiting merge of #370 |
… synthetic method.
0f969f6
to
e9289cf
Compare
/test tier1 |
Hello Jesper, this latest patch is problematic. it does not properly treat Foo.default as a poly expression. As Maurizio's earlier review comments points out:
I don't see the relevant code changes in your change set - i.e there is no changes in ArgumentAttr {} - I would have expected to see a visitDefaultValue method there. As Maurizio further pointed out earlier, in Attr.visitApply, right after arguments are attributed, the type of Foo.default as a method argument should be a deferred type . If I debug the uploaded patch, the type of Foo.default is actually Foo<T> The problem is masked by there being only negative tests in your patch. If I compile the following:
at the line with the .default usage I get:
This shows that there is no real inference happening there. (when a poly type argument is attributed without a target type in context, it should be typed to be DeferredType, when considered against overload candidates the positional parameter type becomes the pt() and the deferred type can now be typed to a concrete type) |
A way to see this in more detail is to debug through the Attr.visitApply call corresponding to the method invocation
in this test case:
You will see that first argument is typed to be DeferredType and second one incorrectly to be X<T> |
Hey @sadayapalam and @mcimadamore, I think this patch is ready for review again. I've not merged lworld (to keep a clearer history), but it still applies cleanly to the current lworld tip, so it should merge OK. |
JCTypeApply applyTree = TreeInfo.getTypeApplication(tree.clazz); | ||
if (applyTree != null) { | ||
return applyTree.arguments.isEmpty(); | ||
} else { |
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.
The return statement is not reachable and so the block is not useful. This is because, Foo<>.default is a syntax error. Given a primitive class Foo we will only support Foo.default involving implicit diamond like inference and will not support Foo<>.default which is what this code is handling.
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.
(I think I meant to say return applyTree.arguments.isEmpty(); will never be true.)
} else { | ||
// No type arguments before .default - Consider if the type is generic or not | ||
return clazztype == null || clazztype.tsym.type.isParameterized(); | ||
} |
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.
This method is problematic - This will always treat Foo.default as a poly expression in an invocation context even when the concerned primitive type is not a generic class. For instance the Foo.default expression in the following snippet becomes a poly expression when it is a standalone expression in reality:
primitive class X {
static void foo(Object o) {
}
public static void main(String [] args) {
foo(X.default);
}
}
switch(tree.getTag()) { | ||
case TYPEAPPLY: return (JCTypeApply)tree; | ||
case NEWCLASS: return getTypeApplication(((JCNewClass)tree).clazz); | ||
case ANNOTATED_TYPE: return getTypeApplication(((JCAnnotatedType)tree).underlyingType); |
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.
I think there is a copy + paste problem. We can never see a NEWCLASS here. The LHS of .default is just a type node and cannot be an rvalue expression ==> new X().default is illegal.
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.
I think perhaps what we really need to do here is to attribute the clazz node of JCDefaultValue first - perhaps by first making a copy of the node and then see if it is a generic class but lacks type arguments. Such as this:
@Override
public void visitDefaultValue(JCDefaultValue that) {
JCExpression copy = new TreeCopier<Void>(attr.make).copy(that.clazz);
attr.attribType(copy, env);
if (copy.type.getTypeArguments().isEmpty() && copy.type.tsym.type.isParameterized()) {
processArg(that, speculativeTree -> new ResolvedDefaultType(that, env, speculativeTree));
} else {
//not a poly expression, just call Attr
setResult(that, attr.attribTree(that, env, attr.unknownExprInfo));
}
}
* Argument type for default values. | ||
*/ | ||
class ResolvedDefaultType extends ResolvedMemberType<JCDefaultValue> { | ||
|
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.
I am surprised that ResolvedDefaultType extends ResolvedMemberType rather than ArgumentType directly. While the Inference behavior could be indirectly modelled it as if the class has a generic factory method named 'default':
<T> static Foo<T> default() { ... }
this implementation detail could be pushed deep down and not manifest here and in elsewhere (setPolyKind())
enclosingContext.report(tree.clazz, | ||
diags.fragment(Fragments.CantApplyDiamond1(Fragments.Diamond(tsym), details))); | ||
} | ||
}; |
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.
I don't see a test that triggers this diagnostic.
@@ -294,6 +315,9 @@ public static void setPolyKind(JCTree tree, PolyKind pkind) { | |||
case NEWCLASS: | |||
((JCNewClass)tree).polyKind = pkind; | |||
break; | |||
case DEFAULT_VALUE: | |||
((JCDefaultValue)tree).polyKind = pkind; | |||
break; | |||
case REFERENCE: |
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.
This is surprising. I would expected polyKind to be set in a similar fashion to switch and conditional expressions.
Hi Jesper, I have left some comments - but I will need to make a couple of more passes to fully understand the implementation as this is a fairly involved and complex task. I will actually have to tinker with the version you have posted to comprehend it - Could you give me some time - I will play around with it and propose a version that addresses the problems I have outlined and also includes further tweaks that could simplify the implementation more ? Then we can study it, finalize it and then ask Maurizio for a final review once both of us are satisfied. |
@sadayapalam : I can take another go at this one to adress the review comments above, which may make a second review pass easier. |
Sure, this is recognized to be a tricky area, so it is normal to have iterate over it multiple times incrementally improving it. As long as this is had in mind, by all means it helps to have the comments made so far addressed. Thanks! |
Make .default a separate node type in the parser.
Issue
JDK-8211914: [lworld] Javac should support type inference for default value creation
Note: The Linux x86 builds in GitHub actions seem to fail with something completely unrelated to these changes.
Progress
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/valhalla pull/369/head:pull/369
$ git checkout pull/369
Update a local copy of the PR:
$ git checkout pull/369
$ git pull https://git.openjdk.java.net/valhalla pull/369/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 369
View PR using the GUI difftool:
$ git pr show -t 369
Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/valhalla/pull/369.diff