Skip to content
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

8244231: [lworld] Add support for ref-default and val-default inline classes. #482

Closed
wants to merge 9 commits into from

Conversation

sadayapalam
Copy link
Collaborator

@sadayapalam sadayapalam commented Jul 16, 2021

Code changes and tests


Progress

  • Change must not contain extraneous whitespace

Issue

  • JDK-8244231: [lworld] Add support for ref-default and val-default inline classes.

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/valhalla pull/482/head:pull/482
$ git checkout pull/482

Update a local copy of the PR:
$ git checkout pull/482
$ git pull https://git.openjdk.java.net/valhalla pull/482/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 482

View PR using the GUI difftool:
$ git pr show -t 482

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/valhalla/pull/482.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jul 16, 2021

👋 Welcome back sadayapalam! A progress list of the required criteria for merging this PR into lworld will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

@openjdk openjdk bot commented Jul 16, 2021

@sadayapalam This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8244231: [lworld] Add support for ref-default and val-default inline classes.

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 no new commits pushed to the lworld branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the lworld branch, type /integrate in a new comment.

*/
public Type valueProjection() {
public ClassType asValueType() {
Copy link
Collaborator Author

@sadayapalam sadayapalam Jul 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These various internal APIs are still crystallizing. I have a follow up ticket JDK-8268734 to evolve them to final form.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Jul 16, 2021

Webrevs

@sadayapalam
Copy link
Collaborator Author

@sadayapalam sadayapalam commented Jul 16, 2021

Recommended order of review:

Flags.java
JavacParser.java
Type.java
ClassWriter.java
ClassReader.java
Attr.java

then other files.

This order in review provide a good understanding of the work flow.

@sadayapalam
Copy link
Collaborator Author

@sadayapalam sadayapalam commented Jul 16, 2021

(Although the title says add support for val-default classes, this is all about ref-defaul classes. The plain primitive classes we have been supporting all the while are all val-default)

@@ -1212,7 +1211,8 @@ public Boolean visitClassType(ClassType t, Type s) {
// If t is an intersection, sup might not be a class type
if (!sup.hasTag(CLASS)) return isSubtypeNoCapture(sup, s);
return sup.tsym == s.tsym
&& (t.tsym != s.tsym || t.isReferenceProjection() == s.isReferenceProjection())
&& (t.tsym != s.tsym ||
(t.isReferenceProjection() == s.isReferenceProjection() && t.isValueProjection() == s.isValueProjection()))
// Check type variable containment
Copy link
Collaborator Author

@sadayapalam sadayapalam Jul 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check should really be comparing that t.flavor == s.flavor. But this is causing some issues at the moment and will be followed up in JDK-8270882

@@ -1459,14 +1459,15 @@ public Boolean visitClassType(ClassType t, Type s) {
}
return t.tsym == s.tsym
&& t.isReferenceProjection() == s.isReferenceProjection()
&& t.isValueProjection() == s.isValueProjection()
&& visit(getEnclosingType(t), getEnclosingType(s))
Copy link
Collaborator Author

@sadayapalam sadayapalam Jul 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likewise this should be checking that t.flavor == s.flavor. Will be followed up in JDK-8270882

if (firstExplicitBound.hasTag(CLASS))
wantedFlavor = firstExplicitBound.getFlavor();
// Todo: Handle Type variable case.
}
Copy link
Collaborator Author

@sadayapalam sadayapalam Jul 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This TODO is captured as a follow up task in JDK-8270882

@@ -984,9 +984,6 @@ public void visitClassDef(JCClassDecl tree) {
env.tree.hasTag(NEWCLASS)) {
c.flags_field |= NOOUTERTHIS;
}
if (env.tree.hasTag(NEWCLASS) && types.isPrimitiveClass(c.getSuperclass())) {
c.flags_field |= PRIMITIVE_CLASS; // avoid further secondary errors.
}
attribClass(tree.pos(), c);
Copy link
Collaborator Author

@sadayapalam sadayapalam Jul 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This chunk purportedly introduced to serve the purpose of "avoiding further secondary errors" had outlived its purpose and is removed now. This change is not "essential" to the overall implementation and is a incidental cleanup

@@ -60,7 +61,7 @@
public static final int ACC_MANDATED = 0x8000; // method parameter
public static final int ACC_MODULE = 0x8000; // class

public static enum Kind { Class, InnerClass, Field, Method}
public static enum Kind { Class, InnerClass, Field, Method, JavacExtra}

Copy link
Collaborator Author

@sadayapalam sadayapalam Jul 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an half hearted attempt to find a place for ACC_REF_DEFAULT - I don't think it belongs here really since it is not a part of the class flags. But I have stuck this in here for now - In javap code to reference ACC_REF_DEFAULT symbolically this is useful (since the compiler's definition from Flags.java is not visible there in javap). To be cleaned up in JDK-8270882

@@ -48,6 +48,7 @@
public static final String EnclosingMethod = "EnclosingMethod";
public static final String Exceptions = "Exceptions";
public static final String InnerClasses = "InnerClasses";
public static final String JavaFlags = "JavaFlags";
public static final String LineNumberTable = "LineNumberTable";
Copy link
Collaborator Author

@sadayapalam sadayapalam Jul 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name and attribute syntax was suggested by Dan, this may incur/undergo some bikeshedding later.

indent(+1);
if ((attr.extendedFlags & ACC_REF_DEFAULT) != 0)
println("ACC_REF_DEFAULT");
indent(-1);
Copy link
Collaborator Author

@sadayapalam sadayapalam Jul 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

com.sun.tools.javac.code.Flags#ACC_REF_DEFAULT is not visible here. So com.sun.tools.classfile.AccessFlags has been enhanced to define ACC_REF_DEFAULT - but this is a stop gap

CheckFinal.java:19:42: compiler.err.concrete.supertype.for.primitive.class: compiler.misc.anonymous.class: CheckFinal$1, CheckFinal
CheckFinal.java:19:25: compiler.err.prob.found.req: (compiler.misc.inconvertible.types: compiler.misc.anonymous.class: CheckFinal, CheckFinal)
6 errors
4 errors
Copy link
Collaborator Author

@sadayapalam sadayapalam Jul 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The (now deleted) attempt to avoid further secondary errors in Attr.java having outlived its purpose was actually causing additional errors to be emitted! Fixed.

@sadayapalam
Copy link
Collaborator Author

@sadayapalam sadayapalam commented Jul 19, 2021

I have annotated the code changes with hopefully helpful comments that will help the reviewer.
https://bugs.openjdk.java.net/browse/JDK-8270882 has various tasks deliberately deferred.

Copy link
Collaborator

@mcimadamore mcimadamore left a comment

Overall the changes look solid. My main note is a naming gripe - e.g. Type::isPrimitiveClass is now no longer expressive - and it is outright confusing. I've left suggestions for possible alternate names which I think sit well with where the code is going.

I'm also a bit unsure (and left comment) on whether isReferenceProjection and isValueProjection are implemented correctly. They do not seem to be the dual of each other at the moment (or at least do not seem to do what the javadoc says they should).

Copy link
Collaborator

@mcimadamore mcimadamore left a comment

Added some more comments

if (owntype.isPrimitiveClass() && tree.hasTag(SELECT) && ((JCFieldAccess) tree).name == names.ref) {
owntype = new ClassType(owntype.getEnclosingType(), owntype.getTypeArguments(), (TypeSymbol)sym, owntype.getMetadata(), Flavor.L_TypeOf_Q);
if (sym.isPrimitiveClass()) {
if (sym.isReferenceFavoringPrimitiveClass()) {
Copy link
Collaborator

@mcimadamore mcimadamore Jul 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we simplify this to try and use asValueType - probably this suggests there should be an asReferenceType too?

Copy link
Collaborator Author

@sadayapalam sadayapalam Jul 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ATM, we have com.sun.tools.javac.code.Type#referenceProjection()

Again, I will use JDK-8268734 to rationalize the various internal APIs we have - while doing so, also looking at choice of terminology adopted by java.lang.Class

But I didn't understand the comment about the simplification you are suggesting - can you clarify ?

Copy link
Collaborator

@mcimadamore mcimadamore Jul 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean - this code does a bunch of checks - and then creates a new class type with the opposite flavor/polarity. Isn't that what methods like Type::asValueType are supposed to do? E.g. if I have a reference-favoring primitive and I see .val can't I just get the type I want by calling asValueType() on it? If so, should we also have the dual?

Copy link
Collaborator Author

@sadayapalam sadayapalam Jul 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean - this code does a bunch of checks - and then creates a new class type with the opposite flavor/polarity. Isn't that what methods like Type::asValueType are supposed to do? E.g. if I have a reference-favoring primitive and I see .val can't I just get the type I want by calling asValueType() on it? If so, should we also have the dual?

OK, understand your point and agree that this could avoid some code duplication, Will follow up. Thanks!

@sadayapalam
Copy link
Collaborator Author

@sadayapalam sadayapalam commented Jul 23, 2021

Thanks Maurizio!

The confusing and unhelpful javadoc issues have been fixed.

In JDK-8268734 ("Various internal Type/Symbol APIs need rationalization/adjustment") I will follow up with a comprehensive review of all the Type and Symbol APIs - these APIs were rolled up as and when needed at widely different points in time
and it is an useful exercise to take a step back and review them in a holistic fashion.

In JDK-8270882 (" Loose ends in reference default primitive class implementation.") I will follow up on the various known
issues that have been deliberately deferred to a later pass so as to make it more easily manageable,

@sadayapalam
Copy link
Collaborator Author

@sadayapalam sadayapalam commented Jul 23, 2021

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Jul 23, 2021

Going to push as commit a4066d7.

@openjdk openjdk bot closed this Jul 23, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Jul 23, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jul 23, 2021

@sadayapalam Pushed as commit a4066d7.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@sadayapalam sadayapalam deleted the JDK-8244231 branch Jul 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants