Skip to content

[move-only] Fix borrowing address only no consume diagnostic to not say can't capture. #66988

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

Merged
merged 1 commit into from
Jun 28, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 19 additions & 2 deletions lib/SILOptimizer/Mandatory/MoveOnlyAddressCheckerUtils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1852,11 +1852,28 @@ bool GatherUsesVisitor::visitUse(Operand *op) {
assert(checkKind == MarkMustCheckInst::CheckKind::NoConsumeOrAssign);
moveChecker.diagnosticEmitter.emitObjectGuaranteedDiagnostic(
markedValue);
} else {
// Otherwise, we need to emit an escaping closure error.
return true;
}

// If we have a function argument that is no_consume_or_assign and we do
// not have any partial apply uses, then we know that we have a use of
// an address only borrowed parameter that we need to
if (fArg &&
checkKind == MarkMustCheckInst::CheckKind::NoConsumeOrAssign &&
!moveChecker.canonicalizer.hasPartialApplyConsumingUse()) {
moveChecker.diagnosticEmitter.emitObjectGuaranteedDiagnostic(
markedValue);
return true;
}

// Finally try to emit either a global or class field error...
if (!moveChecker.diagnosticEmitter
.emitGlobalOrClassFieldLoadedAndConsumed(markedValue)) {
// And otherwise if we failed emit an escaping closure error.
moveChecker.diagnosticEmitter
.emitAddressEscapingClosureCaptureLoadedAndConsumed(markedValue);
}

return true;
}

Expand Down
14 changes: 10 additions & 4 deletions lib/SILOptimizer/Mandatory/MoveOnlyDiagnostics.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -775,7 +775,7 @@ void DiagnosticEmitter::emitObjectInstConsumesAndUsesValue(
registerDiagnosticEmitted(markedValue);
}

void DiagnosticEmitter::emitAddressEscapingClosureCaptureLoadedAndConsumed(
bool DiagnosticEmitter::emitGlobalOrClassFieldLoadedAndConsumed(
MarkMustCheckInst *markedValue) {
SmallString<64> varName;
getVariableNameForValue(markedValue, varName);
Expand All @@ -789,7 +789,7 @@ void DiagnosticEmitter::emitAddressEscapingClosureCaptureLoadedAndConsumed(
diag::sil_movechecking_notconsumable_but_assignable_was_consumed,
varName, /*isGlobal=*/false);
registerDiagnosticEmitted(markedValue);
return;
return true;
}

// is it a global?
Expand All @@ -799,10 +799,16 @@ void DiagnosticEmitter::emitAddressEscapingClosureCaptureLoadedAndConsumed(
diag::sil_movechecking_notconsumable_but_assignable_was_consumed,
varName, /*isGlobal=*/true);
registerDiagnosticEmitted(markedValue);
return;
return true;
}

// remaining cases must be a closure capture.
return false;
}

void DiagnosticEmitter::emitAddressEscapingClosureCaptureLoadedAndConsumed(
MarkMustCheckInst *markedValue) {
SmallString<64> varName;
getVariableNameForValue(markedValue, varName);
diagnose(markedValue->getModule().getASTContext(),
markedValue,
diag::sil_movechecking_capture_consumed,
Expand Down
11 changes: 11 additions & 0 deletions lib/SILOptimizer/Mandatory/MoveOnlyDiagnostics.h
Original file line number Diff line number Diff line change
Expand Up @@ -160,8 +160,19 @@ class DiagnosticEmitter {
Operand *consumingUse,
Operand *nonConsumingUse);

/// Emit a diagnostic for a case where we have one of the following cases:
///
/// 1. A partial_apply formed from a borrowed address only value.
/// 2. A use of a captured value in a closure callee.
void emitAddressEscapingClosureCaptureLoadedAndConsumed(
MarkMustCheckInst *markedValue);

/// Try to emit a diagnostic for a load/consume from an
/// assignable_but_not_consumable access to a global or a class field. Returns
/// false if we did not find something we pattern matched as being either of
/// those cases. Returns true if we emitted a diagnostic.
bool emitGlobalOrClassFieldLoadedAndConsumed(MarkMustCheckInst *markedValue);

void emitPromotedBoxArgumentError(MarkMustCheckInst *markedValue,
SILFunctionArgument *arg);

Expand Down
19 changes: 19 additions & 0 deletions test/SILOptimizer/moveonly_addresschecker_diagnostics.swift
Original file line number Diff line number Diff line change
Expand Up @@ -2476,6 +2476,25 @@ public func addressOnlyGenericAccessConsumeGrandFieldArg4a<T>(_ x2: consuming Ad
}
}

public func addressOnlyGenericBorrowingConsume<T>(_ x: borrowing AddressOnlyGeneric<T>) {
// expected-error @-1 {{'x' is borrowed and cannot be consumed}}
let _ = x // expected-note {{consumed here}}
}

public func addressOnlyGenericBorrowingConsumeField<T>(_ x: borrowing AddressOnlyGeneric<T>) {
// expected-error @-1 {{'x' is borrowed and cannot be consumed}}
let _ = x.moveOnly // expected-note {{consumed here}}
}

public func addressOnlyGenericBorrowingConsumeField2<T>(_ x: borrowing AddressOnlyGeneric<T>) {
let _ = x.copyable
}

public func addressOnlyGenericBorrowingConsumeGrandField<T>(_ x: borrowing AddressOnlyGeneric<T>) {
// expected-error @-1 {{'x' is borrowed and cannot be consumed}}
let _ = x.moveOnly.k // expected-note {{consumed here}}
}

extension AddressOnlyGeneric {
func testNoUseSelf() { // expected-error {{'self' is borrowed and cannot be consumed}}
let x = self // expected-note {{consumed here}}
Expand Down
2 changes: 2 additions & 0 deletions test/SILOptimizer/moveonly_partial_consumption.swift
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,8 @@ func addressOnlyTestArg(_ x: borrowing AddressOnlyType) {
// expected-error @-1 {{'x' is borrowed and cannot be consumed}}
// expected-error @-2 {{'x' is borrowed and cannot be consumed}}
// expected-error @-3 {{'x' is borrowed and cannot be consumed}}
// expected-error @-4 {{'x' is borrowed and cannot be consumed}}
let _ = x.e // expected-note {{consumed here}}
let _ = x.k
let _ = x.l.e // expected-note {{consumed here}}
let _ = x.l.k // expected-note {{consumed here}}
Expand Down