[TypeChecker] SE-0213: Implement literal init via coercion #17860
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@swift-ci please smoke test compiler performance |
|
@swift-ci please benchmark |
| // 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 |
There was a problem hiding this comment.
@xedin Nice to see that the above FIXME's are addressed by this fix. Thanks!
jrose-apple
reviewed
Jul 10, 2018
test/Sema/diag_erroneous_iuo.swift
Outdated
| let _: Optional<Int!> = nil // expected-error {{'!' is not allowed here; perhaps '?' was intended?}}{{20-21=?}} | ||
| _ = Int!?(0) // expected-error {{'!' is not allowed here; perhaps '?' was intended?}}{{8-9=?}} | ||
| _ = Int!?(0) // expected-error 3 {{'!' is not allowed here; perhaps '?' was intended?}}{{8-9=?}} |
There was a problem hiding this comment.
This looks like we're processing the type multiple times…
There was a problem hiding this comment.
Yeah, we have that in a couple of places already unfortunately, because we don't track if we produced the same diagnostic multiple times...
There was a problem hiding this comment.
Looking at the TypeLoc, we should see it has already been resolved to a Type, and not resolve the TypeRepr again. Where is the diagnostic being emitted?
There was a problem hiding this comment.
Ah I see, it's probably my calls to resolveType which caused this, I'll try to fix it.
There was a problem hiding this comment.
Done, just needed to cache the type in a couple of places :)
Build comment file:Optimized (O)Regression (4)
Improvement (21)
No Changes (433)
Unoptimized (Onone)Regression (1)
Improvement (9)
No Changes (448)
Hardware Overview |
|
@swift-ci please benchmark |
|
@swift-ci please smoke test compiler performance |
Build comment file:Summary for master smoketestUnexpected test results, excluded stats for ReactiveCocoa No regressions above thresholds Debugdebug briefRegressed (0)
Improved (0)
Unchanged (delta < 1.0% or delta < 100.0ms) (2)
debug detailedRegressed (0)
Improved (0)
Unchanged (delta < 1.0% or delta < 100.0ms) (23)
Releaserelease briefRegressed (0)
Improved (0)
Unchanged (delta < 1.0% or delta < 100.0ms) (2)
release detailedRegressed (0)
Improved (0)
Unchanged (delta < 1.0% or delta < 100.0ms) (23)
|
Build comment file:Optimized (O)Regression (7)
Improvement (7)
No Changes (444)
Unoptimized (Onone)Regression (5)
Improvement (3)
No Changes (450)
Hardware Overview |
moiseev
approved these changes
Jul 11, 2018
|
@swift-ci please test |
|
Oops, looks like I missed one 32-bit test. |
|
@swift-ci please test |
|
@swift-ci please test source compatibility |
DougGregor
reviewed
Jul 17, 2018
lib/Sema/TypeCheckConstraints.cpp
Outdated
| auto *parent = ExprStack.back(); | ||
| if (isa<BindOptionalExpr>(parent) || isa<ForceValueExpr>(parent)) | ||
| return nullptr; | ||
| } |
There was a problem hiding this comment.
This semantic change isn't described in the proposal, and I think we wouldn't deviate from it. The rule as specified in SE-0213 is a clean syntactic rule that users can reason about. This tweak would come as a surprise.
If we need it for source compatibility, perhaps it would be best to put it behind !isSwiftVersionAtLeast(5).
There was a problem hiding this comment.
I was just trying to avoid breaking existing code like https://github.com/apple/swift/pull/17860/files#diff-de58b3c2866a24533bc39d1bfd40fc3cR276 but maybe I shouldn't do it at all then?
There was a problem hiding this comment.
Alright, this check is hidden under !isSwiftVersionAtLeast(5) now.
| return nullptr; | ||
|
|
||
| // Don't bother to convert deprecated selector syntax. | ||
| if (auto selectorTy = TC.getObjCSelectorType(DC)) { |
There was a problem hiding this comment.
This is the use of string literals for selectors? I wonder if we can (separately) kill that now...
There was a problem hiding this comment.
That's right, it's #selector(...) vs. Selector("...")
| let base = U${Self}(_value) | ||
| return self < (0 as ${Self}) ? ~base + 1 : base | ||
| @inline(__always) | ||
| get { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
It is, unfortunately. @_transparent here causes the compile-time overflow on a branch that would not be taken at runtime.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Why not write return self < (0 as ${Self}) ? ~base &+ 1 : base? You are, after all, performing a bit twiddling operation.
There was a problem hiding this comment.
It is very much possible (and probably reasonable) to try the &+ approach in a separate PR once this lands.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
No, _value is the same type regardless of whether it's Int or UInt, and that initializer simply assigns self._value = other._value.
There was a problem hiding this comment.
@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 &+
There was a problem hiding this comment.
I'm going to open follow up PR for this and run the benchmarks
Implementation is as follows: In `preCheckExpression` try to detect if there is `T(literal)` call in the AST, replace it with implicit `literal as T`, while trying to form type-checked AST, after constraint solving, restore source information and drop unnecessary coercion expression. Resolves: rdar://problem/17088188 Resolves: rdar://problem/39120081 Resolves: rdar://problem/23672697 Resolves: rdar://problem/40379985
…ons in Swift < 5 Maintain source compatibility, but only when Swift < 5, in cases when type declares conformance to literal protocol and at the same time has failable initializer with the same parameter type, see `Source compatibility` of the proposal for more details.
|
@swift-ci please test |
|
@swift-ci please test source compatibility |
|
Build failed |
|
@swift-ci please test |
|
Build failed |
|
@swift-ci please test source compatibility |
|
Build failed |
|
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Implementation is as follows: In
preCheckExpressiontry todetect if there is
T(literal)call in the AST, replace it withimplicit
literal as T, while trying to form type-checked AST,after constraint solving, restore source information and drop unnecessary
coercion expression.
Resolves: rdar://problem/17088188
Resolves: rdar://problem/39120081
Resolves: rdar://problem/23672697
Resolves: rdar://problem/40379985