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

universal type variables: initial prototype #521

Conversation

@vicente-romero-oracle
Copy link
Contributor

@vicente-romero-oracle vicente-romero-oracle commented Aug 4, 2021

First iteration of the protype for universal type variables


Progress

  • Change must not contain extraneous whitespace

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 521

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Aug 4, 2021

👋 Welcome back vromero! A progress list of the required criteria for merging this PR into universal-tvars 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 Aug 4, 2021

@vicente-romero-oracle 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:

universal type variables: initial prototype

Reviewed-by: mcimadamore

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 universal-tvars 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 universal-tvars branch, type /integrate in a new comment.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Aug 4, 2021

Copy link
Collaborator

@mcimadamore mcimadamore left a comment

Overall looks like a solid first attempt. I think we should spend some cycles making sure we get the TypeVar model right, and that we put info (e.g. universality) where it belongs (e.g. symbol), because that can affect how clients interact with these variables quite a lot.

@@ -61,6 +61,7 @@

public enum Feature {
SWITCH_PATTERN_MATCHING,
UNIVERSAL_TVARS,
Copy link
Collaborator

@mcimadamore mcimadamore Aug 4, 2021

Choose a reason for hiding this comment

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

As Dan noted yesterday, for other Valhalla features we do not generally use preview mechanisms (this code needs to be pushed in the valhalla repo). We can postpone the preview dance for later. In fact, to ease experiments in JDK code it would be better to just make universal type variable syntax available by default.

There are some other places where this UNIVERSAL_TVARS constant is used which should be cleaned up for now (e.g. Preview.java).

@@ -1943,12 +1943,34 @@ public TypeKind getKind() {
*/
public Type lower;

public boolean universal = false;
Copy link
Collaborator

@mcimadamore mcimadamore Aug 4, 2021

Choose a reason for hiding this comment

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

Can we use a flag on the tsym for this? After all, being "universal" is a declaration (symbol) not a use (type) property?

Copy link
Contributor Author

@vicente-romero-oracle vicente-romero-oracle Aug 4, 2021

Choose a reason for hiding this comment

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

well yes and no, if we have:

class C<__universal T> {
    T.ref t;
}

T and T.ref share the same symbol but they have different types that's why the universal field has been set to the type

Copy link
Collaborator

@mcimadamore mcimadamore Aug 5, 2021

Choose a reason for hiding this comment

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

yes, but universal is a property of the declaration, while ref-ness is a property of the use-site (hence type). Note that the underlying Valhalla compiler already has machinery to talk about reference projections, so perhaps we should use that to speak about T vs. T.ref, rather than inventing a new one. But we still need a flag somewhere to say whether the variable was declared as universal or not (as that affects what's compatible with that var).

}

@Override
public Type withTypeVar(Type t) {
Copy link
Collaborator

@mcimadamore mcimadamore Aug 4, 2021

Choose a reason for hiding this comment

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

Uhm - isn't this method used on wildcard types, and it all other types is supposed to just return this ?

@@ -2040,6 +2088,8 @@ public CapturedType(Name name,
this.lower = Assert.checkNonNull(lower);
this.setUpperBound(upper);
this.wildcard = wildcard;
this.universal = upper.hasTag(TYPEVAR) && ((TypeVar)upper).universal ||
Copy link
Collaborator

@mcimadamore mcimadamore Aug 4, 2021

Choose a reason for hiding this comment

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

This probably requires tweaking - I see two issues:

  • right now it detects cases where upper/lower are type variables themselves - but doesn't detect cases originating from e.g. List<? extends Point> - where you just have a captured variable whose bound is Point.
  • the logic does not recurse - but maybe that's ok if universal is always set on creation

I think another approach could be to look at the wildcard type associated with the captured type. The wildcard type keeps track of the declared type variable it comes from, which means you can then look at the type symbol in there and decide if it's an universal variable or not. E.g.

captured.isUniversal() == captured.wildcard.bound.isUniversal()

@@ -1051,6 +1086,10 @@ private boolean isSubtypeUncheckedInternal(Type t, Type s, boolean capture, Warn
}
} else if (isSubtype(t, s, capture)) {
return true;
} else if (allowUniversalTVars && t.hasTag(TYPEVAR) && s.hasTag(TYPEVAR) && t.tsym == s.tsym) {
Copy link
Collaborator

@mcimadamore mcimadamore Aug 4, 2021

Choose a reason for hiding this comment

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

Are you sure the check does what the comment says?

Copy link
Contributor Author

@vicente-romero-oracle vicente-romero-oracle Aug 5, 2021

Choose a reason for hiding this comment

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

will remove the comment, probably a self comment I guess

@@ -3555,6 +3612,13 @@ public Type visitTypeVar(TypeVar t, Void ignored) {
if (t.equalsIgnoreMetadata(from.head)) {
return to.head.withTypeVar(t);
}
if (allowUniversalTVars &&
Copy link
Collaborator

@mcimadamore mcimadamore Aug 4, 2021

Choose a reason for hiding this comment

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

Not sure about this - the main issue here seems that t.equalsIgnoreMetadata is too weak and would return false for ref vs.val mismatches. But again I'm uncertain about the need of overriding withTypeVar (see comment above).

Copy link
Contributor Author

@vicente-romero-oracle vicente-romero-oracle Aug 5, 2021

Choose a reason for hiding this comment

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

not sure I follow you here, t.equalsIgnoreMetadata is already being used in the method, not sure I see in what case it can give an incorrect answer

Copy link
Collaborator

@mcimadamore mcimadamore Aug 5, 2021

Choose a reason for hiding this comment

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

What I mean is that the newly added code seems to workaround the fact that in some cases we do not call withTypeVar. I'm saying that because you added another test which, if satisfied, ends up calling to.head.withTypeVar(t); again, which seems to suggest that you found cases where this call was needed, but was not triggered - probably because of a mismatch between T and T.ref?

return attribTypes(trees, env, i -> false);
}

List<Type> attribTypes(List<JCExpression> trees, Env<AttrContext> env, Predicate<JCExpression> valueOK) {
Copy link
Collaborator

@mcimadamore mcimadamore Aug 4, 2021

Choose a reason for hiding this comment

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

Do you really need a predicate here? Wouldn't a boolean be ok? Or perhaps a flag in env, or some other means?

Copy link
Contributor Author

@vicente-romero-oracle vicente-romero-oracle Aug 4, 2021

Choose a reason for hiding this comment

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

the thing is that one value only doesn't apply to all cases, what if some type variables are universal but others aren't?

Copy link
Collaborator

@mcimadamore mcimadamore Aug 5, 2021

Choose a reason for hiding this comment

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

But you have only one predicate? Anyway - see the other related comment - you really need this predicate when you call this later on:

attribTypes(tree.arguments, env, arg -> valueOKSet.contains(arg));

But it feels like the code in there can be simplified so that you use a single attribType inside a loop, by passing the right boolean flag for that parameter.

Copy link
Contributor Author

@vicente-romero-oracle vicente-romero-oracle Aug 6, 2021

Choose a reason for hiding this comment

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

it wont be that simple, see that the method invoked, Attr.attribTypes, is doing:

    List<Type> types = attribAnyTypes(trees, env);
    return chk.checkRefTypes(trees, types, valueOK);

and Check.checkRefTypes is doing:

        List<JCExpression> tl = trees;
        for (List<Type> l = types; l.nonEmpty(); l = l.tail) {
            l.head = checkRefType(tl.head.pos(), l.head, valueOK.test(tl.head));
            tl = tl.tail;
        }
        return types;

and Check.checkRefType is the one using the flag, so in order to do all this at the level of Attr.visitTypeApply I will have to gather all the code, popping up code that belongs in Check, and then the code in Attr.visitTypeApply will lose readability. This is why I opted for the current solution.

Copy link
Collaborator

@mcimadamore mcimadamore Aug 6, 2021

Choose a reason for hiding this comment

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

I see what you mean - it seems like the current code is working against us here. In fact, attribAnyTypes specifically says that clients are expected to check for reference types after the fact...

Here's an idea... you could maybe call attribAnyTypes yourself, to attribute all the types, and then, in the loop you already have, only call checkRefTypes on the types you really need? (e.g. avoid attribTypes)

Copy link
Contributor Author

@vicente-romero-oracle vicente-romero-oracle Aug 6, 2021

Choose a reason for hiding this comment

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

OK


// Attribute type parameters
List<Type> actuals = attribTypes(tree.arguments, env);
List<Type> actuals = !allowUniversalTVars ?
Copy link
Collaborator

@mcimadamore mcimadamore Aug 4, 2021

Choose a reason for hiding this comment

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

If you are doing this, you might be better off just attributing types one at a time inside the loop above, and avoid using a set to collect the ones which need special treatment?

Copy link
Contributor Author

@vicente-romero-oracle vicente-romero-oracle Aug 4, 2021

Choose a reason for hiding this comment

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

I will try that out

@@ -686,7 +704,7 @@ private boolean checkExtends(Type a, Type bound) {
return true;
} else if (!a.hasTag(WILDCARD)) {
a = types.cvarUpperBound(a);
return types.isSubtype(a, bound);
return types.isBoundedBy(a, bound, (t, s, w) -> types.isSubtype(t, s));
Copy link
Collaborator

@mcimadamore mcimadamore Aug 4, 2021

Choose a reason for hiding this comment

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

This is a good change - I'm a bit scared about the other calls in this routine, to isCastable and notSoftSubtype. These type relations are mostly doing things outside the spec, and we probably need to sprinkle isBounded in there as well.

Copy link
Contributor Author

@vicente-romero-oracle vicente-romero-oracle Aug 4, 2021

Choose a reason for hiding this comment

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

ok

@@ -1053,7 +1053,8 @@ public MethodCheckContext(boolean strict, DeferredAttrContext deferredAttrContex
public boolean compatible(Type found, Type req, Warner warn) {
InferenceContext inferenceContext = deferredAttrContext.inferenceContext;
return strict ?
types.isSubtypeUnchecked(inferenceContext.asUndetVar(found), inferenceContext.asUndetVar(req), warn) :
types.isBoundedBy(inferenceContext.asUndetVar(found), inferenceContext.asUndetVar(req), warn,
Copy link
Collaborator

@mcimadamore mcimadamore Aug 4, 2021

Choose a reason for hiding this comment

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

I'm not 100% sure on this. I think step one should use just plain subtyping, while step 2 is allowed to use the more powerful relationship. My mental model is int -> Object which is only allowed on step 2 - so I think we want to do something similar for Point -> Object ? Of course Point.ref -> Object would be allowed on step 1.

if (tv.universal) {
valueOKSet.add(args.head);
}
argTypes.head = chk.checkRefType(args.head.pos(), argTypes.head, ((TypeVar) t).isValueProjection());
Copy link
Collaborator

@mcimadamore mcimadamore Aug 10, 2021

Choose a reason for hiding this comment

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

this should be isUniversal(), no?

Copy link
Contributor Author

@vicente-romero-oracle vicente-romero-oracle Aug 11, 2021

Choose a reason for hiding this comment

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

yep will change it

@@ -4603,15 +4597,14 @@ private Symbol selectSym(JCFieldAccess tree,
case WILDCARD:
throw new AssertionError(tree);
case TYPEVAR:
if (allowUniversalTVars && name == names.ref && ((TypeVar)site).universal) {
if (allowUniversalTVars && name == names.ref && ((TypeVar)site).isValueProjection()) {
Copy link
Collaborator

@mcimadamore mcimadamore Aug 10, 2021

Choose a reason for hiding this comment

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

This should be isUniversal() ?

Copy link
Contributor Author

@vicente-romero-oracle vicente-romero-oracle Aug 11, 2021

Choose a reason for hiding this comment

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

yep u are right

@@ -2666,7 +2660,7 @@ public void visitApply(JCMethodInvocation tree) {
&& (tree.meth.hasTag(SELECT))
&& ((JCFieldAccess)tree.meth).selected.hasTag(IDENT)
&& TreeInfo.name(((JCFieldAccess)tree.meth).selected) == names._super;
boolean qualifierIsUniversal = allowUniversalTVars && qualifier.hasTag(TYPEVAR) && ((TypeVar)qualifier).universal;
boolean qualifierIsUniversal = allowUniversalTVars && qualifier.hasTag(TYPEVAR) && ((TypeVar)qualifier).isValueProjection();
Copy link
Collaborator

@mcimadamore mcimadamore Aug 10, 2021

Choose a reason for hiding this comment

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

This should be isUniversal() I think - wait and friends should warn, even on .ref stuff.

Copy link
Contributor Author

@vicente-romero-oracle vicente-romero-oracle Aug 11, 2021

Choose a reason for hiding this comment

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

not sure to be honest, although there is no reference in the JEP to these warnings so we can do either now

Copy link
Collaborator

@mcimadamore mcimadamore Aug 11, 2021

Choose a reason for hiding this comment

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

I double checked with Brian yesterday - he confirmed that both T and T.ref should not support identity operation (well, given warning when attempting to).

Copy link
Contributor Author

@vicente-romero-oracle vicente-romero-oracle Aug 11, 2021

Choose a reason for hiding this comment

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

ok, thanks for double checking, will fix this

Copy link
Member

@forax forax Aug 11, 2021

Choose a reason for hiding this comment

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

T.ref still support == null, but it's a corner case

@@ -1185,7 +1183,7 @@ public Boolean visitType(Type t, Type s) {
case TYPEVAR:
return isSubtypeNoCapture(t.getUpperBound(), s);
case BOT:
if (allowUniversalTVars && s.hasTag(TYPEVAR) && ((TypeVar)s).universal) {
if (allowUniversalTVars && s.hasTag(TYPEVAR) && ((TypeVar)s).isValueProjection()) {
Copy link
Collaborator

@mcimadamore mcimadamore Aug 10, 2021

Choose a reason for hiding this comment

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

I agree that here you need isValueProjection

from.head.hasTag(TYPEVAR) &&
((TypeVar)from.head).referenceProjection != null &&
t.equalsIgnoreMetadata(((TypeVar)from.head).referenceProjection)) {
((TypeVar)from.head).projection != null &&
Copy link
Collaborator

@mcimadamore mcimadamore Aug 10, 2021

Choose a reason for hiding this comment

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

Shouldn't this test be isUniversal() ?

Copy link
Contributor Author

@vicente-romero-oracle vicente-romero-oracle Aug 11, 2021

Choose a reason for hiding this comment

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

doing this change right now breaks the build, I think it is because we generate reference projections for universal type vars in a lazy way, so some universal type variables could have its projection field set to null

Copy link
Collaborator

@mcimadamore mcimadamore Aug 11, 2021

Choose a reason for hiding this comment

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

ugh - I understand - moving forward it'd be nice not to depend on tricky init semantics.

Copy link
Contributor Author

@vicente-romero-oracle vicente-romero-oracle Aug 11, 2021

Choose a reason for hiding this comment

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

yep, will fix this

((TypeVar)from.head).referenceProjection != null &&
t.equalsIgnoreMetadata(((TypeVar)from.head).referenceProjection)) {
((TypeVar)from.head).projection != null &&
t.equalsIgnoreMetadata(((TypeVar)from.head).projection)) {
Copy link
Collaborator

@mcimadamore mcimadamore Aug 10, 2021

Choose a reason for hiding this comment

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

and here, we should call referencceProjection() ?

Copy link
Contributor Author

@vicente-romero-oracle vicente-romero-oracle Aug 11, 2021

Choose a reason for hiding this comment

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

yep I agree

@@ -3613,10 +3611,10 @@ public Type visitTypeVar(TypeVar t, Void ignored) {
return to.head.withTypeVar(t);
}
if (allowUniversalTVars &&
t.createdFromUniversalTypeVar &&
!t.isValueProjection() &&
Copy link
Collaborator

@mcimadamore mcimadamore Aug 10, 2021

Choose a reason for hiding this comment

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

Not sure about this here - "not value projection might also mean it's a regular tvar". Doesn't this test want to check for T.ref (in which case you should check for isReferenceProjection

this(name, owner, lower, false, false);
}

public TypeVar(Name name, Symbol owner, Type lower, boolean isUniversal, boolean isReferenceProjection) {
Copy link
Collaborator

@mcimadamore mcimadamore Aug 10, 2021

Choose a reason for hiding this comment

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

This constructor probably should not take the isReferenceProjection as this one creates a new tsym and you want the reference projection to share the same symbol - so you probably are always passing false here.

Copy link
Collaborator

@mcimadamore mcimadamore left a comment

Looks good - just one nitpick

@@ -1986,6 +1988,9 @@ public TypeVar(TypeSymbol tsym, Type bound, Type lower,
this.lower = lower;
this.isReferenceProjection = isReferenceProjection;
this.isUniversal = (tsym.flags_field & UNIVERSAL) != 0;
if (isUniversal && !isReferenceProjection) {
Copy link
Collaborator

@mcimadamore mcimadamore Aug 12, 2021

Choose a reason for hiding this comment

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

isn't isReferenceProjection always false here (you have just created the class)

Copy link
Contributor Author

@vicente-romero-oracle vicente-romero-oracle Aug 12, 2021

Choose a reason for hiding this comment

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

not really, because this constructor is used again to build the reference, so I have to add it to the condition to avoid stackoverflow errors

@vicente-romero-oracle
Copy link
Contributor Author

@vicente-romero-oracle vicente-romero-oracle commented Aug 12, 2021

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Aug 12, 2021

Going to push as commit 3e896fa.

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

@openjdk openjdk bot commented Aug 12, 2021

@vicente-romero-oracle Pushed as commit 3e896fa.

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

@vicente-romero-oracle vicente-romero-oracle deleted the first_iteration branch Aug 12, 2021
@vicente-romero-oracle
Copy link
Contributor Author

@vicente-romero-oracle vicente-romero-oracle commented Aug 12, 2021

thanks for the review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants