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

[Typechecker] Diagnose key paths with contextual root type but no leading dot #30164

Merged
merged 1 commit into from
Mar 3, 2020
Merged

[Typechecker] Diagnose key paths with contextual root type but no leading dot #30164

merged 1 commit into from
Mar 3, 2020

Conversation

theblixguy
Copy link
Collaborator

@theblixguy theblixguy commented Mar 2, 2020

If a Swift key path has root type inferred but does not have a leading dot, then diagnose it, because it's not valid. For example:

struct Foo {
  let property: [Int] = []
  let kp: KeyPath<Foo, Int> = \property.count // error
}

The key path's path starts with an implicit TypeExpr/DeclRefExpr as root, which only happens if the type is supposed to be inferred and the leading dot is missing. In all other scenarios, we have either an implicit KeyPathDotExpr or an explicit TypeExpr. So, I use this information to detect this scenario and diagnose.

Resolves SR-12290
Resolves rdar://problem/59874355

@theblixguy theblixguy requested review from xedin and hborla March 2, 2020 22:15
Copy link
Contributor

@xedin xedin left a comment

Choose a reason for hiding this comment

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

LGTM!

test/expr/unary/keypath/keypath.swift Outdated Show resolved Hide resolved
@theblixguy
Copy link
Collaborator Author

theblixguy commented Mar 2, 2020

Actually, I missed one other scenario - the root starting with a resolved but implicit DeclRefExpr. This can happen in the following case:

func foo<T>(_: KeyPath<SR_12290, T>) {}
foo(\property.count)

At the moment, we already diagnose this with "invalid component of Swift key path", but I have added an explicit check for it so we can emit a better diagnostic now.

@xedin
Copy link
Contributor

xedin commented Mar 2, 2020

Great, thank you!

@theblixguy
Copy link
Collaborator Author

@swift-ci please smoke test

@theblixguy
Copy link
Collaborator Author

Once test passes, I'll run source compatibility tests just in case someone is relying on it... if so we might have to downgrade it to a warning.

@xedin
Copy link
Contributor

xedin commented Mar 2, 2020

That would be really unfortunate...

@theblixguy
Copy link
Collaborator Author

@swift-ci please smoke test

2 similar comments
@xedin
Copy link
Contributor

xedin commented Mar 3, 2020

@swift-ci please smoke test

@xedin
Copy link
Contributor

xedin commented Mar 3, 2020

@swift-ci please smoke test

@xedin
Copy link
Contributor

xedin commented Mar 3, 2020

@swift-ci please smoke test Linux platform

@xedin
Copy link
Contributor

xedin commented Mar 3, 2020

@swift-ci please test source compatibility

@theblixguy theblixguy merged commit 13487ed into swiftlang:master Mar 3, 2020
@theblixguy theblixguy deleted the fix/SR-12290 branch March 3, 2020 12:17
@dan-zheng
Copy link
Contributor

This broke KeyPathIterable derived conformances on tensorflow branch, which constructs implicit TypeExpr as root: https://github.com/apple/swift/blob/3e7cb166a69de3bf2c87456645b7586f8d48c9e0/lib/Sema/DerivedConformanceKeyPathIterable.cpp#L85-L86.

struct Foo: KeyPathIterable {
  var float: Float
  var int: Float
  var string: String
}
$ swift kpi.swift
<unknown>:0: error: a Swift key path with contextual root must begin with a leading dot
<unknown>:0: error: a Swift key path with contextual root must begin with a leading dot
<unknown>:0: error: a Swift key path with contextual root must begin with a leading dot

Attempting to make the TypeExpr not implicit doesn't work either:

Assertion failed: (Loc.isValid() || isImplicit), function createForDecl, file /Users/danielzheng/swift-dev/swift/lib/AST/Expr.cpp, line 1934.
Stack dump:
0.	Program arguments: /Users/danielzheng/swift-dev/build/Ninja-ReleaseAssert/swift-macosx-x86_64/bin/swiftc -frontend -target x86_64-apple-macosx10.9 -module-cache-path /Users/danielzheng/swift-dev/build/Ninja-ReleaseAssert/swift-macosx-x86_64/swift-test-results/x86_64-apple-macosx10.9/clang-module-cache -sdk /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.15.sdk -swift-version 4 -ignore-module-source-info -typo-correction-limit 10 -typecheck -verify -primary-file /Users/danielzheng/swift-dev/swift/test/Sema/class_differentiable.swift /Users/danielzheng/swift-dev/swift/test/Sema/Inputs/class_differentiable_other_module.swift
1.	Swift version 5.2-dev (LLVM b3057cffb6, Swift 5209f634da)
2.	While evaluating request TypeCheckSourceFileRequest(source_file "/Users/danielzheng/swift-dev/swift/test/Sema/class_differentiable.swift")
3.	While evaluating request TypeCheckFunctionBodyUntilRequest(class_differentiable.(file).TestKeyPathIterable.TangentVector._, )
4.	While evaluating request ParseAbstractFunctionBodyRequest(class_differentiable.(file).TestKeyPathIterable.TangentVector._)
0  swiftc                   0x0000000109e1dff5 llvm::sys::PrintStackTrace(llvm::raw_ostream&) + 37
1  swiftc                   0x0000000109e1cfa8 llvm::sys::RunSignalHandlers() + 248
2  swiftc                   0x0000000109e1e5ec SignalHandler(int) + 268
3  libsystem_platform.dylib 0x00007fff57f22b5d _sigtramp + 29
4  swiftc                   0x000000010c76e997 cmark_strbuf__initbuf + 176698
5  libsystem_c.dylib        0x00007fff57ddc6a6 abort + 127
6  libsystem_c.dylib        0x00007fff57da520d basename_r + 0
7  swiftc                   0x000000010a2fb3c3 swift::TypeExpr::createForDecl(swift::DeclNameLoc, swift::TypeDecl*, swift::DeclContext*, bool) (.cold.5) + 35
8  swiftc                   0x000000010699d1d5 swift::TypeExpr::createForDecl(swift::DeclNameLoc, swift::TypeDecl*, swift::DeclContext*, bool) + 373
9  swiftc                   0x0000000106492a05 deriveBodyKeyPathIterable_allKeyPaths(swift::AbstractFunctionDecl*, void*) + 165

I wonder what's the best way to address this issue? On tensorflow branch, I think we have no choice but to revert this change, which is unfortunate. Shall we revert this change on master too?


Context: we've been incubating KeyPathIterable and derived conformances on tensorflow branch for about a year now. We started a Swift Evolution pitch thread but there doesn't seem to be a clear path to closure. We have some APIs and users that depend quite heavily on KeyPathIterable.

dan-zheng added a commit that referenced this pull request Mar 10, 2020
@theblixguy
Copy link
Collaborator Author

Sorry about that @dan-zheng. I think one solutions we discussed on the SR was for the parser to record the missing dot on the AST and use that information in Sema to diagnose. I think that would be the way to go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants