[Typechecker] Fix fix-it location for missing try when called on optional protocol value

[theblixguy]

When we call a throwing method on an optional protocol-typed value and forget to add try/try?/try!, then we emit the fix-it at the wrong location. This is because the source locations for OptionalEvaluationExpr points to the throwing method rather than the outer value. We also have some additional expressions, such as an OpenExistentialExpr, which wraps the CallExpr of the throwing method and holds the correct source location for the fix-it.

Example:

protocol Foo {
  func bar() throws
}

class HasFoo {
  var foo: Foo?

  func someMethod() throws {
    foo?.bar()
       ^ this is where we currently emit the fix-it

    foo?.bar()
   ^ this is where we should emit the fix-it
  }
}

Resolves SR-11016.

[theblixguy]

cc @xedin

[hamishknight]

Would it be possible to instead store a SourceRange on OpaqueValueExpr which, for cases where it represents some underlying expression, would be that expression's source range? That way I believe we'd be able to preserve the correct source range for the apply here without having to look at the OpenExistentialExpr.

[theblixguy]

I think that could work. I believe at the moment we're passing the OpenExistentialExpr (or whatever the base expression is) location to the OpaqueValueExpr in all the places.

open_existential_expr implicit type='()' location=/Users/suyashsrijan/Desktop/test.swift:9:10 range=[/Users/suyashsrijan/Desktop/test.swift:9:8 - line:9:14]
   (opaque_value_expr implicit type='SR_11016_P' location=/Users/suyashsrijan/Desktop/test.swift:9:8 range=[/Users/suyashsrijan/Desktop/test.swift:9:8 - line:9:8] @ 0x7f92280db318)
   (load_expr implicit type='SR_11016_P' location=/Users/suyashsrijan/Desktop/test.swift:9:8 range=[/Users/suyashsrijan/Desktop/test.swift:9:5 - line:9:8]
      (bind_optional_expr type='@lvalue SR_11016_P' location=/Users/suyashsrijan/Desktop/test.swift:9:8 range=[/Users/suyashsrijan/Desktop/test.swift:9:5 - line:9:8] depth=0
      (member_ref_expr type='@lvalue SR_11016_P?' location=/Users/suyashsrijan/Desktop/test.swift:9:5 range=[/Users/suyashsrijan/Desktop/test.swift:9:5 - line:9:5] decl=test

This is what the AST looks like. The same OpaqueValueExpr appears later inside the DotSyntaxCallExpr as well:

(call_expr type='()' location=/Users/suyashsrijan/Desktop/test.swift:9:10 range=[/Users/suyashsrijan/Desktop/test.swift:9:8 - line:9:14] throws arg_labels=
  (dot_syntax_call_expr type='() throws -> ()' location=/Users/suyashsrijan/Desktop/test.swift:9:10 range=[/Users/suyashsrijan/Desktop/test.swift:9:8 - line:9:10] nothrow
     (declref_expr type='(SR_11016_P) -> () throws -> ()' location=/Users/suyashsrijan/Desktop/test.swift:9:10 range=[/Users/suyashsrijan/Desktop/test.swift:9:10 - line:9:10] decl=test.(file).SR_11016_P.bar()@/Users/suyashsrijan/Desktop/test.swift:2:8 [with (substitution_map generic_signature=<Self where Self : SR_11016_P> (substitution Self -> SR_11016_P))] function_ref=single)
     (opaque_value_expr implicit type='SR_11016_P' location=/Users/suyashsrijan/Desktop/test.swift:9:8 range=[/Users/suyashsrijan/Desktop/test.swift:9:8 - line:9:8] @ 0x7f92280db318))

If the OpaqueValueExpr location was the same as the LoadExpr then we could access that when looking at the apply. I don't think we're actually using its location anywhere, so it shouldn't break anything if we make that change.

[hamishknight]

@theblixguy Yeah, currently OpaqueValueExpr just stores a single SourceLoc, meaning that its source range can only cover a single token. For an open existential like this, that location is the base's getLoc() (in openExistentialReference I believe). For a bind-optional base such as foo?, that token is the ?, so the resulting source range doesn't cover the foo.

If OpaqueValueExpr used the actual source range of foo?, then I believe both the DotSyntaxCallExpr and CallExpr source ranges would both start at the correct place. This change does have the potential to affect other code, but given we're now using more accurate source range info, I don't believe it should cause any regressions.

[theblixguy]

Sounds good. This works for my test case. @xedin Does the change look good to you? I haven't changed it so it stores the entire SourceRange, but I can add that if needed.

[theblixguy]

ping @xedin did you had a chance to take a look at this? Thanks!

[xedin]

Thanks for the reminder! This is in my queue, I'll try to take a look ASAP.

[xedin]

Looks great!

[xedin]

@swift-ci please smoke test