Skip to content

Small cleanups from "optimize optional conversion" #79619

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
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
7 changes: 0 additions & 7 deletions include/swift/AST/DiagnosticsSema.def
Original file line number Diff line number Diff line change
Expand Up @@ -7523,13 +7523,6 @@ ERROR(result_builder_buildpartialblock_accumulated_not_accessible,none,
"expression shuffles the elements of this tuple; "
"this behavior is deprecated", ())

//------------------------------------------------------------------------------
// MARK: Implicit conversion diagnostics
//------------------------------------------------------------------------------
ERROR(cannot_implicitly_convert_in_optional_context,none,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For posterity: this is going to come back because it's still a useful warning to have.

"cannot implicitly convert value of type %0 to expected type %1",
(Type, Type))

//------------------------------------------------------------------------------
// MARK: marker protocol diagnostics
//------------------------------------------------------------------------------
Expand Down
2 changes: 1 addition & 1 deletion include/swift/AST/SimpleRequest.h
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ enum class RequestFlags {
/// incremental dependency pair and defines a new dependency scope.
///
/// This bit is optional. High-level requests
/// (e.g. \c TypeCheckSourceFileRequest) will require it.
/// (e.g. \c TypeCheckPrimaryFileRequest) will require it.
///
/// For further discussion on incremental dependencies
/// see DependencyAnalysis.md.
Expand Down
4 changes: 2 additions & 2 deletions include/swift/AST/TypeCheckRequests.h
Original file line number Diff line number Diff line change
Expand Up @@ -3300,9 +3300,9 @@ class ApplyAccessNoteRequest
void cacheResult(evaluator::SideEffect value) const;
};

