Skip to content

Commit

Permalink
[Switch][Record Patterns] Unchecked conversion error missing from ECJ (
Browse files Browse the repository at this point in the history
…#3069)

check all patterns via isApplicable()
+ enclosing instanceof and case stmt are responsible for its invocation
+ make isApplicable() more capable to issue the desired problems
+ let problem reporter see the error location to select problemID
+ more renaming for clarity
+ fix regression by more complete propagation of outerExpressionType

fix "left" and "right" types in error reporting

Test changes:
+ compatibility errors more consistently mark the entire instanceof expr
+ remove unwanted type qualifiers
+ don't expect some secondary errors
+ test case for #3074
  • Loading branch information
stephan-herrmann authored Oct 24, 2024
1 parent 05c3001 commit 55f53bd
Show file tree
Hide file tree
Showing 12 changed files with 182 additions and 84 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -381,11 +381,8 @@ private Constant resolveCasePattern(BlockScope scope, TypeBinding caseType, Type
// The following code is copied from InstanceOfExpression#resolve()
// But there are enough differences to warrant a copy
if (!type.isReifiable()) {
if (expressionType != TypeBinding.NULL && !(e instanceof RecordPattern)) {
boolean isLegal = e.checkCastTypesCompatibility(scope, type, expressionType, e, false);
if (!isLegal || (e.bits & ASTNode.UnsafeCast) != 0) {
scope.problemReporter().unsafeCastInInstanceof(e, type, expressionType);
}
if (!e.isApplicable(switchExpressionType, scope, e)) {
return Constant.NotAConstant;
}
} else if (type.isValidBinding()) {
// if not a valid binding, an error has already been reported for unresolved type
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,13 @@ public void setIsGuarded() {
this.patterns[i].setIsGuarded();
}

@Override
public void setOuterExpressionType(TypeBinding expressionType) {
super.setOuterExpressionType(expressionType);
for (int i = 0; i < this.patternsCount; i++)
this.patterns[i].setOuterExpressionType(expressionType);
}

@Override
public TypeBinding resolveType(BlockScope scope) {
boolean hasError = false;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,12 @@ public void setIsEitherOrPattern() {
this.primaryPattern.setIsEitherOrPattern();
}

@Override
public void setOuterExpressionType(TypeBinding expressionType) {
super.setOuterExpressionType(expressionType);
this.primaryPattern.setOuterExpressionType(expressionType);
}

@Override
public boolean coversType(TypeBinding type, Scope scope) {
return isUnguarded() && this.primaryPattern.coversType(type, scope);
Expand Down Expand Up @@ -130,7 +136,7 @@ public void traverse(ASTVisitor visitor, BlockScope scope) {
}

@Override
protected boolean isApplicable(TypeBinding other, BlockScope scope) {
return this.primaryPattern.isApplicable(other, scope);
protected boolean isApplicable(TypeBinding expressionType, BlockScope scope, ASTNode location) {
return this.primaryPattern.isApplicable(expressionType, scope, location);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,13 @@ public TypeBinding resolveType(BlockScope scope) {
if (expressionType == null || checkedType == null)
return null;

if (this.pattern != null) {
if (this.pattern.isApplicable(expressionType, scope, this)) {
checkForPrimitives(scope, checkedType, expressionType);
}
return this.resolvedType = TypeBinding.BOOLEAN;
}

if (!checkedType.isReifiable()) {
CompilerOptions options = scope.compilerOptions();
// Report same as before for older compliances
Expand All @@ -286,25 +293,26 @@ public TypeBinding resolveType(BlockScope scope) {
if (expressionType != TypeBinding.NULL) {
boolean isLegal = checkCastTypesCompatibility(scope, checkedType, expressionType, this.expression, true);
if (!isLegal || (this.bits & ASTNode.UnsafeCast) != 0) {
scope.problemReporter().unsafeCastInInstanceof(this.expression, checkedType, expressionType);
scope.problemReporter().unsafeCastInTestingContext(this.expression, checkedType, expressionType);
} else {
checkRefForPrimitivesAndAddSecretVariable(scope, checkedType, expressionType);
}
}
}
} else if (checkedType.isValidBinding()) {
// if not a valid binding, an error has already been reported for unresolved type
if ((expressionType != TypeBinding.NULL && expressionType.isBaseType()) // disallow autoboxing
|| checkedType.isBaseType()
|| !checkCastTypesCompatibility(scope, checkedType, expressionType, null, true)) {
checkForPrimitives(scope, checkedType, expressionType);
}
checkForPrimitives(scope, checkedType, expressionType);
}

return this.resolvedType = TypeBinding.BOOLEAN;
}

private void checkForPrimitives(BlockScope scope, TypeBinding checkedType, TypeBinding expressionType) {
boolean needToCheck = (expressionType != TypeBinding.NULL && expressionType.isBaseType()) // disallow autoboxing
|| checkedType.isBaseType()
|| !checkCastTypesCompatibility(scope, checkedType, expressionType, null, true);
if (!needToCheck)
return;
PrimitiveConversionRoute route = Pattern.findPrimitiveConversionRoute(checkedType, expressionType, scope);
this.testContextRecord = new TestContextRecord(checkedType, expressionType, route);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -139,25 +139,37 @@ public TypeReference getType() {
}

// 14.30.3 Properties of Patterns: A pattern p is said to be applicable at a type T if ...
protected boolean isApplicable(TypeBinding other, BlockScope scope) {
TypeBinding patternType = this.resolvedType;
if (patternType == null) // ill resolved pattern
return false;
protected boolean isApplicable(TypeBinding expressionType, BlockScope scope, ASTNode location) {
if (expressionType == TypeBinding.NULL)
return true;
TypeReference typeRef = getType();
if (typeRef == null)
return true; // nothing to be checked for wildcard '_'
TypeBinding patternType = typeRef.resolvedType;
if (patternType == null || !patternType.isValidBinding() || !expressionType.isValidBinding())
return false; // problem already reported

// 14.30.3 Properties of Patterns doesn't allow boxing nor unboxing, primitive widening/narrowing (< JLS23)
if (patternType.isBaseType() != other.isBaseType() && !JavaFeature.PRIMITIVES_IN_PATTERNS.isSupported(scope.compilerOptions())) {
scope.problemReporter().incompatiblePatternType(this, other, patternType);
if (patternType.isBaseType() != expressionType.isBaseType() && !JavaFeature.PRIMITIVES_IN_PATTERNS.isSupported(scope.compilerOptions())) {
scope.problemReporter().notCompatibleTypesError(location, expressionType, patternType);
return false;
}
if (patternType.isBaseType()) {
PrimitiveConversionRoute route = Pattern.findPrimitiveConversionRoute(this.resolvedType, this.outerExpressionType, scope);
if (!TypeBinding.equalsEquals(other, patternType)
if (!TypeBinding.equalsEquals(expressionType, patternType)
&& route == PrimitiveConversionRoute.NO_CONVERSION_ROUTE) {
scope.problemReporter().incompatiblePatternType(this, other, patternType);
scope.problemReporter().notCompatibleTypesError(location, expressionType, patternType);
return false;
}
} else {
if (!checkCastTypesCompatibility(scope, patternType, expressionType, null, true)) {
scope.problemReporter().notCompatibleTypesError(location, expressionType, patternType);
return false;
}
if ((this.bits & ASTNode.UnsafeCast) != 0) {
scope.problemReporter().unsafeCastInTestingContext(location, patternType, this.outerExpressionType);
return false;
}
} else if (!checkCastTypesCompatibility(scope, other, patternType, null, true)) {
scope.problemReporter().incompatiblePatternType(this, other, patternType);
return false;
}
return true;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ public TypeBinding resolveType(BlockScope scope) {
}
}
TypeBinding componentType = componentBinding.type;
if (p1.isApplicable(componentType, scope)) {
if (p1.isApplicable(componentType, scope, p1)) {
p1.isTotalTypeNode = p1.coversType(componentType, scope);
MethodBinding[] methods = this.resolvedType.getMethods(componentBinding.name);
if (methods != null && methods.length > 0) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7303,7 +7303,7 @@ public void noSuchEnclosingInstance(TypeBinding targetType, ASTNode location, bo
location.sourceStart,
location instanceof LambdaExpression ? ((LambdaExpression)location).diagnosticsSourceEnd() : location.sourceEnd);
}
public void notCompatibleTypesError(EqualExpression expression, TypeBinding leftType, TypeBinding rightType) {
public void notCompatibleTypesError(ASTNode location, TypeBinding leftType, TypeBinding rightType) {
String leftName = new String(leftType.readableName());
String rightName = new String(rightType.readableName());
String leftShortName = new String(leftType.shortReadableName());
Expand All @@ -7312,28 +7312,18 @@ public void notCompatibleTypesError(EqualExpression expression, TypeBinding left
leftShortName = leftName;
rightShortName = rightName;
}
this.handle(
IProblem.IncompatibleTypesInEqualityOperator,
new String[] {leftName, rightName },
new String[] {leftShortName, rightShortName },
expression.sourceStart,
expression.sourceEnd);
}
public void notCompatibleTypesError(Expression expression, TypeBinding leftType, TypeBinding rightType) {
String leftName = new String(leftType.readableName());
String rightName = new String(rightType.readableName());
String leftShortName = new String(leftType.shortReadableName());
String rightShortName = new String(rightType.shortReadableName());
if (leftShortName.equals(rightShortName)){
leftShortName = leftName;
rightShortName = rightName;
int problemId = IProblem.IncompatibleTypesInEqualityOperator;
if (location instanceof Pattern p && p.getEnclosingPattern() instanceof RecordPattern) {
problemId = IProblem.PatternTypeMismatch;
} else if (location instanceof InstanceOfExpression) {
problemId = IProblem.IncompatibleTypesInConditionalOperator;
}
this.handle(
IProblem.IncompatibleTypesInConditionalOperator,
problemId,
new String[] {leftName, rightName },
new String[] {leftShortName, rightShortName },
expression.sourceStart,
expression.sourceEnd);
location.sourceStart,
location.sourceEnd);
}
public void notCompatibleTypesErrorInForeach(Expression expression, TypeBinding leftType, TypeBinding rightType) {
String leftName = new String(leftType.readableName());
Expand Down Expand Up @@ -8538,21 +8528,21 @@ public void typeCastError(CastExpression expression, TypeBinding leftType, TypeB
expression.sourceStart,
expression.sourceEnd);
}
public void unsafeCastInInstanceof(Expression expression, TypeBinding leftType, TypeBinding rightType) {
String leftName = new String(leftType.readableName());
String rightName = new String(rightType.readableName());
String leftShortName = new String(leftType.shortReadableName());
String rightShortName = new String(rightType.shortReadableName());
if (leftShortName.equals(rightShortName)){
leftShortName = leftName;
rightShortName = rightName;
public void unsafeCastInTestingContext(ASTNode location, TypeBinding castType, TypeBinding expressionType) {
String castName = new String(castType.readableName());
String exprName = new String(expressionType.readableName());
String castShortName = new String(castType.shortReadableName());
String exprShortName = new String(expressionType.shortReadableName());
if (castShortName.equals(exprShortName)){
castShortName = castName;
exprShortName = exprName;
}
this.handle(
IProblem.UnsafeCast,
new String[] { rightName, leftName },
new String[] { rightShortName, leftShortName },
expression.sourceStart,
expression.sourceEnd);
new String[] { exprName, castName },
new String[] { exprShortName, castShortName },
location.sourceStart,
location.sourceEnd);
}
public void typeCollidesWithEnclosingType(TypeDeclaration typeDecl) {
String[] arguments = new String[] {new String(typeDecl.name)};
Expand Down Expand Up @@ -12404,14 +12394,6 @@ public void recordPatternSignatureMismatch(TypeBinding type, ASTNode element) {
element.sourceStart,
element.sourceEnd);
}
public void incompatiblePatternType(ASTNode element, TypeBinding type, TypeBinding expected) {
this.handle(
IProblem.PatternTypeMismatch,
new String[] {new String(type.readableName()), new String(expected.readableName())},
new String[] {new String(type.shortReadableName()), new String(expected.readableName())},
element.sourceStart,
element.sourceEnd);
}
public void cannotInferRecordPatternTypes(RecordPattern pattern) {
String arguments [] = new String [] { pattern.toString() };
this.handle(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -646,4 +646,28 @@ public static void gain(String[] args) {
+ "A pattern variable with the same name is already defined in the statement\n"
+ "----------\n");
}

public void testGH3074() {
runNegativeTest(
new String[] {
"Example.java",
"""
class Example<T> {
private void foo(String x) {
if (x instanceof Example<String> es) {
}
}
}
"""
},
"""
----------
1. ERROR in Example.java (at line 3)
if (x instanceof Example<String> es) {
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Incompatible conditional operand types String and Example<String>
----------
""");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2401,7 +2401,7 @@ public void testBug562392c() {
"----------\n" +
"1. ERROR in X.java (at line 4)\n" +
" if (obj instanceof T t) {\n" +
" ^^^\n" +
" ^^^^^^^^^^^^^^^^^^\n" +
"Type Object cannot be safely cast to T\n" +
"----------\n",
"X.java:4: error: Object cannot be safely cast to T\n" +
Expand Down Expand Up @@ -2460,7 +2460,7 @@ public void testBug562392e() {
"----------\n" +
"1. ERROR in X.java (at line 4)\n" +
" if (obj instanceof X<String> p) {\n" +
" ^^^\n" +
" ^^^^^^^^^^^^^^^^^^^^^^^^^^\n" +
"Type X<capture#1-of ?> cannot be safely cast to X<String>\n" +
"----------\n",
"",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2302,6 +2302,28 @@ int foo4(int myInt) {
""");
}

public void testIncompatiblePrimitiveInInstanceof() {
runNegativeTest(new String[] {
"X.java",
"""
public class X {
void foo() {
if (this instanceof int i)
return;
}
}
"""
},
"""
----------
1. ERROR in X.java (at line 3)
if (this instanceof int i)
^^^^^^^^^^^^^^^^^^^^^
Incompatible conditional operand types X and int
----------
""");
}

// test from spec
public void _testSpec001() {
runConformTest(new String[] {
Expand Down
Loading

0 comments on commit 55f53bd

Please sign in to comment.