Skip to content

Commit

Permalink
feat: consider nullability of upper type parameter bound in various c…
Browse files Browse the repository at this point in the history
…hecks (#892)

### Summary of Changes

Type parameter types can be nullable in two different way:
1. They are explicitly declared as nullable using a `?`.
2. They have a nullable upper bound.

This PR now updates various checks to also consider the 2. option:
* Info: Elvis operator is unnecessary, because its left operand is never
`null`.
* Info: Null-safe index access/member access is unnecessary, because the
receiver is never `null`.
* Error: Index access/member access must be null-safe, because the
receiver can be `null`.
  • Loading branch information
lars-reimann authored Feb 17, 2024
1 parent add650d commit 940515a
Show file tree
Hide file tree
Showing 7 changed files with 143 additions and 24 deletions.
15 changes: 15 additions & 0 deletions packages/safe-ds-lang/src/language/typing/safe-ds-type-checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -375,6 +375,21 @@ export class SafeDsTypeChecker {
}
};

/**
* Returns whether {@link type} can be `null`. Compared to {@link Type.isNullable}, this method also considers the
* upper bound of type parameter types.
*/
canBeNull = (type: Type): boolean => {
if (type.isNullable) {
return true;
} else if (type instanceof TypeParameterType) {
const upperBound = this.typeComputer().computeUpperBound(type);
return upperBound.isNullable;
} else {
return false;
}
};

