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

DataDict subscript function crashes on iOS 14.4 and under #2668

Closed
bdunay3 opened this issue Nov 16, 2022 · 5 comments · Fixed by #2784
Closed

DataDict subscript function crashes on iOS 14.4 and under #2668

bdunay3 opened this issue Nov 16, 2022 · 5 comments · Fixed by #2784
Assignees
Labels
bug Generally incorrect behavior planned-next Slated to be included in the next release

Comments

@bdunay3
Copy link

bdunay3 commented Nov 16, 2022

Bug report

We have a value in our graph that maps to String?. If we send a query that has a property field with a value type of String? and in the response that property has a nil value, the response is parsed just fine by Apollo client. But, when we go to access that property from the generated code we get a crash in this code:

  @inlinable public subscript<T: AnyScalarType & Hashable>(_ key: String) -> T {
    get { _data[key] as! T }
    set { _data[key] = newValue }
    _modify {
      var value = _data[key] as! T
      defer { _data[key] = value }
      yield &value
    }
  }

In Swift 5.4 additional type safety was introduced. I also see that they started adding the plumbing to support existential types via the compiler. I believe what is happening in this code is that the Swift 5.4 runtime can deal with casting a AnyHashable to a concrete type more easily. But the Swift 5.3.2 runtime and lower can not.

When I'm in the debugger if I try to access the value using this statement:

e (_data[key] as? AnyHashable)?.base as? String

Then LLDB reports back the correct value of nil. But if I try to access it doing something like:

e _data[key].base as? String

Then I get back errors from LLDB. Usually something like:

error: <EXPR>:8:10: error: value of optional type 'JSONValue?' (aka 'Optional<AnyHashable>') must be unwrapped to refer to member 'base' of wrapped base type 'JSONValue' (aka 'AnyHashable')
_data[key].base as? String
         ^

<EXPR>:8:10: note: chain the optional using '?' to access member 'base' only for non-'nil' base values
_data[key].base as? String
         ^
          ?

<EXPR>:8:10: note: force-unwrap using '!' to abort execution if the optional value contains 'nil'
_data[key].base as? String
         ^
          !

I think this code needs to be a little more robust. The force-unwrap is just opening up clients to crashes. What I would like to see is something where either this property returns a T? or maybe the the property needs to throw?

If you have suggestions on better ways to handle this from the client side, or if there are things we can do in our graph (outside of change the property to not be nilable) I would very much be open to trying them out. But ideally the code should be force unwrapping and deal with the type conversion is a safer way.

Thank you for your time and any help is appreciated.

Versions

Please fill in the versions you're currently using:

  • apollo-ios SDK version: 1.0.2
  • Xcode version: 14.1
  • Swift version: 5.7
  • Package manager: 5.7

Steps to reproduce

Please replace this line with steps to reproduce the problem.

  • Define a graph object/field who's type is a Nilable String
  • Create a query that requests that property but make sure the fields value is nil in the returned data
  • Run the query through the code generation tool
  • Run the generated query code on iOS 14.4 or earlier
@bdunay3
Copy link
Author

bdunay3 commented Nov 16, 2022

Just an addendum; We do not see this crash on iOS 14.5 and higher. But we unfortunately need to support iOS 14.0 and up for the foreseeable future.

@calvincestari
Copy link
Member

Hmm, this seems pretty weird. I'll try replicate but may need to reach out for a sample project if I cannot get it crashing.

@calvincestari
Copy link
Member

@bdunay3 - confirmed, I can replicate the crash on 14.4 and lower. Thanks for the great bug report, we'll get this resolved.

@AnthonyMDev
Copy link
Contributor

@calvincestari This might be as easy as changing the getter to:

get { (_data[key] as? AnyHashable)?.base as! T }

@calvincestari calvincestari added bug Generally incorrect behavior and removed needs investigation labels Dec 13, 2022
@calvincestari calvincestari added this to the Release 1.0.6 milestone Dec 13, 2022
@calvincestari calvincestari added the planned-next Slated to be included in the next release label Jan 5, 2023
@calvincestari
Copy link
Member

calvincestari commented Jan 19, 2023

@bdunay3 this is now fixed and in main. Since having tests for this on CI is difficult, given the specific version needs, I have manually verified locally with an iOS 14.4 simulator that the previously failing tests now work. It would be helpful if you could confirm whether the fix resolves the original reported issue for you too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Generally incorrect behavior planned-next Slated to be included in the next release
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants