[SR-11223] Generics Type Resolution Regression on Linux DEVELOPMENT-SNAPSHOT-2019-07-26-a (61261fcddf) #53624
Comments
|
This pipeline passed on the trunk version of Swift 2 weeks ago: https://gitlab.com/Mordil/swift-redi-stack/pipelines/70820924 Now, it's failing as mentioned above, with no changes to |
|
@swift-ci create |
|
Looks like an error in the RediStack. The call stack when the assertion fires contains:
The source code for frame 25 is:
And frame 24 is:
So I don't know why it didn't fail before, but it looks like the failure is correct. |
|
@mikeash but public func writeAndFlush(_ data: NIOAny, promise: EventLoopPromise<Void>?) {
pipeline.writeAndFlush(data, promise: promise)
}
public func writeAndFlush<T>(_ any: T) -> EventLoopFuture<Void> {
return self.writeAndFlush(NIOAny(any))
}
I'm not saying we couldn't work around this issue in NIO but the problem is that it always worked and suddenly it doesn't seem to work anymore. @Mordil you can confirm that the very same code works on Swift 5.0, right? |
|
Yes. This exact code works with Swift 5, and 5.1 on both Linux and macOS |
|
How would it select the first |
|
@mikeash like so: $ cat /tmp/test.swift
func foo<T>(_ data: T) {
print("generic \(data)")
}
func foo(_ data: NIOAny) {
print("boxed \(data)")
}
struct NIOAny {}
foo(NIOAny())
$ swift /tmp/test.swift
boxed NIOAny() |
|
Right, but your quoted code has a |
|
@mikeash sorry, my bad, there's still two of those: |
|
so we have all of those: $ git grep 'public func writeAndFlush' | grep -v ChannelPipeline
Sources/NIO/Channel.swift: public func writeAndFlush(_ data: NIOAny, promise: EventLoopPromise<Void>?) {
Sources/NIO/Channel.swift: public func writeAndFlush<T>(_ any: T) -> EventLoopFuture<Void> {
Sources/NIO/Channel.swift: public func writeAndFlush<T>(_ any: T, promise: EventLoopPromise<Void>?) {
Sources/NIO/ChannelInvoker.swift: public func writeAndFlush(_ data: NIOAny, file: StaticString = #file, line: UInt = #line) -> EventLoopFuture<Void> {and |
|
Got it, that should definitely be invoking the more specific variant of |
|
This simple test case illustrates the problem: func foo<T>(_ data: T) {
print("generic \(data)")
}
func foo(_ data: NIOAny, file: StaticString = #file) {
print("boxed \(data)")
}
struct NIOAny {}
foo(NIOAny())On current master, it calls the "generic" one, but it should call "boxed." |
|
Oh, I think I know what is going on here. You'd need not one more `foo(NIOAny())` to see this: On first `foo(NIOAny())` solver has this behavior: But the second time around this happens: And since there are two solutions it would pick the one with concrete types from parameters over the generic version as per ranking rules. I wonder if this is happening because of #26074 @theblixguy do you want to take a closer look at this? |
|
@xedin It is indeed caused by my change! I disabled the change and tested Mike's example above and it worked as expected. This is happening because we're adding an ArgumentConversion constraint (between defaultTy and paramTy). |
|
Interesting, I think we need to approach this a bit differently then and have it do a conversion just line in "not yet type-checked" case... |
|
@xedin Do you mean when the defaultValueExpr isn't type checked? We only add the constraint when it doesn't have a type. |
|
Yeah, sorry I mean in the "not yet type-checked" case has to behave exactly the same as if defaultValue has been type-checked already, so it can't increase a score otherwise we'd end up producing different solutions. |
|
Hmm so what do we do in that case? changing the `ArgumentConversion` constraint to just Conversion doesn't make any difference. For the example above, we never succeed the `if (defaultValueExpr->getType()) continue;` check, which means in this case we always end up with a defaultValueExpr of null type and end up adding the `ArgumentConversion` constraint. When we do have a type, we don't actually do anything, we just continue (as mentioned above). |
|
That's a good question, we probably have no choice but to add conversion regardless whether `defaultValueExpr` has been type-checked or not, otherwise in presence of possibility of implicit conversions there is always going to be a risk to difference in behavior. |
|
Okay, I can create a patch for that, but unfortunately it doesn't fix this issue when we add a Conversion constraint in both cases. |
|
Sorry, we don't only have to add a constraint, we have to generate constraints for default value expression in both cases. |
|
Hmm no luck in that case as well. Here's my patch: |
|
The problem is in ConstraintGenerator::visitLiteralExpr - if the expression has the type set already, it just returns that type instead of adding conformance constraint. |
|
Oh, even if we always add the constraint, it makes no difference |
|
I think we should revert the changes and re-think the strategy here. |
|
@theblixguy Could you please revert the original PR until there is a fix for this problem? |
|
Sorry, I missed your previous comment. I’ll create a PR shortly. |
|
Thank you! |
|
Fixed on master. Please verify using the next available master snapshot and mark the issue as closed. Thanks! |
Environment
Ubuntu 16.04
Swift DEVELOPMENT-SNAPSHOT-2019-07-26-a
Additional Detail from JIRA
md5: 665b8c631694dff27c64b4d3ef5cb046
Issue Description:
RediStack builds against the nightly Swift builds on Ubuntu 16.04 to catch any errors that might crop up in the future.
Between 2 weeks ago and now, I am seeing an assertion failure that states
while running unit tests.
Which in
NIOAny.swiftisThis initializer is triggered from these methods in SwiftNIO's
Channel.swift:Which are called by this single code snippet in
RedisConnection.swift:Full stack trace:
The text was updated successfully, but these errors were encountered: