Skip to content
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

Convert InterpolatedStringLiteralExpr to not use tc.callWitness(). #26076

Merged
merged 1 commit into from Jul 16, 2019

Conversation

Projects
None yet
4 participants
@pschuh
Copy link
Collaborator

commented Jul 10, 2019

For reference, all other callers of callWitness have been migrated.

@pschuh pschuh requested a review from slavapestov Jul 10, 2019

@pschuh pschuh force-pushed the pschuh:s-10 branch from 7df662d to fc5b176 Jul 10, 2019

@jrose-apple jrose-apple requested a review from brentdax Jul 10, 2019

@brentdax
Copy link
Collaborator

left a comment

This is such a great change! I have a few comments, but they're fairly superficial.

Show resolved Hide resolved include/swift/AST/Expr.h
Show resolved Hide resolved lib/SILGen/SILGenExpr.cpp
Show resolved Hide resolved lib/Sema/CSApply.cpp Outdated
Show resolved Hide resolved lib/Sema/CSApply.cpp
Show resolved Hide resolved lib/Sema/CSApply.cpp Outdated
Show resolved Hide resolved lib/Sema/CSApply.cpp Outdated
Show resolved Hide resolved include/swift/AST/Expr.h Outdated
Show resolved Hide resolved lib/Sema/CSGen.cpp
Show resolved Hide resolved lib/Sema/CSApply.cpp Outdated
Show resolved Hide resolved lib/SILGen/SILGenExpr.cpp Outdated

@pschuh pschuh force-pushed the pschuh:s-10 branch 2 times, most recently from 8bfb211 to 7f561b7 Jul 11, 2019

@pschuh pschuh requested a review from brentdax Jul 11, 2019

@slavapestov
Copy link
Member

left a comment

This is great!

On a related note I saw that ObjectLiteralExpr still stores a SemanticExpr but it's not used. I'm going to remove it.

@pschuh

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 12, 2019

@swift-ci please test

@brentdax
Copy link
Collaborator

left a comment

This is much improved—I only have a few more refinements to suggest.

Show resolved Hide resolved include/swift/AST/Expr.h
Show resolved Hide resolved include/swift/AST/Expr.h
Show resolved Hide resolved lib/Sema/CSApply.cpp Outdated
Show resolved Hide resolved lib/Sema/CSApply.cpp Outdated

@pschuh pschuh force-pushed the pschuh:s-10 branch from 7f561b7 to d276e7e Jul 12, 2019

@pschuh pschuh requested a review from brentdax Jul 12, 2019

@swift-ci

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2019

Build failed
Swift Test Linux Platform
Git Sha - 7f561b7

@swift-ci

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2019

Build failed
Swift Test OS X Platform
Git Sha - 7f561b7

@brentdax
Copy link
Collaborator

left a comment

This is squeaky clean. Thank you!

@pschuh pschuh force-pushed the pschuh:s-10 branch from d276e7e to 18e8bb0 Jul 12, 2019

@pschuh

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 12, 2019

@swift-ci please test

@pschuh pschuh force-pushed the pschuh:s-10 branch from 18e8bb0 to b62449c Jul 12, 2019

@pschuh

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 12, 2019

@swift-ci please test

@swift-ci

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2019

Build failed
Swift Test Linux Platform
Git Sha - 18e8bb0

@brentdax

This comment has been minimized.

Copy link
Collaborator

commented Jul 12, 2019

@pschuh It looks like this now conflicts with the SemanticExpr change Slava mentioned he wanted to make.

@pschuh pschuh force-pushed the pschuh:s-10 branch from b62449c to bfdfc76 Jul 12, 2019

@pschuh

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 12, 2019

@swift-ci please test

@swift-ci

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2019

Build failed
Swift Test Linux Platform
Git Sha - b62449c

@swift-ci

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2019

Build failed
Swift Test OS X Platform
Git Sha - b62449c

@pschuh

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 12, 2019

Having a bit of trouble with it trying to re-type check the generated expr and not understanding the OpaqueValueExpr.

