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.

Sign up for GitHub

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

Jump to bottom

universal type variables: initial prototype #521

Closed
wants to merge 4 commits into from
Closed

universal type variables: initial prototype #521

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e28bdb6
universal type variables: initial prototype
vicente-romero-oracle Aug 4, 2021
39e7775
addressing review comments
vicente-romero-oracle Aug 4, 2021
3b30083
second iteration of review comments
vicente-romero-oracle Aug 10, 2021
fe1a449
review comments, another iteration
vicente-romero-oracle Aug 11, 2021
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
1 change: 1 addition & 0 deletions src/java.base/share/classes/jdk/internal/javac/PreviewFeature.java
View file
Open in desktop
Expand Up @@ -61,6 +61,7 @@

public enum Feature {
SWITCH_PATTERN_MATCHING,
UNIVERSAL_TVARS,
Copy link
Collaborator

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).

/**
* A key for testing.
*/
Expand Down
7 changes: 6 additions & 1 deletion src/jdk.compiler/share/classes/com/sun/tools/javac/code/Lint.java
View file
Open in desktop
Expand Up @@ -324,7 +324,12 @@ public enum LintCategory {
/**
* Warn about use of preview features.
*/
PREVIEW("preview");
PREVIEW("preview"),

/**
* Warn about use of universal type variables.
*/
UNIVERSAL("universal");

LintCategory(String option) {
this(option, false);
Expand Down
1 change: 1 addition & 0 deletions src/jdk.compiler/share/classes/com/sun/tools/javac/code/Preview.java
View file
Open in desktop
Expand Up @@ -187,6 +187,7 @@ public boolean isPreview(Feature feature) {
return switch (feature) {
case CASE_NULL -> true;
case PATTERN_SWITCH -> true;
case UNIVERSAL_TVARS -> true;

//Note: this is a backdoor which allows to optionally treat all features as 'preview' (for testing).
//When real preview features will be added, this method can be implemented to return 'true'
Expand Down
1 change: 1 addition & 0 deletions src/jdk.compiler/share/classes/com/sun/tools/javac/code/Source.java
View file
Open in desktop
Expand Up @@ -234,6 +234,7 @@ public enum Feature {
CASE_NULL(JDK17, Fragments.FeatureCaseNull, DiagKind.NORMAL),
PATTERN_SWITCH(JDK17, Fragments.FeaturePatternSwitch, DiagKind.PLURAL),
REDUNDANT_STRICTFP(JDK17),
UNIVERSAL_TVARS(JDK18, Fragments.FeatureUniversalTvars, DiagKind.PLURAL),
;

enum DiagKind {
Expand Down
52 changes: 52 additions & 0 deletions src/jdk.compiler/share/classes/com/sun/tools/javac/code/Type.java
View file
Open in desktop
Expand Up @@ -1943,12 +1943,34 @@ public static class TypeVar extends Type implements TypeVariable {
*/
public Type lower;

public boolean universal = false;
Copy link
Collaborator

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

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

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).


public boolean createdFromUniversalTypeVar = false;

/** if this type variable is universal then it will 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 referenceProjection = null;

/** link back to universal type var when applicable, this field will have a value if this current
* type variable was derived form a type variable declaration using the .ref suffix, once the code
* is more mature we can fold fields referenceTypeVar and universalTypeVar
*/
public TypeVar universalTypeVar = null;

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

public TypeVar(Name name, Symbol owner, Type lower, boolean universal) {
super(null, TypeMetadata.EMPTY);
Assert.checkNonNull(lower);
tsym = new TypeVariableSymbol(0, name, this, owner);
this.setUpperBound(null);
this.lower = lower;
this.universal = universal;
}

public TypeVar(TypeSymbol tsym, Type bound, Type lower) {
Expand All @@ -1957,10 +1979,16 @@ public TypeVar(TypeSymbol tsym, Type bound, Type lower) {

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

public TypeVar(TypeSymbol tsym, Type bound, Type lower,
TypeMetadata metadata, boolean universal) {
super(tsym, metadata);
Assert.checkNonNull(lower);
this.setUpperBound(bound);
this.lower = lower;
this.universal = universal;
}

@Override
Expand Down Expand Up @@ -2021,6 +2049,26 @@ public boolean isNullOrReference() {
public <R, P> R accept(TypeVisitor<R, P> v, P p) {
return v.visitTypeVariable(this, p);
}

public boolean isUniversal() {
return universal;
}

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

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).createdFromUniversalTypeVar &&
referenceProjection != null) {
return referenceProjection;
}
return this;
}

public void createReferenceProjection() {
referenceProjection = new TypeVar(tsym, _bound, lower, metadata, false);
referenceProjection.createdFromUniversalTypeVar = true;
referenceProjection.universalTypeVar = this;
}
}

/** A captured type variable comes from wildcards which can have
Expand All @@ -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

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()

lower.hasTag(TYPEVAR) && ((TypeVar)lower).universal;
}

public CapturedType(TypeSymbol tsym,
Expand All @@ -2050,6 +2100,8 @@ public CapturedType(TypeSymbol tsym,
TypeMetadata metadata) {
super(tsym, bound, lower, metadata);
this.wildcard = wildcard;
this.universal = upper.hasTag(TYPEVAR) && ((TypeVar)upper).universal ||
lower.hasTag(TYPEVAR) && ((TypeVar)lower).universal;
}

@Override
Expand Down
76 changes: 72 additions & 4 deletions src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java
View file
Open in desktop
Expand Up @@ -95,6 +95,7 @@ public class Types {
final boolean allowDefaultMethods;
final boolean mapCapturesToBounds;
final boolean allowValueBasedClasses;
final boolean allowUniversalTVars;
final Check chk;
final Enter enter;
JCDiagnostic.Factory diags;
Expand Down Expand Up @@ -125,7 +126,10 @@ protected Types(Context context) {
diags = JCDiagnostic.Factory.instance(context);
noWarnings = new Warner(null);
Options options = Options.instance(context);
Preview preview = Preview.instance(context);
allowValueBasedClasses = options.isSet("allowValueBasedClasses");
allowUniversalTVars = (!preview.isPreview(Feature.UNIVERSAL_TVARS) || preview.isEnabled()) &&
Feature.UNIVERSAL_TVARS.allowedInSource(source);
}
// </editor-fold>

Expand Down Expand Up @@ -608,6 +612,11 @@ public boolean isConvertible(Type t, Type s, Warner warn) {

boolean tValue = t.isPrimitiveClass();
boolean sValue = s.isPrimitiveClass();
if (allowUniversalTVars && (s.hasTag(TYPEVAR)) && ((TypeVar)s).isUniversal() &&
(t.hasTag(BOT) || t.hasTag(TYPEVAR) && !((TypeVar)t).isUniversal())) {
warn.warn(LintCategory.UNIVERSAL);
return true;
}
if (tValue != sValue) {
return tValue ?
isSubtype(t.referenceProjection(), s) :
Expand Down Expand Up @@ -1014,6 +1023,32 @@ public boolean isPrimitiveClass(Type t) {
return t != null && t.isPrimitiveClass();
}

@FunctionalInterface
public interface SubtypeTestFlavor {
boolean subtypeTest(Type t, Type s, Warner warn);
}

public boolean isBoundedBy(Type t, Type s, SubtypeTestFlavor subtypeTestFlavor) {
return isBoundedBy(t, s, noWarnings, subtypeTestFlavor);
}

/**
* Is type t bounded by s?
*/
public boolean isBoundedBy(Type t, Type s, Warner warn, SubtypeTestFlavor subtypeTestFlavor) {
boolean result = subtypeTestFlavor.subtypeTest(t, s, warn);
if (allowUniversalTVars && !result) {
if (isPrimitiveClass(t)) {
return isBoundedBy(t.referenceProjection(), s, warn, subtypeTestFlavor);
} else if (t.hasTag(TYPEVAR) && ((TypeVar)t).universal) {
return isBoundedBy(t.getUpperBound(), s, warn, subtypeTestFlavor);
} else if (s.hasTag(TYPEVAR) && ((TypeVar)s).universal) {
return isBoundedBy(t, s.getLowerBound(), warn, subtypeTestFlavor);
}
}
return result;
}

// <editor-fold defaultstate="collapsed" desc="isSubtype">
/**
* Is t an unchecked subtype of s?
Expand Down Expand Up @@ -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

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

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

// we are seeing a case of a universal type variable being assigned to a non-universal one
warn.warn(LintCategory.UNIVERSAL);
return true;
} else if (t.hasTag(TYPEVAR)) {
return isSubtypeUncheckedInternal(t.getUpperBound(), s, false, warn);
} else if (!s.isRaw()) {
Expand Down Expand Up @@ -1102,6 +1141,9 @@ public final boolean isSubtypeNoCapture(Type t, Type s) {
public boolean isSubtype(Type t, Type s, boolean capture) {
if (t.equalsIgnoreMetadata(s))
return true;
if (t.hasTag(TYPEVAR) && s.hasTag(TYPEVAR) && t.tsym == s.tsym) {
return true;
}
if (s.isPartial())
return isSuperType(s, t);

Expand Down Expand Up @@ -1143,6 +1185,9 @@ public Boolean visitType(Type t, Type s) {
case TYPEVAR:
return isSubtypeNoCapture(t.getUpperBound(), s);
case BOT:
if (allowUniversalTVars && s.hasTag(TYPEVAR) && ((TypeVar)s).universal) {
warnStack.head.warn(LintCategory.UNIVERSAL);
}
return
s.hasTag(BOT) || (s.hasTag(CLASS) && !isPrimitiveClass(s)) ||
s.hasTag(ARRAY) || s.hasTag(TYPEVAR);
Expand Down Expand Up @@ -1641,8 +1686,10 @@ public Boolean visitWildcardType(WildcardType t, Type s) {
// ---------------------------------------------------------------------------
return isSameWildcard(t, s)
|| isCaptureOf(s, t)
|| ((t.isExtendsBound() || isSubtypeNoCapture(wildLowerBound(t), wildLowerBound(s))) &&
(t.isSuperBound() || isSubtypeNoCapture(wildUpperBound(s), wildUpperBound(t))));
|| ((t.isExtendsBound() || isBoundedBy(wildLowerBound(t), wildLowerBound(s),
(t1, s1, w) -> isSubtypeNoCapture(t1, s1))) &&
(t.isSuperBound() || isBoundedBy(wildUpperBound(s), wildUpperBound(t),
(t1, s1, w) -> isSubtypeNoCapture(t1, s1))));
}
}

Expand All @@ -1655,6 +1702,16 @@ public Boolean visitUndetVar(UndetVar t, Type s) {
}
}

@Override
public Boolean visitTypeVar(TypeVar t, Type s) {
if (s.hasTag(TYPEVAR)) {
TypeVar other = (TypeVar)s;
if (allowUniversalTVars && t.universal != other.universal && t.tsym == other.tsym)
return true;
}
return isSameType(t, s);
}

@Override
public Boolean visitErrorType(ErrorType t, Type s) {
return true;
Expand Down Expand Up @@ -2075,7 +2132,7 @@ public boolean notSoftSubtype(Type t, Type s) {
if (!s.hasTag(WILDCARD))
s = cvarUpperBound(s);

return !isSubtype(t, relaxBound(s));
return !isBoundedBy(t, relaxBound(s), (t1, s1, w) -> isSubtype(t1, s1));
}

private Type relaxBound(Type t) {
Expand Down Expand Up @@ -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

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

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

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.createdFromUniversalTypeVar &&
from.head.hasTag(TYPEVAR) &&
((TypeVar)from.head).referenceProjection != null &&
t.equalsIgnoreMetadata(((TypeVar)from.head).referenceProjection)) {
return to.head.withTypeVar(t);
}
}
return t;
}
Expand Down Expand Up @@ -3704,7 +3768,11 @@ public List<Type> newInstances(List<Type> tvars) {
private static final TypeMapping<Void> newInstanceFun = new TypeMapping<Void>() {
@Override
public TypeVar visitTypeVar(TypeVar t, Void _unused) {
return new TypeVar(t.tsym, t.getUpperBound(), t.getLowerBound(), t.getMetadata());
TypeVar newTV = new TypeVar(t.tsym, t.getUpperBound(), t.getLowerBound(), t.getMetadata(), t.universal);
if (t.referenceProjection != null) {
newTV.createReferenceProjection();
}
return newTV;
}
};
// </editor-fold>
Expand Down