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-2338] Compiler Crash on Implementing Optional convenience init of CGMutablePath #44945

Closed
swift-ci opened this issue Aug 14, 2016 · 22 comments

Comments

@swift-ci
Copy link
Collaborator

swift-ci commented Aug 14, 2016

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
Copy link
Contributor

belkadan commented Aug 15, 2016

@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
Copy link
Mannequin

milseman mannequin commented Aug 15, 2016

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
Copy link
Contributor

belkadan commented Aug 15, 2016

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

@belkadan
Copy link
Contributor

belkadan commented Aug 15, 2016

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

@milseman
Copy link
Mannequin

milseman mannequin commented Aug 15, 2016

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
Copy link
Contributor

belkadan commented Aug 15, 2016

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

@slavapestov
Copy link
Member

slavapestov commented Aug 16, 2016

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
Copy link
Collaborator Author

swift-ci commented Aug 16, 2016

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
Copy link
Contributor

belkadan commented Aug 16, 2016

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
Copy link
Member

slavapestov commented Aug 16, 2016

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

@milseman
Copy link
Mannequin

milseman mannequin commented Aug 17, 2016

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
Copy link
Mannequin

milseman mannequin commented Aug 17, 2016

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
Copy link
Contributor

belkadan commented Aug 17, 2016

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
Copy link
Member

slavapestov commented Aug 17, 2016

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

@belkadan
Copy link
Contributor

belkadan commented Aug 18, 2016

CF types do not have public class symbols, no.

@belkadan
Copy link
Contributor

belkadan commented Aug 18, 2016

(or guaranteed runtime names)

@slavapestov
Copy link
Member

slavapestov commented Aug 18, 2016

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
Copy link
Contributor

belkadan commented Aug 18, 2016

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

@slavapestov
Copy link
Member

slavapestov commented Aug 18, 2016

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
Copy link
Mannequin

milseman mannequin commented Aug 19, 2016

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
Copy link
Contributor

belkadan commented Aug 20, 2016

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
Copy link
Mannequin

milseman mannequin commented Apr 10, 2019

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

@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

3 participants