@pschuh pschuh force-pushed the pschuh:s-10 branch from bfdfc76 to 3c92358 Jul 13, 2019

@pschuh

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 13, 2019

@swift-ci please test

@swift-ci

This comment has been minimized.

Copy link
Contributor

commented Jul 13, 2019

Build failed
Swift Test OS X Platform
Git Sha - bfdfc76

@swift-ci

This comment has been minimized.

Copy link
Contributor

commented Jul 13, 2019

Build failed
Swift Test Linux Platform
Git Sha - bfdfc76

@pschuh

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 13, 2019

@slavapestov I noticed that this is the only use case of TapExpr. If I deleted TapExpr, I could attach the var and body directly on the InterpolatedStringExpr. I have a hack that works in CSGen.cc. I'll let you judge.

@slavapestov
Copy link
Member

left a comment

I only have two minor cosmetic comments, otherwise its ready to merge once you fix a small conflict.

I think @brentdax should chime in on the relative merits of merging TapExpr vs keeping it separate. If you do decide to go ahead with that, I would suggest doing it in a separate PR.

Also, feel free to actually delete callWitness(), either here or in a follow-up.

Show resolved Hide resolved lib/Sema/CSGen.cpp
Show resolved Hide resolved lib/SILGen/SILGenExpr.cpp Outdated
Show resolved Hide resolved include/swift/AST/Expr.h Outdated
@brentdax

This comment has been minimized.

Copy link
Collaborator

commented Jul 13, 2019

You can delete TapExpr if you want, but these days I’m thinking we should just fix captures of partially initialized values and generate a closure so that it doesn’t need a specialized mechanism at all, and that change might be a bit easier if TapExpr is still separate. But we don’t know if that will actually happen, and if it doesn’t or it takes a long time, merging would be better.

(@slavapestov I asked @pschuh to remove callWitness() in a separate commit, although two commits in the same PR would be fine!)

@slavapestov

This comment has been minimized.

Copy link
Member

commented Jul 13, 2019

How were you thinking of using TapExpr for partially applied functions? I agree that would be a good change but I was imagining CSApply would build a ClosureExpr, similar to what it does with the keypath to function conversion path.

@brentdax

This comment has been minimized.

Copy link
Collaborator

commented Jul 14, 2019

Not partially applied functions, but partially initialized values. The main reason we don't currently enclose the append statements of a string interpolation in a closure is that things like this won't work with a closure:

let tuple: (Int, Int)
tuple.0 = 42
print("\(tuple.0)")

The closure would try to capture all of tuple, but DI would reject that because tuple.1 is uninitialized.

The tuple example is a bit silly, but the same problem happens in the first phase of an initializer, when you can't capture self yet because some properties are not yet initialized. We decided that regressing on string interpolation support in initializers wasn't acceptable, so I designed TapExpr instead. If it weren't for this issue, I believe we could use a closure instead of TapExpr.

What I'm saying is that, if this capture issue ever goes away, it'll be easier to refactor to use closures if TapExpr is still a separate AST node—we can just drop in a ClosureExpr and delete TapExpr without having to disentangle its SIL lowering from InterpolatedStringLiteralExpr's. Having said that, the problem probably isn't that serious.

@pschuh pschuh force-pushed the pschuh:s-10 branch from 3c92358 to 5c1f289 Jul 15, 2019

@pschuh

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 15, 2019

@swift-ci please test

@swift-ci

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2019

Build failed
Swift Test OS X Platform
Git Sha - 3c92358

@swift-ci

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2019

Build failed
Swift Test Linux Platform
Git Sha - 3c92358

@pschuh pschuh requested a review from slavapestov Jul 15, 2019

@pschuh

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 15, 2019

OpaqueValueExpr re-orders the sil in a way that confuses the constant evaluation. I did the simplest thing of inlining the work for TapExpr. Attaching a closure that computes the RValue instead of the RValue itself as the OpaqueValue would make the old code work.

Convert InterpolatedStringLiteralExpr to not use tc.callWitness().
For reference, all other callers of callWitness have been migrated.

