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

RFC: Swift 2.0 Changes #302

Open
justin opened this issue Aug 25, 2015 · 28 comments
Open

RFC: Swift 2.0 Changes #302

justin opened this issue Aug 25, 2015 · 28 comments
Assignees

Comments

@justin
Copy link
Collaborator

justin commented Aug 25, 2015

We are discussing changes to make for Swift 2.0. Given that Apple has had no qualms about breaking support for Swift between releases, we want to use it as an opportunity to modernize our Swift support to take full advantage of the language.

This won't have any effect on the Objective-C code, but if you're using our Swift support, you will likely need to massage your code a bit.

  • Switch from subclasses to extensions. EntityName+MOGenerator.swift is an extension on your managed object subclass. This is a replacement for _EntityName.swift.
  • Define an MOGenerator protocol that has entityName() and entity class functions define. Extending that MOGenerator protocol implements those methods, rather than cloning them all in each subclass.
  • Switching from NSNumber to specific scalar values: Int, Int32, etc.
  • Update the validate* methods to use try/catch.
  • Adjust the relationship methods to not pass in implicitly unwrapped options.
  • Adjust our usage of NSSet, Set<NSObject> to be more sane.

Feedback welcomed on this. Nothing is set in stone yet.

@justin justin self-assigned this Aug 25, 2015
@rentzsch
Copy link
Owner

At first take all of this looks great. Swift extensions can Do More than Obj-C categories, so I think they're a win now.

@jdriscoll
Copy link

How do you plan to handle optional attributes? While scalar values can be optional in Swift you can't currently mark an @NSManaged scalar property as optional. Hopefully this will change but as of Xcode 7 b6 the compiler will complain it can't be represented in Objective-C. A nil value for an optional Integer 64 will return zero if defined as Int64 in your model.

@justin
Copy link
Collaborator Author

justin commented Aug 26, 2015

Ugh. They still haven't fixed that? Welp. Scratch the all scalar all the time thing.

@justin
Copy link
Collaborator Author

justin commented Sep 7, 2015

Pull request #305 is sort of starting us down this path. Need to address it before I can move on to knocking out some Swift-ey stuff.

@frr149
Copy link

frr149 commented Dec 24, 2015

What's the time frame for making the generated code Swift 2.0 compliant? If nobody is currently working on this, I'll jump in.

@justin
Copy link
Collaborator Author

justin commented Dec 26, 2015

Here's a sample project that is based on where the default Swift templates are heading. I'm using scalar types where possible thanks to a blog post by @protocool.

https://github.com/justin/mogenerator-sample

Feedback welcomed. The next step is to convert this format into templates and then update the internals of mogenerator to generate code in this style, while still keeping things sane for Objective-C users.

@beccadax
Copy link

Thanks for asking me to chime in. A few design comments:

  • I'm not sure we want to use Int32 et al. Numeric conversions are somewhat burdensome in Swift, so typical Swift style is to simply use Int for everything. This is especially a good idea if Core Data will cause a validation failure for an out-of-bounds integer—will it? That is, if you try to store Int64.max as NSNumber into a 32-bit integer field, does Core Data complain about it during the save, or does it silently truncate?
  • The MOGeneratorEntity protocol is a good idea, but it doesn't follow the normal name rules for a protocol. MOGeneratorEntityType, I suppose?
  • I'm also wondering if entityName() should be a computed property in Swift.
  • If there's not some reason they can't be, I think the property enums should be nested inside the type they belong to—that is, HumanMO.Attributes, rather than HumanMOAttributes. With that in place, when you're writing the HumanMO class, you'll be able to say simply Attributes. (It may even be possible to make these enums requirements of MOEntityGeneratorType by specifying them as associated type requirements.)

And implementation things:

  • I see a lot of exclamation points inside the generated getters for integer properties. That makes me nervous. Swift sometimes implicitly calls the getter before a set, so I'm not sure this is safe. Can we look up the default value for that property and return that if the primitive value is nil and shouldn't be?
  • Does entity(_:) always have that same implementation? If so, it might make sense to make it a protocol extension method on MOGeneratorEntityType, rather than generating an identical implementation for every type.
  • It would be nice if we could provide overloads for various things that allow you to use the enums directly. For instance, willAccessValueForKey(Attributes.age) instead of willAccessValueForKey(Attributes.age.rawValue). This should be possible if you're willing to make MOGeneratorEntityType.swift large enough to include all the overloads and supporting code, although actually doing it is obviously nontrivial. (If we're not willing to do this, these should probably become static String properties of a struct, instead of cases of an enum; rawValue everywhere is pretty annoying.)

