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

Closed
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -1943,32 +1943,30 @@ public static class TypeVar extends Type implements TypeVariable {
*/
public Type lower;

public TVFlavor flavor;

/** if this type variable is universal then it will also have a link to a pure reference
/** if this type variable is universal then it could also have a link to a pure reference
* type variable, it is important to know that a universal type variable and its
* corresponding referenceTypeVar share the same tsym. So if it is needed to double check if
* a type variable is universal or not, we need to check its type not the type of its tsym
*/
public TypeVar projection = null;

public enum TVFlavor {
REFERENCE,
UNIVERSAL,
REFERENCE_FROM_UNIVERSAL
}
public boolean isReferenceProjection;

// redundant for now but helpful for debug reasons
public boolean isUniversal;

public TypeVar(Name name, Symbol owner, Type lower) {
this(name, owner, lower, TVFlavor.REFERENCE);
this(name, owner, lower, false, false);
}

public TypeVar(Name name, Symbol owner, Type lower, TVFlavor flavor) {
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.

super(null, TypeMetadata.EMPTY);
Assert.checkNonNull(lower);
tsym = new TypeVariableSymbol(flavor == TVFlavor.UNIVERSAL ? UNIVERSAL : 0, name, this, owner);
tsym = new TypeVariableSymbol(isUniversal ? UNIVERSAL : 0, name, this, owner);
this.setUpperBound(null);
this.lower = lower;
this.flavor = flavor;
this.isReferenceProjection = isReferenceProjection;
this.isUniversal = isUniversal;
}

public TypeVar(TypeSymbol tsym, Type bound, Type lower) {
@@ -1977,21 +1975,22 @@ public TypeVar(TypeSymbol tsym, Type bound, Type lower) {

public TypeVar(TypeSymbol tsym, Type bound, Type lower,
TypeMetadata metadata) {
this(tsym, bound, lower, metadata, TVFlavor.REFERENCE);
this(tsym, bound, lower, metadata, false);
}

public TypeVar(TypeSymbol tsym, Type bound, Type lower,
TypeMetadata metadata, TVFlavor flavor) {
TypeMetadata metadata, boolean isReferenceProjection) {
super(tsym, metadata);
Assert.checkNonNull(lower);
this.setUpperBound(bound);
this.lower = lower;
this.flavor = flavor;
this.isReferenceProjection = isReferenceProjection;
this.isUniversal = (tsym.flags_field & UNIVERSAL) != 0;
}

@Override
public TypeVar cloneWithMetadata(TypeMetadata md) {
return new TypeVar(tsym, getUpperBound(), lower, md) {
return new TypeVar(tsym, getUpperBound(), lower, md, isReferenceProjection) {
@Override
public Type baseType() { return TypeVar.this.baseType(); }

@@ -2048,18 +2047,10 @@ public <R, P> R accept(TypeVisitor<R, P> v, P p) {
return v.visitTypeVariable(this, p);
}

public TVFlavor getTVFlavor() {
return flavor;
}

public boolean hasUniversalFlavor() {
return flavor == TVFlavor.UNIVERSAL;
}

@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 ?

if (t.hasTag(TYPEVAR) &&
((TypeVar)t).flavor == TVFlavor.REFERENCE_FROM_UNIVERSAL &&
((TypeVar)t).isReferenceProjection() &&
projection != null) {
return projection;
}
@@ -2069,10 +2060,22 @@ public Type withTypeVar(Type t) {
@Override
public TypeVar referenceProjection() {
if (projection == null) {
projection = new TypeVar(tsym, _bound, lower, metadata, TVFlavor.REFERENCE_FROM_UNIVERSAL);
projection = new TypeVar(tsym, _bound, lower, metadata, true);
}
return projection;
}

public boolean isUniversal() {
return ((tsym.flags_field & UNIVERSAL) != 0);
}

public boolean isReferenceProjection() {
return isReferenceProjection;
}

public boolean isValueProjection() {
return isUniversal() && !isReferenceProjection();
}
}

/** A captured type variable comes from wildcards which can have
@@ -2092,7 +2095,7 @@ public CapturedType(Name name,
this.lower = Assert.checkNonNull(lower);
this.setUpperBound(upper);
this.wildcard = wildcard;
this.flavor = wildcard.bound != null ? wildcard.bound.flavor : TVFlavor.REFERENCE;
this.isReferenceProjection = wildcard.bound != null ? wildcard.bound.isReferenceProjection : false;
}

public CapturedType(TypeSymbol tsym,
@@ -2103,7 +2106,7 @@ public CapturedType(TypeSymbol tsym,
TypeMetadata metadata) {
super(tsym, bound, lower, metadata);
this.wildcard = wildcard;
this.flavor = wildcard.bound != null ? wildcard.bound.flavor : TVFlavor.REFERENCE;
this.isReferenceProjection = wildcard.bound != null ? wildcard.bound.isReferenceProjection : false;
}

@Override
@@ -611,8 +611,8 @@ public boolean isConvertible(Type t, Type s, Warner warn) {

boolean tValue = t.isPrimitiveClass();
boolean sValue = s.isPrimitiveClass();
if (allowUniversalTVars && (s.hasTag(TYPEVAR)) && ((TypeVar)s).hasUniversalFlavor() &&
(t.hasTag(BOT) || t.hasTag(TYPEVAR) && !((TypeVar)t).hasUniversalFlavor())) {
if (allowUniversalTVars && (s.hasTag(TYPEVAR)) && ((TypeVar)s).isValueProjection() &&
(t.hasTag(BOT) || t.hasTag(TYPEVAR) && !((TypeVar)t).isValueProjection())) {
warn.warn(LintCategory.UNIVERSAL);
return true;
}
@@ -1039,9 +1039,9 @@ public boolean isBoundedBy(Type t, Type s, Warner warn, SubtypeTestFlavor subtyp
if (allowUniversalTVars && !result) {
if (isPrimitiveClass(t)) {
return isBoundedBy(t.referenceProjection(), s, warn, subtypeTestFlavor);
} else if (t.hasTag(TYPEVAR) && ((TypeVar)t).hasUniversalFlavor()) {
} else if (t.hasTag(TYPEVAR) && ((TypeVar)t).isUniversal()) {
return isBoundedBy(t.getUpperBound(), s, warn, subtypeTestFlavor);
} else if (s.hasTag(TYPEVAR) && ((TypeVar)s).hasUniversalFlavor()) {
} else if (s.hasTag(TYPEVAR) && ((TypeVar)s).isUniversal()) {
return isBoundedBy(t, s.getLowerBound(), warn, subtypeTestFlavor);
}
}
@@ -1183,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).hasUniversalFlavor()) {
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

warnStack.head.warn(LintCategory.UNIVERSAL);
}
return
@@ -1704,7 +1704,7 @@ public Boolean visitUndetVar(UndetVar t, Type s) {
public Boolean visitTypeVar(TypeVar t, Type s) {
if (s.hasTag(TYPEVAR)) {
TypeVar other = (TypeVar)s;
if (allowUniversalTVars && t.hasUniversalFlavor() != other.hasUniversalFlavor() && t.tsym == other.tsym)
if (allowUniversalTVars && t.isValueProjection() != other.isValueProjection() && t.tsym == other.tsym)
return true;
}
return isSameType(t, s);
@@ -3611,7 +3611,7 @@ public Type visitTypeVar(TypeVar t, Void ignored) {
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?

t.flavor == TypeVar.TVFlavor.REFERENCE_FROM_UNIVERSAL &&
!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

from.head.hasTag(TYPEVAR) &&
((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

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

@@ -3766,7 +3766,7 @@ public List<Type> newInstances(List<Type> tvars) {
private static final TypeMapping<Void> newInstanceFun = new TypeMapping<Void>() {
@Override
public TypeVar visitTypeVar(TypeVar t, Void _unused) {
TypeVar newTV = new TypeVar(t.tsym, t.getUpperBound(), t.getLowerBound(), t.getMetadata(), t.flavor);
TypeVar newTV = new TypeVar(t.tsym, t.getUpperBound(), t.getLowerBound(), t.getMetadata(), t.isReferenceProjection);
if (t.projection != null) {
newTV.referenceProjection();
}
@@ -2660,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).hasUniversalFlavor();
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

if (qualifier.tsym.isPrimitiveClass() || qualifierIsUniversal || superCallOnPrimitiveReceiver) {
int argSize = argtypes.size();
Name name = symbol.name;
@@ -4597,7 +4597,7 @@ private Symbol selectSym(JCFieldAccess tree,
case WILDCARD:
throw new AssertionError(tree);
case TYPEVAR:
if (allowUniversalTVars && name == names.ref && ((TypeVar)site).hasUniversalFlavor()) {
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

TypeVar siteTV = (TypeVar)site;
if (siteTV.projection == null) {
TypeVariableSymbol tmpTVarSym = new TypeVariableSymbol(siteTV.tsym.flags(), siteTV.tsym.name, null, siteTV.tsym.owner);
@@ -5146,7 +5146,7 @@ public void visitTypeApply(JCTypeApply tree) {
List<Type> argTypes, argTypes2;
argTypes2 = argTypes = attribAnyTypes(args, env);
for (Type t : ((ClassType) clazztype.tsym.type).typarams_field) {
argTypes.head = chk.checkRefType(args.head.pos(), argTypes.head, ((TypeVar) t).hasUniversalFlavor());
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

args = args.tail;
argTypes = argTypes.tail;
}
@@ -565,7 +565,7 @@ protected void duplicateClass(DiagnosticPosition pos, ClassSymbol c) {
public void visitTypeParameter(JCTypeParameter tree) {
TypeVar a = (tree.type != null)
? (TypeVar)tree.type
: new TypeVar(tree.name, env.info.scope.owner, syms.botType, tree.universal ? TypeVar.TVFlavor.UNIVERSAL : TypeVar.TVFlavor.REFERENCE);
: new TypeVar(tree.name, env.info.scope.owner, syms.botType, tree.universal, false);
tree.type = a;
if (chk.checkUnique(tree.pos(), a.tsym, env.info.scope)) {
env.info.scope.enter(a.tsym);
@@ -1871,7 +1871,7 @@ boolean isUninitializedFieldOfUniversalTVar(VarSymbol sym) {
return allowUniversalTVars && sym.owner.kind == TYP &&
((sym.flags() & (FINAL | HASINIT | PARAMETER)) == 0 &&
classDef.sym.isEnclosedBy((ClassSymbol)sym.owner) &&
sym.type.hasTag(TYPEVAR) && ((Type.TypeVar)sym.type).hasUniversalFlavor());
sym.type.hasTag(TYPEVAR) && ((Type.TypeVar)sym.type).isValueProjection());
}

/** Initialize new trackable variable by setting its address field
@@ -1078,7 +1078,7 @@ public JCMethodDecl MethodDef(MethodSymbol m, Type mtype, JCBlock body) {
*/
public JCTypeParameter TypeParam(Name name, TypeVar tvar) {
return (JCTypeParameter)
TypeParameter(name, Types(types.getBounds(tvar)), List.nil(), tvar.hasUniversalFlavor()).setPos(pos).setType(tvar);
TypeParameter(name, Types(types.getBounds(tvar)), List.nil(), tvar.isValueProjection()).setPos(pos).setType(tvar);
}

/** Create a list of type parameter trees from a list of type variables.