@pschuh pschuh force-pushed the pschuh:s-10 branch from 5c1f289 to 8ad782c Jul 15, 2019

@pschuh

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 15, 2019

@swift-ci please test

@brentdax

This comment has been minimized.

Copy link
Collaborator

commented Jul 15, 2019

@pschuh If you were seeing failures in e.g. IRGen/condfail_message.swift and SILOptimizer/pound_assert.swift, these seem to be test failures that snuck into master this morning.

@swift-ci

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2019

Build failed
Swift Test OS X Platform
Git Sha - 5c1f289

@swift-ci

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2019

Build failed
Swift Test Linux Platform
Git Sha - 5c1f289

@pschuh

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 15, 2019

@brentdax Alas, no, this was SILOptimizer/OSLogPrototypeCompileTest.swift. I diffed the SIL before and after, and just moving around a single alloc_box instruction fixed the issue (it triggers: oslog_non_constant_message). Manually evaluating the TapExpr without OpaqueValue lets me control the emitted order which makes the test pass. Now, the only difference in the sil from this change is that the metatype instructions are moved closer to the callsite of the initializers.

@pschuh

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 15, 2019

@swift-ci please test macos

@pschuh pschuh merged commit 26c4ccc into apple:master Jul 16, 2019

4 checks passed

Swift Test Linux Platform No test results found.
Details
Swift Test Linux Platform (smoke test)
Details
Swift Test OS X Platform No test results found.
Details
Swift Test OS X Platform (smoke test)
Details

theblixguy added a commit to theblixguy/swift that referenced this pull request Jul 17, 2019

[Typechecker] Conversion may fail if we're using a magic literal
Revert "[Typechecker] Conversion may fail if we're using a magic literal"

This reverts commit d8e901d.

[CS] Validate default arguments by generating constraints for the expression and the types

[Test] Add tests

[CS] Only validate default argument if its a magic literal, etc

[CSDiagnostics] Adds a new fix to diagnose calling a method with default argument whose type does not match and the argument is a literal

[CSDiagnostics] Use getChoiceFor to simplify the code and use default expression type directly instead of getting it from CS

[CSSimplify] Bail out if the defaultValueExpr has no type in the constraint system

[CSSimplify] Address PR comment

[CSSimplify] Bail out if parameter does not have a default value expression

[CSSimplify] Bail out if callee is null

[CSSimplify] Use getParameterAt to support other decls like init

[Test] Tweak diagnostic

[stdlib] Simplify __SwiftNativeNSArrayWithContiguousStorage.count

SwiftShims: restrict AArch64 workaround to C++

`RefCount.h` can be included in a C context - e.g. building the
SwiftShims module.  Restrict the C++ overloads to the C++ context only.
This partially improves the build for Windows ARM64.

AST: attempt to rewrite the expression to be more legible

This restores the original spelling of the restriction.  This is more
legible and may be something which VS2017 will handle properly.

Thanks to @drodriguez for suggesting this!

Runtime: print type layout values correctly on big-endian systems

The memcpy in the type layout verifier was not correct for big-
endian systems. While we are here change 'long long' to a fixed
width unsigned type (uint64_t). It doesn't really make sense to
print the value as a signed number since we have zero extended
it from its original bit width using the memcpy.

Remove TypeChecker::getWitnessType()

Platform: extend WinSDK further

This extends the WinSDK module definition further to better modularise
the headers.  This should improve the header organisation as well as
setup additional autolink rules.

Revert "Better runtime failure messages (not yet enabled by default)"

[CodeCompletion] Add defensive nullptr check

Prevent crash.
rdar://problem/51599533

[Type checker] Lazily synthesize the empty implicit destructor of a class.

This is more a matter of principle than an optimization, eliminating
unnecessary calls to `AbstractFunctionDecl::setBody()`.

[AST] Separate out the operations that set *parsed* bodies of functions.

When the parser wires up the body of a function, it's a legitimate use of
setting a parsed body. Separate these out from the other uses of
setBody() that we want to eliminate over time.