If I haven't talked about it, that means I like it. This is some great work!

@rentzsch
Copy link
Owner

I took a look at https://github.com/justin/mogenerator-sample. Looks Good To Me. I'm still not using Swift in anger, so I'm not the best judge on the Swift codegen side of mogenerator. I'm very happy to delegate control of Swift codegen to @justin, who's actually using it to Get Work Done.

@brentdax thanks for the insightful feedback.

@justin
Copy link
Collaborator Author

justin commented Dec 30, 2015

@brentdax I pushed a few changes:

  1. Use Int instead of int16|32|64
  2. Rename MOGeneratorEntity to MOGeneratorEntityType
  3. Move Attributes & Relationships Enums internal to classes.
  4. Move entity(managedObjectContext:) to protocol extension so we can DRY up our models.

I am not sure how to handle entityName() as a computed property in the instance that someone subclasses? ie. ParentMO and ChildMO are subclasses of HumanMO. Ideas?

Not really following the thing re the implicitly unwrapped options in the getters. Small code snippet example?

And re: the overloads, I'll leave that as an exercise for a future release. I just want to get a decent foundation for Swift in here before we go forward.

Thanks for the feedback!

@jdriscoll
Copy link

I'm opposed to using Int. Int is either 32 bit or 64 bit depending on platform and Core Data will overflow if given a larger value than it expects. If you're assuming 64 bit numbers your app will be broken on 32 bit devices. If you're looking to store 16 bit numbers your in memory storage will not reflect what will be saved to disk.

@beccadax
Copy link

beccadax commented Jan 4, 2016

@jdriscoll I was hoping that the object would fail to validate if you tried to store a number that wasn't in the type's range, but I just did a bit of playground testing and unfortunately Core Data doesn't seem to do that. So as much as I hate it, we may need to switch back to using specifically-sized types, or at least provide some kind of option to do so.

@justin I believe that you can generate the child class's entityName property with an override keyword. Or am I wrong about that? Maybe there's some sort of complication I didn't anticipate.