/**
* Checks whether {@link type} is allowed as the type of a constant parameter.
*/
Expand Down
39 changes: 29 additions & 10 deletions packages/safe-ds-lang/src/language/typing/safe-ds-type-computer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -791,18 +791,22 @@ export class SafeDsTypeComputer {
* If invalid lower bounds are specified (e.g. because of an unresolved reference or a cycle), `$unknown` is
* returned. The result is simplified as much as possible.
*/
computeLowerBound(nodeOrType: SdsTypeParameter | TypeParameterType): Type {
computeLowerBound(nodeOrType: SdsTypeParameter | TypeParameterType, options: ComputeBoundOptions = {}): Type {
let type: TypeParameterType;
if (nodeOrType instanceof TypeParameterType) {
type = nodeOrType;
} else {
type = this.computeType(nodeOrType) as TypeParameterType;
}

return this.doComputeLowerBound(type, new Set());
return this.doComputeLowerBound(type, new Set(), options);
}

private doComputeLowerBound(type: TypeParameterType, visited: Set<SdsTypeParameter>): Type {
private doComputeLowerBound(
type: TypeParameterType,
visited: Set<SdsTypeParameter>,
options: ComputeBoundOptions,
): Type {
// Check for cycles
if (visited.has(type.declaration)) {
return UnknownType;
Expand All @@ -817,10 +821,10 @@ export class SafeDsTypeComputer {
const boundType = this.computeLowerBoundType(lowerBounds[0]!);
if (!(boundType instanceof NamedType)) {
return UnknownType;
} else if (!(boundType instanceof TypeParameterType)) {
} else if (options.stopAtTypeParameterType || !(boundType instanceof TypeParameterType)) {
return boundType;
} else {
return this.doComputeLowerBound(boundType, visited);
return this.doComputeLowerBound(boundType, visited, options);
}
}

Expand All @@ -837,19 +841,23 @@ export class SafeDsTypeComputer {
* invalid upper bounds are specified, but are invalid (e.g. because of an unresolved reference or a cycle),
* `$unknown` is returned. The result is simplified as much as possible.
*/
computeUpperBound(nodeOrType: SdsTypeParameter | TypeParameterType): Type {
computeUpperBound(nodeOrType: SdsTypeParameter | TypeParameterType, options: ComputeBoundOptions = {}): Type {
let type: TypeParameterType;
if (nodeOrType instanceof TypeParameterType) {
type = nodeOrType;
} else {
type = this.computeType(nodeOrType) as TypeParameterType;
}

const result = this.doComputeUpperBound(type, new Set());
const result = this.doComputeUpperBound(type, new Set(), options);
return result.updateNullability(result.isNullable || type.isNullable);
}

private doComputeUpperBound(type: TypeParameterType, visited: Set<SdsTypeParameter>): Type {
private doComputeUpperBound(
type: TypeParameterType,
visited: Set<SdsTypeParameter>,
options: ComputeBoundOptions,
): Type {
// Check for cycles
if (visited.has(type.declaration)) {
return UnknownType;
Expand All @@ -864,10 +872,10 @@ export class SafeDsTypeComputer {
const boundType = this.computeUpperBoundType(upperBounds[0]!);
if (!(boundType instanceof NamedType)) {
return UnknownType;
} else if (!(boundType instanceof TypeParameterType)) {
} else if (options.stopAtTypeParameterType || !(boundType instanceof TypeParameterType)) {
return boundType;
} else {
return this.doComputeUpperBound(boundType, visited);
return this.doComputeUpperBound(boundType, visited, options);
}
}

Expand Down Expand Up @@ -1140,6 +1148,17 @@ export class SafeDsTypeComputer {
}
}

/**
* Options for {@link computeLowerBound} and {@link computeUpperBound}.
*/
interface ComputeBoundOptions {
/**
* If `true`, the computation stops at type parameter types and returns them as is. Otherwise, it finds the bounds
* for the type parameter types recursively.
*/
stopAtTypeParameterType?: boolean;
}

interface GroupTypesResult {
classTypes: ClassType[];
constants: Constant[];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,25 +15,21 @@ export const chainedExpressionsMustBeNullSafeIfReceiverIsNullable = (services: S
}

const receiverType = typeComputer.computeType(node.receiver);
if (receiverType === UnknownType) {
if (receiverType === UnknownType || !typeChecker.canBeNull(receiverType)) {
return;
}

if (isSdsCall(node) && receiverType.isNullable && typeChecker.canBeCalled(receiverType)) {
if (isSdsCall(node) && typeChecker.canBeCalled(receiverType)) {
accept('error', 'The receiver can be null so a null-safe call must be used.', {
node,
code: CODE_CHAINED_EXPRESSION_MISSING_NULL_SAFETY,
});
} else if (
isSdsIndexedAccess(node) &&
receiverType.isNullable &&
typeChecker.canBeAccessedByIndex(receiverType)
) {
} else if (isSdsIndexedAccess(node) && typeChecker.canBeAccessedByIndex(receiverType)) {
accept('error', 'The receiver can be null so a null-safe indexed access must be used.', {
node,
code: CODE_CHAINED_EXPRESSION_MISSING_NULL_SAFETY,
});
} else if (isSdsMemberAccess(node) && receiverType.isNullable) {
} else if (isSdsMemberAccess(node)) {
accept('error', 'The receiver can be null so a null-safe member access must be used.', {
node,
code: CODE_CHAINED_EXPRESSION_MISSING_NULL_SAFETY,
Expand Down
11 changes: 6 additions & 5 deletions packages/safe-ds-lang/src/language/validation/style.ts
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,7 @@ export const constraintListShouldNotBeEmpty = (services: SafeDsServices) => {
export const elvisOperatorShouldBeNeeded = (services: SafeDsServices) => {
const partialEvaluator = services.evaluation.PartialEvaluator;
const settingsProvider = services.workspace.SettingsProvider;
const typeChecker = services.types.TypeChecker;
const typeComputer = services.types.TypeComputer;

return async (node: SdsInfixOperation, accept: ValidationAcceptor) => {
Expand All @@ -235,7 +236,7 @@ export const elvisOperatorShouldBeNeeded = (services: SafeDsServices) => {
}

const leftType = typeComputer.computeType(node.leftOperand);
if (!leftType.isNullable) {
if (!typeChecker.canBeNull(leftType)) {
accept(
'info',
'The left operand is never null, so the elvis operator is unnecessary (keep the left operand).',
Expand Down Expand Up @@ -322,14 +323,14 @@ export const chainedExpressionNullSafetyShouldBeNeeded = (services: SafeDsServic
}

const receiverType = typeComputer.computeType(node.receiver);
if (receiverType === UnknownType) {
if (receiverType === UnknownType || typeChecker.canBeNull(receiverType)) {
return;
}

if (
(isSdsCall(node) && !receiverType.isNullable && typeChecker.canBeCalled(receiverType)) ||
(isSdsIndexedAccess(node) && !receiverType.isNullable && typeChecker.canBeAccessedByIndex(receiverType)) ||
(isSdsMemberAccess(node) && !receiverType.isNullable)
(isSdsCall(node) && typeChecker.canBeCalled(receiverType)) ||
(isSdsIndexedAccess(node) && typeChecker.canBeAccessedByIndex(receiverType)) ||
isSdsMemberAccess(node)
) {
accept('info', 'The receiver is never null, so null-safety is unnecessary.', {
node,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,24 @@ segment indexedAccess(
»unresolved?[0]«;
}

class IndexedAccess<Nullable, NonNullable>(
nullable: Nullable,
nonNullable: NonNullable,

// $TEST$ error "The receiver can be null so a null-safe indexed access must be used."
p1: Any? = »nullable[0]«,
// $TEST$ no error "The receiver can be null so a null-safe indexed access must be used."
p2: Any? = »nonNullable[0]«,

// $TEST$ no error "The receiver can be null so a null-safe indexed access must be used."
p3: Any? = »nullable?[0]«,
// $TEST$ no error "The receiver can be null so a null-safe indexed access must be used."
p4: Any? = »nonNullable?[0]«,
) where {
Nullable sub List<Int>?,
NonNullable sub List<Int>
}

segment memberAccess(
myClass: MyClass,
myClassOrNull: MyClass?,
Expand Down Expand Up @@ -128,3 +146,21 @@ segment memberAccess(
// $TEST$ no error "The receiver can be null so a null-safe member access must be used."
»unresolved?.a«;
}

class MemberAccess<Nullable, NonNullable>(
nullable: Nullable,
nonNullable: NonNullable,

// $TEST$ error "The receiver can be null so a null-safe member access must be used."
p1: Any? = »nullable.a«,
// $TEST$ no error "The receiver can be null so a null-safe member access must be used."
p2: Any? = »nonNullable.a«,

// $TEST$ no error "The receiver can be null so a null-safe member access must be used."
p3: Any? = »nullable?.a«,
// $TEST$ no error "The receiver can be null so a null-safe member access must be used."
p4: Any? = »nonNullable?.a«,
) where {
Nullable sub MyClass?,
NonNullable sub MyClass
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
package validation.style.unnecessaryElvisOperator

fun f() -> result: Any?
@Pure fun f() -> result: Any?

pipeline test {

Expand Down Expand Up @@ -40,3 +40,19 @@ pipeline test {
// $TEST$ info "Both operands are always null, so the elvis operator is unnecessary (replace it with null)."
»null ?: null«;
}

class TestsForTypeParameters<Nullable, NonNullable>(
nullable: Nullable,
nonNullable: NonNullable,

// $TEST$ no info "The left operand is never null, so the elvis operator is unnecessary (keep the left operand)."
p1: Any? = »nullable ?: 2«,
// $TEST$ no info "The left operand is never null, so the elvis operator is unnecessary (keep the left operand)."
p2: Any? = »nullable ?: null«,
// $TEST$ info "The left operand is never null, so the elvis operator is unnecessary (keep the left operand)."
p3: Any? = »nonNullable ?: 2«,
// $TEST$ info "The left operand is never null, so the elvis operator is unnecessary (keep the left operand)."
p4: Any? = »nonNullable ?: null«,
) where {
NonNullable sub Any
}
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,24 @@ segment indexedAccess(
»unresolved?[0]«;
}

class IndexedAccess<Nullable, NonNullable>(
nullable: Nullable,
nonNullable: NonNullable,

// $TEST$ no info "The receiver is never null, so null-safety is unnecessary."
p1: Any? = »nullable[0]«,
// $TEST$ no info "The receiver is never null, so null-safety is unnecessary."
p2: Any? = »nonNullable[0]«,

// $TEST$ no info "The receiver is never null, so null-safety is unnecessary."
p3: Any? = »nullable?[0]«,
// $TEST$ info "The receiver is never null, so null-safety is unnecessary."
p4: Any? = »nonNullable?[0]«,
) where {
Nullable sub List<Int>?,
NonNullable sub List<Int>
}

segment memberAccess(
myClass: MyClass,
myClassOrNull: MyClass?,
Expand Down Expand Up @@ -128,3 +146,21 @@ segment memberAccess(
// $TEST$ no info "The receiver is never null, so null-safety is unnecessary."
»unresolved?.a«;
}

class MemberAccess<Nullable, NonNullable>(
nullable: Nullable,
nonNullable: NonNullable,

// $TEST$ no info "The receiver is never null, so null-safety is unnecessary."
p1: Any? = »nullable.a«,
// $TEST$ no info "The receiver is never null, so null-safety is unnecessary."
p2: Any? = »nonNullable.a«,

// $TEST$ no info "The receiver is never null, so null-safety is unnecessary."
p3: Any? = »nullable?.a«,
// $TEST$ info "The receiver is never null, so null-safety is unnecessary."
p4: Any? = »nonNullable?.a«,
) where {
Nullable sub MyClass?,
NonNullable sub MyClass
}

0 comments on commit 940515a

Please sign in to comment.