[Type checker] Lazily synthesize body of default constructors.

[Type checker] Fuse typeCheck(Function|Constructor|Destructor)BodyUntil().

These three functions shared a bunch of code in common. Fuse them into
their only caller, typeCheckAbstractFunctionBodyUntil() and centralize
the point at which we set the body of the function.

[Type checker] Fold typeCheckFunctionBuilderFuncBody() so it isn't so special

Instead of being a completely separate body type-checking pass, refactor
this function so it does the function builder transform on the body, then
continues with the rest of typeCheckFunctionBodyUntil() as normal.

[Type checker] Factor "type check function body until" into a request.

Extend the "type check function body" request to also cover the case
where we have a specific ending source location. Fold all of this
functionality into a single request, so we consistently go through a
request to compute a type-checked function body.

Adds an expected-error test characterizing the issue in SR-9482.

Fix issue SR-9482.

Solution based on the outline in the issue description.

Also fixes the corresponding rdar://problem/46644027.

SIL: Extend cond_fail by a second operand, which is a static string literal, indicating the failure reason.

SIL: add a failure message operand to Builtin.condfail

The SIL generation for this builtin also changes: instead of generating the cond_fail instructions upfront, let the optimizer generate it, if the operand is a static string literal.
In worst case, if the second operand is not a static string literal, the Builtin.condfail is lowered at the end of the optimization pipeline with a default message: "unknown program error".

IRGen: debug info generation for cond_fail messages.

To display a failure message in the debugger, create a function in the debug info which has the name of the failure message.
The debug location of the trap/cond_fail is then wrapped into this function and the function is declared as "inlined".
In case the debugger stops at the trap instruction, it displays the inline function, which looks like the failure message.
For example:

* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_INSTRUCTION (code=EXC_I386_INVOP, subcode=0x0)
    frame #0: 0x0000000100000cbf a.out`testit3(_:) [inlined] Unexpectedly found nil while unwrapping an Optional value at test.swift:14:11 [opt]
   11
   12   @inline(never)
   13   func testit(_ a: Int?) -> Int {
-> 14     return a!
   15   }
   16

This change is currently not enabled by default, but can be enabled with the option "-Xllvm -enable-trap-debug-info".
Enabling this feature needs some changes in lldb. When the lldb part is done, this option can be removed and the feature enabled by default.

stdlib: let _precondition include the file+line info in debug builds.

For using the improved condfail messages in the stdlib, we need a function, like precondition(), just taking a StaticString instead of a String for the message.
The existing (underscored) _precondition is a perfect fit for this, except that it's not printing the location info in debug builds.
This change makes _precondition() equivalent to precondition, just taking a StaticString as argument.

