[SR-2338] Compiler Crash on Implementing Optional convenience init of CGMutablePath

[swift-ci]

Previous ID	SR-2338
Radar	None
Original Reporter	Glenn.Howes (JIRA User)
Type	Bug
Status	Closed
Resolution	Done

Environment

OSX 10.11.6 (15G31), Xcode tools 8.0 (8S193k), Apple Swift version 3.0 (swiftlang-800.0.42.1 clang-800.0.36.1)

Additional Detail from JIRA

Votes	0
Component/s	Compiler
Labels	Bug, CompilerCrash
Assignee	@milseman
Priority	Medium

md5: f6596743c7cce37ddcb47a3028c32f0c

relates to:

• SR-1053 Implement SE-0044: Import as member

Issue Description:

From the command line, enter the following:

new-host-4:SignalGH glenn$ swift
Welcome to Apple Swift version 3.0 (swiftlang-800.0.42.1 clang-800.0.36.1). Type :help for assistance.
  1> import CoreGraphics
  2>  
  3> extension CGMutablePath 
  4. { 
  5.     public func addSVGPath(svgPath: String) -> Bool 
  6.     { 
  7.         return true 
  8.     } 
  9.      
 10.     public convenience init?(svgPath: String) 
 11.     { 
 12.         self.init() 
 13.         self.addSVGPath(svgPath: svgPath) 
 14.     } 
 15. } 

Bus error: 10

This also crashes the compiler in the current Xcode 8 beta (8.0 beta 5 (8S193k)), but I thought it would be simpler to provide the command line equivalent.

[belkadan]

@milseman, @slavapestov, essentially the same case as the Dispatch ones, but also happens in Release? Though SILGen just goes off the rails here.

Assertion failed: (memberStatus.isInstance() && "static method with non-metatype self?!"), function getLoweredASTFunctionType, file /Volumes/Data/swift-public/swift/lib/SIL/SILFunctionType.cpp, line 2486.

1.  While silgen constructor initializer SIL function @_TFE4mainCSo13CGMutablePathcfT7svgPathSS_GSqS0__ for 'init' at <stdin>:10:24

[milseman]

Yes, this seems related, and is a good reason to try to prioritize the fix for Swift 3.0, if we can fit it in.

[belkadan]

It's not a regression, so I'm fine with it landing later.

[belkadan]

(Most CF types don't have initializers at all. CG is special because of the import-as-member proposal.)

[milseman]

So, previously if instead this code was:

public convenience init?(svgPath: String) { 
    self = CGPathCreateMutable()
    self.addSVGPath(svgPath: svgPath) 
} 

It also wouldn't work? Then I agree.

[belkadan]

Yep, it's a class type, so you can't assign to self.

[slavapestov]

I'll fix the SILGen crash, but is this supposed to work at all?

It would be nice to be able to assign to self in class convenience initializers also, as long as the class is final. We can't subclass foreign classes, can we?

[swift-ci]

Comment by Glenn Howes (JIRA)

@slavapestov are you asking if the following is supposed to be valid code? Why shouldn't it be?

extension CGMutablePath 
{ 
       public func addSVGPath(svgPath: String) -> Bool 
       { 
              return true 
       } 
     
        public convenience init?(svgPath: String) 
        { 
               self.init() 
               if !self.addSVGPath(svgPath: svgPath) 
               {
                    return nil 
               } 
       }
} 

[belkadan]

Doing this for non-foreign classes (and allowing self-assignment) requires deciding what factory initializers mean in Swift. Just doing it for foreign classes (only with self.init syntax) means we get to kick that discussion a bit further down the road.

[slavapestov]

Actually @belkadan, this is similar to what the dispatch overlay does, so the fact that it crashes must be a bug.

[milseman]

I opened: #4335

This fixes the SILGen assert (by fixing how we import the init). There may be other bugs deeper down, though. I'll investigate further.

[milseman]

While that PR will fix the provided code, actually trying to use it triggers an IRGen assert:

Assertion failed: (!theClass->isForeign()), function emitObjCHeapMetadataRef, file ...swift/lib/IRGen/GenMeta.cpp, line 339.

When trying to IRGen this:
%0 = alloc_ref [objc] $CGMutablePath, scope 0 // users: %3, %4

So, there's more layers here. I'll see how this differs from Dispatch

[belkadan]

This is the "we need to not even try to allocate foreign classes" bit. I think Slava worked around this for Dispatch by special-casing RuntimeOnly foreign classes, whose metadata you can get by run-time lookup, but I don't think that's the correct answer even there.

[slavapestov]

So CGMutablePath does not have Objective-C metadata at all? Interesting...

[belkadan]

CF types do not have public class symbols, no.

[belkadan]

(or guaranteed runtime names)

[slavapestov]

We could emit a fake allocation of some kind. The only thing that's done to it is alloc, dealloc, and possibly retain and release.

[belkadan]

There's no reason to do any allocation at all. We have a mechanism for this already: factory initializers.

[slavapestov]

Right, I agree we should do it properly if possible. Do you think it would be hard to teach SILGen to emit user-defined convenience inits as factory inits, in the case where they delegate to another factory init? Does this create any issues for PrintAsObjC and such?

[milseman]

This will not be supported in Swift 3, because we don't have the concept of user factory inits. This will emit an error in Swift-3, and hopefully we'll get support soon after. #4419

[belkadan]

Slava's approach is what I was thinking, except I was going to make it based on whether the type was an AST-foreign type rather than whether the thing it delegated to was a factory init. (Normal convenience initializers are still inherited, while factory inits are not.)

[milseman]

This properly emits error: convenience initializers are not supported in extensions of CF types