class TypeCheckSourceFileRequest
class TypeCheckPrimaryFileRequest
: public SimpleRequest<
TypeCheckSourceFileRequest, evaluator::SideEffect(SourceFile *),
TypeCheckPrimaryFileRequest, evaluator::SideEffect(SourceFile *),
RequestFlags::SeparatelyCached | RequestFlags::DependencySource> {
public:
using SimpleRequest::SimpleRequest;
Expand Down
2 changes: 1 addition & 1 deletion include/swift/AST/TypeCheckerTypeIDZone.def
Original file line number Diff line number Diff line change
Expand Up @@ -424,7 +424,7 @@ SWIFT_REQUEST(TypeChecker, HasDefaultInitRequest,
bool(NominalTypeDecl *), Cached, NoLocationInfo)
SWIFT_REQUEST(TypeChecker, SynthesizeDefaultInitRequest,
ConstructorDecl *(NominalTypeDecl *), Cached, NoLocationInfo)
SWIFT_REQUEST(TypeChecker, TypeCheckSourceFileRequest,
SWIFT_REQUEST(TypeChecker, TypeCheckPrimaryFileRequest,
bool(SouceFile *), SeparatelyCached, NoLocationInfo)
SWIFT_REQUEST(TypeChecker, ConformanceAccessScopeRequest,
ConformanceAccessScope(DeclContext *, ProtocolDecl *),
Expand Down
2 changes: 1 addition & 1 deletion include/swift/Sema/ConstraintLocator.h
Original file line number Diff line number Diff line change
Expand Up @@ -1275,7 +1275,7 @@ class ConstraintLocatorBuilder {

auto last = std::find_if(
path.rbegin(), path.rend(), [](LocatorPathElt &elt) -> bool {
return elt.getKind() != ConstraintLocator::OptionalPayload &&
return elt.getKind() != ConstraintLocator::OptionalInjection &&
elt.getKind() != ConstraintLocator::GenericArgument;
});

Expand Down
4 changes: 2 additions & 2 deletions include/swift/Sema/ConstraintLocatorPathElts.def
Original file line number Diff line number Diff line change
Expand Up @@ -153,8 +153,8 @@ CUSTOM_LOCATOR_PATH_ELT(OpenedGeneric)
/// type at the base of the locator.
CUSTOM_LOCATOR_PATH_ELT(OpenedOpaqueArchetype)

/// An optional payload.
SIMPLE_LOCATOR_PATH_ELT(OptionalPayload)
/// The optional payload in an optional injection, ie a T -> T? conversion.
SIMPLE_LOCATOR_PATH_ELT(OptionalInjection)

/// The parent of a nested type.
SIMPLE_LOCATOR_PATH_ELT(ParentType)
Expand Down
8 changes: 4 additions & 4 deletions lib/AST/TypeCheckRequests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1655,24 +1655,24 @@ void RenamedDeclRequest::cacheResult(ValueDecl *value) const {
}

//----------------------------------------------------------------------------//
// TypeCheckSourceFileRequest computation.
// TypeCheckPrimaryFileRequest computation.
//----------------------------------------------------------------------------//

evaluator::DependencySource TypeCheckSourceFileRequest::readDependencySource(
evaluator::DependencySource TypeCheckPrimaryFileRequest::readDependencySource(
const evaluator::DependencyRecorder &e) const {
return std::get<0>(getStorage());
}

std::optional<evaluator::SideEffect>
TypeCheckSourceFileRequest::getCachedResult() const {
TypeCheckPrimaryFileRequest::getCachedResult() const {
auto *SF = std::get<0>(getStorage());
if (SF->ASTStage == SourceFile::TypeChecked)
return std::make_tuple<>();

return std::nullopt;
}

void TypeCheckSourceFileRequest::cacheResult(evaluator::SideEffect) const {
void TypeCheckPrimaryFileRequest::cacheResult(evaluator::SideEffect) const {
auto *SF = std::get<0>(getStorage());

// Verify that we've checked types correctly.
Expand Down
18 changes: 1 addition & 17 deletions lib/Sema/CSDiagnostics.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -940,7 +940,7 @@ bool GenericArgumentsMismatchFailure::diagnoseAsError() {

while (!path.empty()) {
auto last = path.back();
if (last.is<LocatorPathElt::OptionalPayload>() ||
if (last.is<LocatorPathElt::OptionalInjection>() ||
last.is<LocatorPathElt::GenericType>() ||
last.is<LocatorPathElt::GenericArgument>()) {
path = path.drop_back();
Expand Down Expand Up @@ -2837,22 +2837,6 @@ bool ContextualFailure::diagnoseAsError() {
break;
}

case ConstraintLocator::OptionalPayload: {
// If this is an attempt at a Double <-> CGFloat conversion
// through optional chaining, let's produce a tailored diagnostic.
if (isExpr<OptionalEvaluationExpr>(getAnchor())) {
if ((fromType->isDouble() || fromType->isCGFloat()) &&
(toType->isDouble() || toType->isCGFloat())) {
fromType = OptionalType::get(fromType);
toType = OptionalType::get(toType);
diagnostic = diag::cannot_implicitly_convert_in_optional_context;
break;
}
}

return false;
}

case ConstraintLocator::EnumPatternImplicitCastMatch: {
// In this case, the types are reversed, as we are checking whether we
// can convert the pattern type to the context type.
Expand Down
2 changes: 1 addition & 1 deletion lib/Sema/CSGen.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4672,7 +4672,7 @@ generateForEachStmtConstraints(ConstraintSystem &cs, DeclContext *dc,
// Add a conversion constraint between the element type of the sequence
// and the type of the element pattern.
auto *elementTypeLoc = cs.getConstraintLocator(
elementLocator, ConstraintLocator::OptionalPayload);
elementLocator, ConstraintLocator::OptionalInjection);
auto elementType = cs.createTypeVariable(elementTypeLoc,
/*flags=*/0);
{
Expand Down
24 changes: 12 additions & 12 deletions lib/Sema/CSSimplify.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3323,11 +3323,11 @@ ConstraintSystem::matchFunctionTypes(FunctionType *func1, FunctionType *func2,
SmallVector<LocatorPathElt, 4> path;
locator.getLocatorParts(path);

// Find the last path element, skipping OptionalPayload elements
// Find the last path element, skipping OptionalInjection elements
// so that we allow this exception in cases of optional injection.
auto last = std::find_if(
path.rbegin(), path.rend(), [](LocatorPathElt &elt) -> bool {
return elt.getKind() != ConstraintLocator::OptionalPayload;
return elt.getKind() != ConstraintLocator::OptionalInjection;
});

auto &ctx = getASTContext();
Expand Down Expand Up @@ -3431,12 +3431,12 @@ ConstraintSystem::matchFunctionTypes(FunctionType *func1, FunctionType *func2,

// Find the last path element, skipping GenericArgument elements
// so that we allow this exception in cases of optional types, and
// skipping OptionalPayload elements so that we allow this
// skipping OptionalInjection elements so that we allow this
// exception in cases of optional injection.
auto last = std::find_if(
path.rbegin(), path.rend(), [](LocatorPathElt &elt) -> bool {
return elt.getKind() != ConstraintLocator::GenericArgument &&
elt.getKind() != ConstraintLocator::OptionalPayload;
elt.getKind() != ConstraintLocator::OptionalInjection;
});

if (last != path.rend()) {
Expand Down Expand Up @@ -3484,7 +3484,7 @@ ConstraintSystem::matchFunctionTypes(FunctionType *func1, FunctionType *func2,
//
// func foo(_: ((Int, Int) -> Void)?) {}
// _ = foo { _ in } <- missing second closure parameter.
if (loc->isLastElement<LocatorPathElt::OptionalPayload>()) {
if (loc->isLastElement<LocatorPathElt::OptionalInjection>()) {
auto path = loc->getPath();
loc = getConstraintLocator(loc->getAnchor(), path.drop_back());
}
Expand Down Expand Up @@ -4144,7 +4144,7 @@ ConstraintSystem::matchExistentialTypes(Type type1, Type type2,
// `value-to-optional` or `optional-to-optional` conversion
// associated with them (expected argument is `AnyObject?`).
if (!path.empty() &&
(path.back().is<LocatorPathElt::OptionalPayload>() ||
(path.back().is<LocatorPathElt::OptionalInjection>() ||
path.back().is<LocatorPathElt::GenericArgument>()))
path.pop_back();

Expand Down Expand Up @@ -4200,7 +4200,7 @@ ConstraintSystem::matchExistentialTypes(Type type1, Type type2,
// If the path ends at `optional payload` it means that this
// check is part of an implicit value-to-optional conversion,
// and it could be safely dropped.
if (!path.empty() && path.back().is<LocatorPathElt::OptionalPayload>())
if (!path.empty() && path.back().is<LocatorPathElt::OptionalInjection>())
path.pop_back();

// Determine whether this conformance mismatch is
Expand Down Expand Up @@ -6495,7 +6495,7 @@ bool ConstraintSystem::repairFailures(
// If the mismatch is a part of either optional-to-optional or
// value-to-optional conversions, let's allow fix refer to a complete
// top level type and not just a part of it.
if (tupleLocator->findLast<LocatorPathElt::OptionalPayload>())
if (tupleLocator->findLast<LocatorPathElt::OptionalInjection>())
break;

if (tupleLocator->isForContextualType()) {
Expand Down Expand Up @@ -6636,7 +6636,7 @@ bool ConstraintSystem::repairFailures(
break;
}

case ConstraintLocator::OptionalPayload: {
case ConstraintLocator::OptionalInjection: {
if (lhs->isPlaceholder() || rhs->isPlaceholder())
return true;

Expand Down Expand Up @@ -7446,7 +7446,7 @@ ConstraintSystem::matchTypes(Type type1, Type type2, ConstraintKind kind,
// Look through all value-to-optional promotions to allow
// conversions like Double -> CGFloat?? and vice versa.
// T -> Optional<T>
if (location.endsWith<LocatorPathElt::OptionalPayload>() ||
if (location.endsWith<LocatorPathElt::OptionalInjection>() ||
location.endsWith<LocatorPathElt::GenericArgument>()) {
SmallVector<LocatorPathElt, 4> path;
auto anchor = location.getLocatorParts(path);
Expand All @@ -7456,7 +7456,7 @@ ConstraintSystem::matchTypes(Type type1, Type type2, ConstraintKind kind,
path.erase(llvm::remove_if(
path,
[](const LocatorPathElt &elt) {
return elt.is<LocatorPathElt::OptionalPayload>() ||
return elt.is<LocatorPathElt::OptionalInjection>() ||
elt.is<LocatorPathElt::GenericArgument>();
}),
path.end());
Expand Down Expand Up @@ -14608,7 +14608,7 @@ ConstraintSystem::simplifyRestrictedConstraintImpl(
if (generic2->getDecl()->isOptionalDecl()) {
auto result = matchTypes(
type1, generic2->getGenericArgs()[0], matchKind, subflags,
locator.withPathElement(ConstraintLocator::OptionalPayload));
locator.withPathElement(ConstraintLocator::OptionalInjection));

if (!(shouldAttemptFixes() && result.isFailure()))
return result;
Expand Down
6 changes: 3 additions & 3 deletions lib/Sema/ConstraintLocator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ unsigned LocatorPathElt::getNewSummaryFlags() const {
case ConstraintLocator::ResultBuilderBodyResult:
case ConstraintLocator::InstanceType:
case ConstraintLocator::AutoclosureResult:
case ConstraintLocator::OptionalPayload:
case ConstraintLocator::OptionalInjection:
case ConstraintLocator::Member:
case ConstraintLocator::MemberRefBase:
case ConstraintLocator::UnresolvedMember:
Expand Down Expand Up @@ -176,8 +176,8 @@ void LocatorPathElt::dump(raw_ostream &out) const {
out << "apply function";
break;

case ConstraintLocator::OptionalPayload:
out << "optional payload";
case ConstraintLocator::OptionalInjection:
out << "optional injection";
break;

case ConstraintLocator::ApplyArgToParam: {
Expand Down
8 changes: 4 additions & 4 deletions lib/Sema/ConstraintSystem.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -644,7 +644,7 @@ ConstraintLocator *ConstraintSystem::getImplicitValueConversionLocator(
// Drop any value-to-optional conversions that were applied along the
// way to reach this one.
while (!path.empty()) {
if (path.back().is<LocatorPathElt::OptionalPayload>()) {
if (path.back().is<LocatorPathElt::OptionalInjection>()) {
path.pop_back();
continue;
}
Expand Down Expand Up @@ -3772,7 +3772,7 @@ void constraints::simplifyLocator(ASTNode &anchor,

case ConstraintLocator::Witness:
case ConstraintLocator::WrappedValue:
case ConstraintLocator::OptionalPayload:
case ConstraintLocator::OptionalInjection:
case ConstraintLocator::ImplicitlyUnwrappedDisjunctionChoice:
case ConstraintLocator::FallbackType:
case ConstraintLocator::KeyPathSubscriptIndex:
Expand Down Expand Up @@ -4068,7 +4068,7 @@ Solution::getFunctionArgApplyInfo(ConstraintLocator *locator) const {
// Look for the apply-arg-to-param element in the locator's path. We may
// have to look through other elements that are generated from an argument
// conversion such as GenericArgument for an optional-to-optional conversion,
// and OptionalPayload for a value-to-optional conversion.
// and OptionalInjection for a value-to-optional conversion.
auto iter = path.rbegin();
auto applyArgElt = locator->findLast<LocatorPathElt::ApplyArgToParam>(iter);
if (!applyArgElt)
Expand Down Expand Up @@ -4343,7 +4343,7 @@ bool ConstraintSystem::isArgumentOfImportedDecl(
// locator elements at the end of the path, they came from
// either value-to-optional promotion or optional-to-optional
// conversion.
if (last.is<LocatorPathElt::OptionalPayload>() ||
if (last.is<LocatorPathElt::OptionalInjection>() ||
last.is<LocatorPathElt::GenericArgument>()) {
path.pop_back();
continue;
Expand Down
2 changes: 1 addition & 1 deletion lib/Sema/TypeCheckDeclPrimary.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3991,7 +3991,7 @@ class DeclChecker : public DeclVisitor<DeclChecker> {
return;
}

// Record a dependency from TypeCheckSourceFileRequest to
// Record a dependency from TypeCheckPrimaryFileRequest to
// ExtendedNominalRequest, since the call to getExtendedNominal()
// above doesn't record a dependency when reading a cached value.
ED->computeExtendedNominal();
Expand Down
4 changes: 2 additions & 2 deletions lib/Sema/TypeChecker.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -251,11 +251,11 @@ void swift::performTypeChecking(SourceFile &SF) {
}

return (void)evaluateOrDefault(SF.getASTContext().evaluator,
TypeCheckSourceFileRequest{&SF}, {});
TypeCheckPrimaryFileRequest{&SF}, {});
}

evaluator::SideEffect
TypeCheckSourceFileRequest::evaluate(Evaluator &eval, SourceFile *SF) const {
TypeCheckPrimaryFileRequest::evaluate(Evaluator &eval, SourceFile *SF) const {
assert(SF && "Source file cannot be null!");
assert(SF->ASTStage != SourceFile::TypeChecked &&
"Should not be re-typechecking this file!");
Expand Down
11 changes: 9 additions & 2 deletions test/Constraints/implicit_double_cgfloat_conversion.swift
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ func test_returns_cgfloat(_: Double) -> CGFloat {
let d: Double = 0.0
let cgf: CGFloat = 0.0

// CHECK: test_various_situations_converting_to_cgfloat()
// CHECK-LABEL: sil hidden [ossa] @$s34implicit_double_cgfloat_conversion038test_various_situations_converting_to_C0yyF : $@convention(thin) () -> () {
func test_various_situations_converting_to_cgfloat() {
// CHECK: function_ref @$s12CoreGraphics7CGFloatVyACSdcfC : $@convention(method) (Double, @thin CGFloat.Type) -> CGFloat
let _: CGFloat = d
Expand Down Expand Up @@ -58,6 +58,7 @@ func test_various_situations_converting_to_cgfloat() {
test_to_cgfloat(test_returns_double(d)) // Two conversions

// Overloads with CGFloat are preferred if that allows to avoid any implicit conversions.
// CHECK-LABEL: sil private [ossa] @$s34implicit_double_cgfloat_conversion038test_various_situations_converting_to_C0yyF0E23_loading_tuple_elementsL_6valuesy12CoreGraphics7CGFloatV_AGtz_tF : $@convention(thin) (@inout (CGFloat, CGFloat)) -> () {
func test_loading_tuple_elements(values: inout (CGFloat, CGFloat)) {
struct S {
init(x: Double, y: Double) {}
Expand All @@ -69,7 +70,7 @@ func test_various_situations_converting_to_cgfloat() {
}
}

// CHECK: test_various_situations_converting_to_double()
// CHECK-LABEL: sil hidden [ossa] @$s34implicit_double_cgfloat_conversion038test_various_situations_converting_to_B0yyF : $@convention(thin) () -> () {
func test_various_situations_converting_to_double() {
// function_ref @$sSd12CoreGraphicsEySdAA7CGFloatVcfC : $@convention(method) (CGFloat, @thin Double.Type) -> Double
let _: Double = cgf
Expand Down Expand Up @@ -102,12 +103,14 @@ func test_various_situations_converting_to_double() {
test_from_cgfloat(test_returns_cgfloat(cgf)) // Two conversions - argument and result.
}

// CHECK-LABEL: sil hidden [ossa] @$s34implicit_double_cgfloat_conversion31test_conversions_with_optionals1vy12CoreGraphics7CGFloatVSg_tF : $@convention(thin) (Optional<CGFloat>) -> () {
func test_conversions_with_optionals(v: CGFloat?) {
// CHECK: function_ref @$s34implicit_double_cgfloat_conversion31test_conversions_with_optionals1vy12CoreGraphics7CGFloatVSg_tFAFyKXEfu_
// CHECK: function_ref @$sSd12CoreGraphicsEySdAA7CGFloatVcfC : $@convention(method) (CGFloat, @thin Double.Type) -> Double
let _: Double = (v ?? 0)
}

// CHECK-LABEL: sil hidden [ossa] @$s34implicit_double_cgfloat_conversion48test_static_members_are_contextually_convertibleyyF : $@convention(thin) () -> () {
func test_static_members_are_contextually_convertible() {
struct S {
static var testProp: CGFloat { 42 }
Expand All @@ -125,6 +128,7 @@ func test_static_members_are_contextually_convertible() {
}
}

// CHECK-LABEL: sil hidden [ossa] @$s34implicit_double_cgfloat_conversion25test_narrowing_is_delayed1x1yySd_12CoreGraphics7CGFloatVtF : $@convention(thin) (Double, CGFloat) -> () {
func test_narrowing_is_delayed(x: Double, y: CGFloat) {
func test(_: CGFloat) {}

Expand Down Expand Up @@ -181,6 +185,7 @@ extension CGFloat {
}

// Make sure that solution with no Double/CGFloat conversions is preferred
// CHECK-LABEL: sil hidden [ossa] @$s34implicit_double_cgfloat_conversion38test_no_ambiguity_with_unary_operators5width6heighty12CoreGraphics7CGFloatV_AGtF : $@convention(thin) (CGFloat, CGFloat) -> () {
func test_no_ambiguity_with_unary_operators(width: CGFloat, height: CGFloat) {
struct R {
init(x: CGFloat, y: CGFloat, width: CGFloat, height: CGFloat) {}
Expand All @@ -193,6 +198,7 @@ func test_no_ambiguity_with_unary_operators(width: CGFloat, height: CGFloat) {
_ = R(x: width / 4, y: -height / 2, width: width, height: height)
}

// CHECK-LABEL: sil hidden [ossa] @$s34implicit_double_cgfloat_conversion40test_conversions_with_optional_promotion1d3cgfySd_12CoreGraphics7CGFloatVtF : $@convention(thin) (Double, CGFloat) -> () {
func test_conversions_with_optional_promotion(d: Double, cgf: CGFloat) {
func test_double(_: Double??, _: Double???) {}
func test_cgfloat(_: CGFloat??, _: CGFloat???) {}
Expand Down Expand Up @@ -332,6 +338,7 @@ func test_implicit_conversion_clash_with_partial_application_check() {
}

// rdar://99352676
// CHECK-LABEL: sil hidden [ossa] @$s34implicit_double_cgfloat_conversion20test_init_validationyyF : $@convention(thin) () -> () {
func test_init_validation() {
class Foo {
static let bar = 100.0
Expand Down
2 changes: 1 addition & 1 deletion test/Frontend/debug-cycles.swift
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ class Outer2: Outer2.Inner {
class Inner {}
}
// CHECK:===CYCLE DETECTED===
// CHECK-NEXT: `--TypeCheckSourceFileRequest({{.*}})
// CHECK-NEXT: `--TypeCheckPrimaryFileRequest({{.*}})
// CHECK-NEXT: `--SuperclassDeclRequest({{.*}})
// CHECK-NEXT: `--InheritedDeclsReferencedRequest({{.*}})
// CHECK-NEXT: `--QualifiedLookupRequest({{.*}})
Expand Down