The other alternative would be to add another variant of precondition, just taking a StaticString. But we already have so many failure functions in Assert.swift, so adapting an existing one seems to be a better solution.
This effectively undos a change from 5 years ago which intentionally removed the location info from _precondition (rdar://problem/16958193).  But this was at a time where swift was not open source yet. So I think today it's okay to always add location information, even if it's from inside the stdlib. It can be even very useful for expert users for looking up the location the stdlib source.

stdlib: Use the new failure message method for the "unexpected found nil while unwrapping an optional" failure message.

Advantage: the failure message is now also visible in release builds.

Validate -static-stdlib on Driver::validate args and removing handles on Darwing toolchain

Moving -static-stdlib validation to Darwin toolchain.

Updating test/Driver tests.

Fixing nitpicks.

Forward declaring and improvement on warning.

Fixing nits and unused code.

Adding todo.

Missing forward

Runtime: simplify loading and storing enum values

This commit centralizes the code that converts variable length
tag values, stored in enum payloads and extra tag bytes, to and
from the 4-byte integer values that the runtime uses to represent
the enum case.

Note that currently big endian machines will store the tag value
in the first word of the destination. This reflects the current
behaviour of the compiler. I am however expecting to change this
so that the value is stored as a true variable-length big-endian
integer in the near future, so the tag value will be stored in
the last 4 bytes of payloads rather than the first 4 bytes like
they are on little-endian systems.

Convert InterpolatedStringLiteralExpr to not use tc.callWitness(). (apple#26076)

For reference, all other callers of callWitness have been migrated.

Platform: fix library link names

This adjusts the library link names so that linking works properly on
case sensitive file systems (e.g. ext4) when cross-linking.

[Typechecker] Check for generic signature requirements when pechecking for overrides

[Typechecker] Check for generic signature requirements when pechecking for overrides

[Test] Add tests

[Test] Remove dead test code

[Test] Adds a test case where a concrete class inherits from a generic class

[Typechecker] Check if the generic signature is null before accessing it

[Typechecker] Pass a substitution map to requirementsNotSatisfiedBy()

[Test] Adds a new test case that mimics a Codable class

This is basically  a sort of regression test that ensures we apply proper context substitutions before checking the generic signatures of the functions

[Test] Fix a typo - use expected-note and not expected-error

[Typechecker] Make sure superclass isn't null before accessing it

[Typechecker] Check for exact generic signature

[Typechecker] Check that either base requirements are not satisfied or derived requirements are not satisfied

[GenericSignature] Fix indentation

[Typechecker] Attempt to extract the logic from configureGenericDesignatedInitOverride and tweak it to work with generic methods

[Typechecker] Fix a few crashes in getNewGenericSignature

[Typechecker] Apply review comments

[Typechecker] No need to assign to subMap as its not used afterwards

[Typechecker] Call baseGenericCtx->isGeneric() before calling getGenericParams() otherwise it can lead to a crash

Apply review feedback and update diagnostic

[Typechecker] Add derived method's generic parameters to GSB

[Typechecker] Fix a crash when the superclass type is null

[Test] Add some more tests

[Test] Fix test class names

[Typechecker] Return nullptr if base class is not present

[Typechecker] Bail out if we don't have any generic params in the GSB, otherwise we're gonna trip an assertion

[Typechecker] Change method name to getOverrideGenericSignature

[Typechecker] Ensure both base and derived generic contexts are not null before calling getOverrideGenericSignature()

[Typechecker] Only check for derivedSig->requirementsNotSatisfiedBy(newSig)

[Typechecker] Check for empty generic parameters early

[Typechecker] Use early return if the base generic context does not have a generic signature

[Diagnostics] Fix a typo in diagnostic message

[Test] Remove diagnostic from a test case

[AST] Undo unnecessary indentation changes

[AST] Emit vtable thunk if base generic requirements are not satisfied by derived

Revert "[AST] Emit vtable thunk if base generic requirements are not satisfied by derived"

This reverts commit e31a360.

[Typechecker] Add the new generic signature to the diagnostic message

[Typechecker] Add base method's generic parameters instead of derived

[Typechecker] Revert to using derivedGenericCtx params and rename depth and superclassDepth vaeiables

[Typechecker] Fix an error due to use of old variable name

Delete TypeChecker::callWitness.

Sema: Refactor maybeAddAccessorsToStorage()

The idea here is to split up the logic into three parts:

- computation of the final ImplInfo for lazy, property wrappers
  and @NSManaged

- deciding if we should add accessors or not

- actually adding the accessors

Serialization: Temporary workaround to validate accessors when needed

AST: Remove skipInaccessible argument from NominalTypeDecl::getStoredProperties()

Sema: Remove TypeChecker::DelayedCircularityChecks

Since validateDecl() never calls back into typeCheckDecl(), I think
it's safe to call validateDecl() from circularity checking.

AST: requiresNewVTableEntry() validates the declarations if needed

AST: Simplify Optional default initialization behavior

SIL: Validate stored properties and enum elements as needed

Serialization: Actually set VarDecl::isLazyStorageProperty() when deserializing

[CSSimplify] Don't call getParameterAt if callee is not a function, enum element or subscript

getParameterAt expects the callee to be a function or enum element otherwise it casts it to a subscript. If we pass a value decl that is not one of them then it will cause a crash

[CS] Don't crash when using magic literals as default arguments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.