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

[SR-12290] Unexpected key-path syntax compiles as some kind of a short-hand form to Self #54718

Closed
DevAndArtist mannequin opened this issue Feb 27, 2020 · 12 comments
Closed

Comments

@DevAndArtist
Copy link
Mannequin

DevAndArtist mannequin commented Feb 27, 2020

Previous ID SR-12290
Radar rdar://problem/59874355
Original Reporter @DevAndArtist
Type Bug
Status Resolved
Resolution Done
Environment

Apple Swift version 5.1.3 (swiftlang-1100.0.282.1 clang-1100.0.33.15)

Additional Detail from JIRA
Votes 1
Component/s Compiler
Labels Bug, TypeChecker
Assignee @DevAndArtist
Priority Medium

md5: b40e79fb4ddfc23b392c983d679e806c

Issue Description:

struct S {
  var value: [Int] = []
  func foo(_: KeyPath<S, Int> = \.value.count) {}
  // THIS NEXT LINE: there is no dot after `\`
  func bar(_: KeyPath<S, Int> = \value.count) {}
}

This syntax works as a short-hand form for current enclosing type.

@propertyWrapper
struct Derived<Root, Value> {
  var wrappedValue: Value {
    get { fatalError() }
    set { fatalError() }
  }
​
  let keyPath: ReferenceWritableKeyPath<Root, Value>
​
  init(_ keyPath: ReferenceWritableKeyPath<Root, Value>) {
    self.keyPath = keyPath
  }
​
  static subscript(
    _enclosingInstance instance: Root,
    wrapped wrappedKeyPath: ReferenceWritableKeyPath<Root, Value>,
    storage storageKeyPath: ReferenceWritableKeyPath<Root, Self>
  ) -> Value {
    get {
      instance[keyPath: instance[keyPath: storageKeyPath].keyPath]
    }
    set {
      instance[keyPath: instance[keyPath: storageKeyPath].keyPath] = newValue
    }
  }
}
​
​
class Parent: UIView {
  private let label = UILabel()
​
  @Derived(\label.text)
  var text: String?
}
​
let instance = Parent()
instance.text // nil
instance.text = "foo"
@hborla
Copy link
Member

hborla commented Feb 28, 2020

@swift-ci create

@theblixguy
Copy link
Collaborator

theblixguy commented Feb 28, 2020

I was looking at this yesterday, but I am not sure if it's possible to diagnose the missing dot in the parser (there's actually a FIXME in parseExprKeyPath for adding diagnostics about it) because we don't know if the root is a type or a property yet. I think it might be better to detect this in Sema. cc @rintaro

@xedin
Copy link
Member

xedin commented Mar 2, 2020

I think that's intentional because root of the keypath in this case is inferred from contextual type of the parameter so dot or no dot is going to produce exactly the same AST. /cc @jckarter

@jckarter
Copy link
Member

jckarter commented Mar 2, 2020

A keypath with a contextual root type is supposed to still require the separating `.` between the type and the path.

@xedin
Copy link
Member

xedin commented Mar 2, 2020

@jckarter I see, thank you!

@jckarter @theblixguy I think to fix this we need to record wether key path started with `dot` or not. And if it didn't, diagnose in `PreCheckExpression` when root turns out to be a member reference that requires an implicit base. WDYT?

@theblixguy
Copy link
Collaborator

theblixguy commented Mar 2, 2020

@xedin I think that is reasonable, but it does involve changing the AST I think.

When I was looking at it, I observed that when we don't use a dot (ex: \comp1.comp2), the parsed root starts with an implicit TypeExpr. When we use a dot (ex: .comp1.comp2), we start with an implicit KeyPathDotExpr and when we have an explicit root (\Root.comp1.comp2), we start with a non-implicit TypeExpr.

So, we can perhaps look at the parsed root to detect this scenario. WDYT? or is it simply better to record it in the AST instead?

@xedin
Copy link
Member

xedin commented Mar 2, 2020

@theblixguy Right, I guess we could reply on presence of KeyPathDotExpr as an indicator whether it's okay to infer contextual base or not. In cases like `\foo.bar` parser would produce an `UnresolvedDeclRefExpr` which then triggers lookup in `PreCheckExpression::resolveDeclRefExpr` to get rewritten into either `DeclRefExpr` or `UnresolvedDotExpr` with (potentially implicit) base.

@theblixguy
Copy link
Collaborator

theblixguy commented Mar 2, 2020

@xedin Hmm, but we should be able to just check whether the root starts with an implicit TypeExpr or not in resolveKeyPathExpr (we do a bunch of diagnostics there anyway, like invalid components etc)? If it is, then it's a keypath like \foo.bar which is not correct. Does it make sense, or am I completely wrong?

@xedin
Copy link
Member

xedin commented Mar 2, 2020

That's the idea yes, but I think it's the matter of how to determine what is a root e.g. `\foo.bar` and `\S.foo.bar` are going to have `getParsedRoot()` but not `getParsedPath()` and `.foo.bar` is not going to have root but going to have a path.

It looks to be simplify a matter of checking whether `getParsedRoot()` starts with an implicit `TypeExpr` expression. Looks like `traversePath` in `resolveKeyPathExpr` already detects that `TypeExpr` could only be found in non-`isParsedPath` case too so we just need to check whether it's implicit or not...

@theblixguy
Copy link
Collaborator

theblixguy commented Mar 2, 2020

Sure, let's discuss it further in the PR: #30164

@theblixguy
Copy link
Collaborator

theblixguy commented Mar 3, 2020

Fixed on master. @DevAndArtist Please verify using the next available development snapshot!

@xedin
Copy link
Member

xedin commented Apr 8, 2020

Should be fixed by #30867 @DevAndArtist please use next development snapshot of master to verify.

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants