Skip to content

[TypeChecker] SE-0213: Implement literal init via coercion #17860

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 5 commits into from
Jul 18, 2018
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
28 changes: 28 additions & 0 deletions include/swift/AST/Expr.h
Original file line number Diff line number Diff line change
Expand Up @@ -4306,6 +4306,11 @@ class IsExpr : public CheckedCastExpr {
///
/// Spelled 'a as T' and produces a value of type 'T'.
class CoerceExpr : public ExplicitCastExpr {
/// Since there is already `asLoc` location,
/// we use it to store `start` of the initializer
/// call source range to save some storage.
SourceLoc InitRangeEnd;

public:
CoerceExpr(Expr *sub, SourceLoc asLoc, TypeLoc type)
: ExplicitCastExpr(ExprKind::Coerce, sub, asLoc, type, type.getType())
Expand All @@ -4315,6 +4320,29 @@ class CoerceExpr : public ExplicitCastExpr {
: CoerceExpr(nullptr, asLoc, type)
{ }

private:
CoerceExpr(SourceRange initRange, Expr *literal, TypeLoc type)
: ExplicitCastExpr(ExprKind::Coerce, literal, initRange.Start,
type, type.getType()), InitRangeEnd(initRange.End)
{ setImplicit(); }

public:
/// Create an implicit coercion expression for literal initialization
/// preserving original source information, this way original call
/// could be recreated if needed.
static CoerceExpr *forLiteralInit(ASTContext &ctx, Expr *literal,
SourceRange range, TypeLoc literalType) {
return new (ctx) CoerceExpr(range, literal, literalType);
}

bool isLiteralInit() const { return InitRangeEnd.isValid(); }

SourceRange getSourceRange() const {
return isLiteralInit()
? SourceRange(getAsLoc(), InitRangeEnd)
: ExplicitCastExpr::getSourceRange();
}

static bool classof(const Expr *E) {
return E->getKind() == ExprKind::Coerce;
}
Expand Down
30 changes: 30 additions & 0 deletions lib/Sema/CSApply.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3621,6 +3621,36 @@ namespace {

auto &tc = cs.getTypeChecker();

// Since this is literal initialization, we don't
// really need to keep wrapping coercion around.
if (expr->isLiteralInit()) {
auto *literalInit = expr->getSubExpr();
// If literal got converted into constructor call
// lets put proper source information in place.
if (auto *call = dyn_cast<CallExpr>(literalInit)) {
call->getFn()->forEachChildExpr([&](Expr *subExpr) -> Expr * {
auto *TE = dyn_cast<TypeExpr>(subExpr);
if (!TE)
return subExpr;

auto type = TE->getInstanceType(
[&](const Expr *expr) { return cs.hasType(expr); },
[&](const Expr *expr) { return cs.getType(expr); });

assert(!type->hasError());

if (!type->isEqual(toType))
return subExpr;

return cs.cacheType(new (tc.Context)
TypeExpr(expr->getCastTypeLoc()));
});
}

literalInit->setImplicit(false);
return literalInit;
}

// Turn the subexpression into an rvalue.
auto rvalueSub = cs.coerceToRValue(expr->getSubExpr());
expr->setSubExpr(rvalueSub);
Expand Down
5 changes: 4 additions & 1 deletion lib/Sema/CSDiag.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2814,7 +2814,10 @@ static bool tryIntegerCastFixIts(InFlightDiagnostic &diag, ConstraintSystem &CS,
if (!isIntegerType(fromType, CS) || !isIntegerType(toType, CS))
return false;

auto getInnerCastedExpr = [&]() -> Expr* {
auto getInnerCastedExpr = [&]() -> Expr * {
if (auto *CE = dyn_cast<CoerceExpr>(expr))
return CE->getSubExpr();

auto *CE = dyn_cast<CallExpr>(expr);
if (!CE)
return nullptr;
Expand Down
7 changes: 4 additions & 3 deletions lib/Sema/CSGen.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1394,11 +1394,12 @@ namespace {
Type visitTypeExpr(TypeExpr *E) {
Type type;
// If this is an implicit TypeExpr, don't validate its contents.
if (auto *rep = E->getTypeRepr()) {
type = resolveTypeReferenceInExpression(rep);
} else {
if (E->getTypeLoc().wasValidated()) {
type = E->getTypeLoc().getType();
} else if (auto *rep = E->getTypeRepr()) {
type = resolveTypeReferenceInExpression(rep);
}

if (!type || type->hasError()) return Type();

auto locator = CS.getConstraintLocator(E);
Expand Down
4 changes: 3 additions & 1 deletion lib/Sema/CSSolver.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1004,7 +1004,7 @@ void ConstraintSystem::shrink(Expr *expr) {

// Counts the number of overload sets present in the tree so far.
// Note that the traversal is depth-first.
llvm::SmallVector<std::pair<ApplyExpr *, unsigned>, 4> ApplyExprs;
llvm::SmallVector<std::pair<Expr *, unsigned>, 4> ApplyExprs;

// A collection of original domains of all of the expressions,
// so they can be restored in case of failure.
Expand All @@ -1031,6 +1031,8 @@ void ConstraintSystem::shrink(Expr *expr) {
}

if (auto coerceExpr = dyn_cast<CoerceExpr>(expr)) {
if (coerceExpr->isLiteralInit())
ApplyExprs.push_back({coerceExpr, 1});
visitCoerceExpr(coerceExpr);
return {false, expr};
}
Expand Down
67 changes: 67 additions & 0 deletions lib/Sema/TypeCheckConstraints.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -974,6 +974,10 @@ namespace {
/// Simplify a key path expression into a canonical form.
void resolveKeyPathExpr(KeyPathExpr *KPE);

/// Simplify constructs like `UInt32(1)` into `1 as UInt32` if
/// the type conforms to the expected literal protocol.
Expr *simplifyTypeConstructionWithLiteralArg(Expr *E);

public:
PreCheckExpression(TypeChecker &tc, DeclContext *dc, Expr *parent)
: TC(tc), DC(dc), ParentExpr(parent) {}
Expand Down Expand Up @@ -1217,6 +1221,9 @@ namespace {
return KPE;
}

if (auto *simplified = simplifyTypeConstructionWithLiteralArg(expr))
return simplified;

return expr;
}

Expand Down Expand Up @@ -1824,6 +1831,66 @@ void PreCheckExpression::resolveKeyPathExpr(KeyPathExpr *KPE) {
KPE->resolveComponents(TC.Context, components);
}

Expr *PreCheckExpression::simplifyTypeConstructionWithLiteralArg(Expr *E) {
// If constructor call is expected to produce an optional let's not attempt
// this optimization because literal initializers aren't failable.
if (!TC.getLangOpts().isSwiftVersionAtLeast(5)) {
if (!ExprStack.empty()) {
auto *parent = ExprStack.back();
if (isa<BindOptionalExpr>(parent) || isa<ForceValueExpr>(parent))
return nullptr;
}
}

auto *call = dyn_cast<CallExpr>(E);
if (!call || call->getNumArguments() != 1)
return nullptr;

auto *typeExpr = dyn_cast<TypeExpr>(call->getFn());
if (!typeExpr)
return nullptr;

auto *argExpr = call->getArg()->getSemanticsProvidingExpr();
auto *literal = dyn_cast<LiteralExpr>(argExpr);
if (!literal)
return nullptr;

auto *protocol = TC.getLiteralProtocol(literal);
if (!protocol)
return nullptr;

Type type;
if (typeExpr->getTypeLoc().wasValidated()) {
type = typeExpr->getTypeLoc().getType();
} else if (auto *rep = typeExpr->getTypeRepr()) {
TypeResolutionOptions options;
options |= TypeResolutionFlags::AllowUnboundGenerics;
options |= TypeResolutionFlags::InExpression;
type = TC.resolveType(rep, DC, options);
typeExpr->getTypeLoc().setType(type);
}

if (!type)
return nullptr;

// Don't bother to convert deprecated selector syntax.
if (auto selectorTy = TC.getObjCSelectorType(DC)) {
Copy link
Member

Choose a reason for hiding this comment

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

This is the use of string literals for selectors? I wonder if we can (separately) kill that now...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's right, it's #selector(...) vs. Selector("...")

if (type->isEqual(selectorTy))
return nullptr;
}

ConformanceCheckOptions options;
options |= ConformanceCheckFlags::InExpression;
options |= ConformanceCheckFlags::SkipConditionalRequirements;

auto result = TC.conformsToProtocol(type, protocol, DC, options);
if (!result || !result->isConcrete())
return nullptr;

return CoerceExpr::forLiteralInit(TC.Context, argExpr, call->getSourceRange(),
typeExpr->getTypeLoc());
}

/// \brief Clean up the given ill-formed expression, removing any references
/// to type variables and setting error types on erroneous expression nodes.
void CleanupIllFormedExpressionRAII::doIt(Expr *expr) {
Expand Down
8 changes: 5 additions & 3 deletions stdlib/public/core/Integers.swift.gyb
Original file line number Diff line number Diff line change
Expand Up @@ -3591,10 +3591,12 @@ ${assignmentOperatorComment(x.operator, True)}
/// to find an absolute value. In addition, because `abs(_:)` always returns
/// a value of the same type, even in a generic context, using the function
/// instead of the `magnitude` property is encouraged.
@_transparent
public var magnitude: U${Self} {
let base = U${Self}(_value)
return self < (0 as ${Self}) ? ~base + 1 : base
@inline(__always)
get {
Copy link
Member

Choose a reason for hiding this comment

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

This seems unrelated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is needed because constant propagation now looks at both branches, so as a way to work around that behavior @ravikandhadai and @moiseev told me that it would be appropriate to switch from @_transparent to @inlineable.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is, unfortunately. @_transparent here causes the compile-time overflow on a branch that would not be taken at runtime.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just to clarify. The changes to how initializers are resolved, as a side-effect, makes constant propagation more precise as it can now propagate through more initializers. While in general it is a good thing, in this specific case, it resulted in a false positive due to mandatory inlining.

Copy link
Collaborator

@xwu xwu Jul 17, 2018

Choose a reason for hiding this comment

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

Why not write return self < (0 as ${Self}) ? ~base &+ 1 : base? You are, after all, performing a bit twiddling operation.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is very much possible (and probably reasonable) to try the &+ approach in a separate PR once this lands.

Copy link
Contributor

Choose a reason for hiding this comment

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

@moiseev @xwu there is one other issue with "magnitude" (besides the +) that could trigger static errors if it is made @_transparent. It is passing a potentially negative (builtin Int) into a UInt : let base = U${Self}(_value). This could trigger a static error on negative values like (-1).magnitude etc. So perhaps keeping it @inline is better.

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, _value is the same type regardless of whether it's Int or UInt, and that initializer simply assigns self._value = other._value.

Copy link
Contributor

Choose a reason for hiding this comment

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

@xwu Ah okay, this initializer should not be a problem for constant prop. There will no static errors here. Okay we can try making + to &+

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm going to open follow up PR for this and run the benchmarks

let base = U${Self}(_value)
return self < (0 as ${Self}) ? ~base + 1 : base
}
}
% end

Expand Down
2 changes: 1 addition & 1 deletion test/Constraints/diagnostics.swift
Original file line number Diff line number Diff line change
Expand Up @@ -951,7 +951,7 @@ func SR_6272_a() {
case bar
}

// expected-error@+2 {{binary operator '*' cannot be applied to operands of type 'Int' and 'Float'}} {{35-41=}} {{42-43=}}
// expected-error@+2 {{binary operator '*' cannot be applied to operands of type 'Int' and 'Float'}} {{35-35=Int(}} {{42-42=)}}
// expected-note@+1 {{expected an argument list of type '(Int, Int)'}}
let _: Int = Foo.bar.rawValue * Float(0)

Expand Down
20 changes: 20 additions & 0 deletions test/Constraints/optional.swift
Original file line number Diff line number Diff line change
Expand Up @@ -272,3 +272,23 @@ class Bar {
func rdar37508855(_ e1: X?, _ e2: X?) -> [X] {
return [e1, e2].filter { $0 == nil }.map { $0! }
}

func se0213() {
struct Q: ExpressibleByStringLiteral {
typealias StringLiteralType = String

var foo: String

init?(_ possibleQ: StringLiteralType) {
return nil
}

init(stringLiteral str: StringLiteralType) {
self.foo = str
}
}

_ = Q("why")?.foo // Ok
_ = Q("who")!.foo // Ok
_ = Q?("how") // Ok
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,5 @@
// These are specific to Swift 4.

func testArithmeticOverflowSwift4() {
var _ = Int8(126) + (1 + 1) // FIXME: Should expect an integer overflow
// error but none happens now (see <rdar://problem/39120081>)
var _ = Int8(126) + (1 + 1) // expected-error {{arithmetic operation '126 + 2' (on type 'Int8') results in an overflow}}
}
Original file line number Diff line number Diff line change
Expand Up @@ -109,8 +109,5 @@ func testIntToFloatConversion() {
let e2: Float80 = 18_446_744_073_709_551_617 // expected-warning {{'18446744073709551617' is not exactly representable as 'Float80'; it becomes '18446744073709551616'}}
_blackHole(e2)

// No warnings are emitted for conversion through explicit constructor calls.
// Note that the error here is because of an implicit conversion of the input
// literal to 'Int'.
_blackHole(Float80(18_446_744_073_709_551_617)) // expected-error {{integer literal '18446744073709551617' overflows when stored into 'Int'}}
_blackHole(Float80(18_446_744_073_709_551_617)) // Ok
Copy link
Contributor

Choose a reason for hiding this comment

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

@xedin Nice to see that the above FIXME's are addressed by this fix. Thanks!

}
Original file line number Diff line number Diff line change
Expand Up @@ -234,8 +234,5 @@ func testArithmeticOverflow_UInt_32bit() {
}

func testIntToFloatConversion() {
// No warnings are emitted for conversion through explicit constructor calls.
// Note that the error here is because of an implicit conversion of the input
// literal to 'Int', which is 32 bits in arch32.
_blackHole(Double(9_007_199_254_740_993)) // expected-error {{integer literal '9007199254740993' overflows when stored into 'Int'}}
_blackHole(Double(9_007_199_254_740_993)) // Ok
}
7 changes: 7 additions & 0 deletions test/expr/expressions.swift
Original file line number Diff line number Diff line change
Expand Up @@ -910,3 +910,10 @@ Sr3439: do {
let obj = C();
_ = obj[#column] // Ok.
}

// rdar://problem/23672697 - No way to express literal integers larger than Int without using type ascription
let _: Int64 = 0xFFF_FFFF_FFFF_FFFF
let _: Int64 = Int64(0xFFF_FFFF_FFFF_FFFF)
let _: Int64 = 0xFFF_FFFF_FFFF_FFFF as Int64
let _ = Int64(0xFFF_FFFF_FFFF_FFFF)
let _ = 0xFFF_FFFF_FFFF_FFFF as Int64
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// RUN: %scale-test --invert-result --begin 1 --end 5 --step 1 --select incrementScopeCounter %s
// RUN: %scale-test --begin 1 --end 10 --step 1 --select incrementScopeCounter %s
// REQUIRES: OS=macosx
// REQUIRES: asserts
public enum E
Expand Down