On the overloads: I think it's okay to push them off, but only if we really do want to implement them later. If we don't, we should just switch to String constants now; they're in practice more useful if we're not going to offer overloads. (By the way, I wouldn't mind helping to implement the overloading and other type systems surrounding this.)

What I'm worried about isn't implicitly unwrapped optionals; it's the forced unwrapping in the getters. For example, consider this getter in ChildMO:

    var type: Int16 {
        get {
            willAccessValueForKey(Attributes.type.rawValue)
            defer { didAccessValueForKey(Attributes.type.rawValue) }
            return (primitiveType?.shortValue)!
        }

The force unwrap in the return line means that, if you call this getter before you initialize type (or primitiveType), it will crash. Now, in theory, that's perfectly appropriate behavior—you're trying to access something you haven't initialized. But in practice, I suspect it will cause trouble. In particular, I've seen hints on the swift-evolution list that, in the future, they will not provide direct access to the setter as a function, but will instead offer an (inout T -> Void) -> Void "lens" which can modify it in-place. Like any inout-based function, this would mean that it would call the getter, pass in the value, and then call the setter with the modified value—and if the getter force-unwraps an optional, that could be trouble.

@groue
Copy link

groue commented Jan 4, 2016

In particular, I've seen hints on the swift-evolution list that...

If I were you, I would get much more specific, or I would stop worrying. In both cases, don't hesitate: share the link to the swift-evolution thread, and be vocal about your concern for mogenerator. Or write your getter in a way that works now, regardless of the possible futures of Swift. I guess the latter solution is the one that is the most wanted by mogenerator users :-).

@protocool
Copy link
Contributor

@brentdax / @justin the custom accessor for ChildMO.type is unnecessary (and, as written, incorrect) if it's simply declaring a scalar type that's compatible with Objective-C.

@NSManaged var type: Int16 will behave the same as @property (assign) int16_t type;, transparently converting nil primitive values to 0.

Obviously nil or garbage primitive values can still be an issue for other property types, such as enums. I didn't specifically talk about it in my article, but in those cases I generate getters that fallback to a meaningful hard-coded value, as in: return primitiveSomething.flatMap { Something(rawValue: $0.integerValue) } ?? .AnAppropriateDefault

@beccadax
Copy link

beccadax commented Jan 4, 2016

@groue There's no formal proposal, simply some rumblings from the core team. This post references the idea a little bit obliquely.

@protocool If the @NSManaged approach works, we should use it. I assumed there was some reason we were defining our own.

@jdriscoll
Copy link

@protocool The reason is to prevent nil being converted to 0 and allowing for optional scalar properties on your managed objects. This is needed when a value can either be null OR 0 and both are valid.

@protocool
Copy link
Contributor

@jdriscoll understood. In this case ChildMO.type is declared to Swift as an Int16 so a null primitive value must either be treated as exceptional or coerced to a reasonable representation of null for the type.

Calling the current implementation 'incorrect' was a bit of an overstatement. It's more that it is inconsistent with the default generated scalar accessor, which coerces to 0.

@frr149
Copy link

frr149 commented Jan 7, 2016

What is the rationale for using an implicitly unwrapped Optional in this init?
init(entity: NSEntityDescription, insertIntoManagedObjectContext context: NSManagedObjectContext!)

Why not use NSManagedObjectContext?

@justin
Copy link
Collaborator Author

justin commented Jan 17, 2016

    var type: Int16 {
        get {
            willAccessValueForKey(Attributes.type.rawValue)
            defer { didAccessValueForKey(Attributes.type.rawValue) }

            if let t = primitiveType {
                return t.shortValue
            }

            return -666
        }
        set {
            willChangeValueForKey(Attributes.type.rawValue)
            defer { didChangeValueForKey(Attributes.type.rawValue) }
            primitiveType = NSNumber(short: newValue)
        }
    }
    @NSManaged private var primitiveType: NSNumber?

In the case of getting around the implicitly unwrapped options, I'm honestly not sure what to put for the return in the extreme case that you don't get a value back. Nothing really stands out as the proper solution to me. Feedback welcomed.

@justin
Copy link
Collaborator Author

justin commented Jan 17, 2016

@protocool
Copy link
Contributor

@justin are you specifically trying to solve the problem of non-optional scalar properties here or is this just your example of dealing with the general case of nil primitive values when the property is non-optional?

Because if you're actually trying to solve the issue of a non-optional Int16 (and friends), then Core Data already handles that: declare the property as @NSManaged var type: Int16 and any nil primitiveType is automatically coerced to 0 by the accessor that Core Data generates.

If you're trying to construct a general-case solution for non-optional, non-scalar properties which cannot be expressed in Objective-C, such as an enum ThingType: Int16, then you're already relying on 'custom' factory code, so IMO it's reasonable to enforce a rule which says that if you supply a default value in the attribute's userInfo, it will be used in a flatMap-or (which I described in #302 (comment) a few weeks back), otherwise the value will be implicitly unwrapped.

@protocool
Copy link
Contributor

@frr149 WRT your question about the signature for the init (and in case you didn't come across the answer yourself), that's the signature for the designated initializer of NSManagedObject.

@akabab
Copy link

akabab commented Apr 13, 2016

any update on swift template? Is it possible to use the template used for mogenerator-sample ?

@justin
Copy link
Collaborator Author

justin commented Apr 13, 2016

Not presently. I work on it as I have time.

@pip8786
Copy link

pip8786 commented May 2, 2016

Is there a way for Swift generation to use Set instead of NSSet? If not, would it be useful for me to try and contribute that (if it's possible)? As of right now I just changed it in my machine files and turned off auto generation on every build.

@rentzsch
Copy link
Owner

rentzsch commented May 2, 2016

@pip8786 I don't know the tradeoffs, we'll see what @justin has to say.

In terms of your own templates, it's really easy to use your own variant that uses Set instead NSSet. Just download the files in mogenerator's /template git dir, hack them up to your liking and then pass --template-path /path/to/templates-hacked/ to mogenerator.

@pip8786
Copy link

pip8786 commented May 2, 2016

@rentzsch Thanks for the tip on using templates.

I realize now that I didn't even mention that the big reason for wanting this was to have generics as well so that I don't have to cast anything coming out of the Set. Looking through the code it looks like they're left out specifically for Swift code generation (line 591 of mogenerator.m). Was there a reason for that?

@Igor-Palaguta
Copy link

Guys, as an experiment added MotoSwift

It allows to specify file name mask
It uses simple Stencil as template language
It uses scalar types
Allows to customize templates with Entity/Attribute/Relationship/FetchedProperty userInfo

@makleso6
Copy link

Hi! Any сhanges / releases? Scalars? Swift extension?

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