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-7578] offer a way to use @inlineable/@_inlineable and @usableFromInline/@_versioned in Swift 4.0, 4.1 and 4.2 #50120

Closed
weissi opened this issue May 1, 2018 · 22 comments
Assignees
Labels
bug A deviation from expected or documented behavior Compiler

Comments

@weissi
Copy link
Member

weissi commented May 1, 2018

Previous ID SR-7578
Radar rdar://problem/40717640
Original Reporter @weissi
Type Bug
Status Resolved
Resolution Done
Additional Detail from JIRA
Votes 1
Component/s Compiler
Labels Bug
Assignee @slavapestov
Priority Medium

md5: 84224d686477c1b60b43776fd5ff3d40

Issue Description:

For SwiftNIO we absolutely rely on @inlineable/@_inlineable and @usableFromInline/@_versioned to get any sensible performance for our users (which can't put their code in the NIO module). Needless to say we were very happy when the public version of this finally arrived. The problem however is that there doesn't seem to be a workable way to get it to work without warning on Swift 4.0/4.1 and 4.2.

I know there technically is one possibility:

#if swift(>=4.2)
@inlineable
func myFunc() {
   /* whole body here */
}
#else
@_inlineable
func myFunc() {
  /* REPEAT whole body here */
#endif

but that would mean to duplicate the whole body of every type/function/anything that is inlineable! That's totally unworkable unfortunately without starting to code generate basically the whole project...

The only way out I see would be to remove the warning as we can't change Swift 4.0 and 4.1 anymore now. Any other ideas?

@belkadan
Copy link
Contributor

belkadan commented May 1, 2018

I don't really know what to say here. What you were doing isn't a supported use case. @slavapestov?

@weissi
Copy link
Member Author

weissi commented May 1, 2018

@belkadan I understand that but without that it'd be so slow that it's unusable...

@weissi
Copy link
Member Author

weissi commented May 5, 2018

ping @slavapestov, this will unfortunately get very important for us as soon as Swift 4.2 will be released. The only (really bad) option I see to remain warning-free is to change our @_inlineable s to @inline(_always) which is really wrong and bad. But we can't just remove them and regress really badly in performance for all of our users.

@weissi
Copy link
Member Author

weissi commented May 5, 2018

ok, what we could do is release SwiftNIO 2.0 with Swift 4.2 an just drop Swift 4.0 support, if we can get the @inlineable and @usableFromInline spellings into Swift 4.1.(n+1) but I reckon that's quite unlikely too, right?

@belkadan
Copy link
Contributor

belkadan commented May 7, 2018

Apart from only taking maintenance fixes at this point (if even that), Swift 4.1 had a bunch of bugs around inlining that we fixed in Swift 4.2. So no, we're not going to add the new spelling to the Swift 4.1 series.

@slavapestov
Copy link
Member

slavapestov commented May 8, 2018

Yeah, your best bet is dropping Swift 4.0/4.1 support as soon as 4.2 is out, or living with warnings.

@weissi
Copy link
Member Author

weissi commented May 14, 2018

@slavapestov actually, we do explicitly ask for the Swift language version 4 which seems to mean 4.0 and 4.1 but not 4.2. So why can't we have this warning only in 4.2?

swift-version 4:

$ swiftc -swift-version 4 test.swift
test.swift:1:2: warning: '@_inlineable' has been renamed to '@inlinable'
@_inlineable
 ^~~~~~~~~~~
 inlinable

seems to be the same as swift-version 4.1 (which is illegal)

$ swiftc -swift-version 4.1 test.swift
<unknown>:0: error: invalid value '4.1' in '-swift-version 4.1'
<unknown>:0: note: use major version, as in '-swift-version 4'
test.swift:1:2: warning: '@_inlineable' has been renamed to '@inlinable'
@_inlineable
 ^~~~~~~~~~~
 inlinable

but 4.2 is not:

$ swiftc -swift-version 4.2 test.swift
test.swift:1:2: warning: '@_inlineable' has been renamed to '@inlinable'
@_inlineable
 ^~~~~~~~~~~
 inlinable

so really, this warning should not be enabled for swift-version 4 but can be enabled for 4.2

@slavapestov
Copy link
Member

slavapestov commented May 14, 2018

Why is it important to compile without warnings on older releases of Swift?

@weissi
Copy link
Member Author

weissi commented May 14, 2018

@slavapestov the problem is that we'll compile with warnings on newer releases of Swift. If it were only for older releases that might be acceptable because at least the warning is actionable (ie. upgrade to get rid of it).

But generally the problems with warnings is that many people want to compile with 'warnings to errors' (especially in CI) which many consider to be good style. But even without it's really bad if you get 100 warnings each time you build. You're bound to miss one warning that is actually important.

We seem to have two bad options:

  1. spell it @_inlineable and compile fine on 4.0 & 4.1 but with loads of warnings on 4.2

  2. spell it @inlinable and only compile on 4.2

My argument is that 4.2 already got its on -swift-version anyway so I don't understand what's wrong with having both spellings be fine for -swift-version 4 but issuing a warning on -swift-version 4.2 for the old spelling.

@slavapestov
Copy link
Member

slavapestov commented May 14, 2018

Ok, you win 🙂 I'll look into muffling the warning in 4.2 mode. There is another issue where 4.1 did not enforce @usableFromInline on type aliases correctly; I might have to bypass that enforcement in -swift-version 4 as well.

@belkadan
Copy link
Contributor

belkadan commented May 14, 2018

I think it's okay to add correct enforcements to the model as long as you can deal with them in 4.1.

@weissi
Copy link
Member Author

weissi commented May 15, 2018

@slavapestov Thank you very much, that makes the whole situation much much better. Ideally, we want the higher-level libraries to adopt new Swift versions the fastest and usually I think that's what's happening as higher level libraries want to use cool new language features like say Codable more often than low-level libraries. And when most clients have deprecated a certain old Swift version low-level libraries such as SwiftNIO can follow and also drop those versions asap. In the case of @inlinable however it was a bit unfortunate as the higher-level libraries don't need them but the low-level ones really do.

@weissi
Copy link
Member Author

weissi commented Jun 1, 2018

@swift-ci create

@slavapestov
Copy link
Member

slavapestov commented Jun 11, 2018

#17090

@belkadan
Copy link
Contributor

belkadan commented Jun 11, 2018

4.2 branch?

@slavapestov
Copy link
Member

slavapestov commented Jun 11, 2018

#17091

@belkadan
Copy link
Contributor

belkadan commented Jun 11, 2018

👍

@weissi
Copy link
Member Author

weissi commented Jun 12, 2018

thanks you @slavapestov, that's awesome!!

@weissi
Copy link
Member Author

weissi commented Jun 18, 2018

@slavapestov thanks very much for that. Just tried to compile NIO again and loads of warnings are indeed gone. However there are still a lot of warnings left:

/Users/johannes/devel/swift-nio/Sources/NIO/ByteBuffer-core.swift:348:18: warning: '@inlinable' declaration is already '@usableFromInline'
    @_inlineable @_versioned
                 ^~~~~~~~~~~

which is correct. Removing the @_versioned however leads to a compile error in 4.1:

$ jw-swift-4.1 swift build
Compile CNIOLinux shim.c
Compile CNIOSHA1 c_nio_sha1.c
Compile CNIODarwin shim.c
Compile CNIOZlib empty.c
Compile CNIOAtomics src/c-atomics.c
Compile CNIOHTTPParser c_nio_http_parser.c
Compile Swift Module 'NIOPriorityQueue' (2 sources)
Compile Swift Module 'NIOConcurrencyHelpers' (2 sources)
Compile Swift Module 'NIO' (52 sources)
/Users/johannes/devel/swift-nio/Sources/NIO/ByteBuffer-core.swift:354:5: error: '@_inlineable' attribute can only be applied to public declarations, but '_moveWriterIndex' is internal
    @_inlineable
    ^~~~~~~~~~~~
    
/Users/johannes/devel/swift-nio/Sources/NIO/ByteBuffer-core.swift:354:5: error: '@_inlineable' attribute can only be applied to public declarations, but '_moveWriterIndex' is internal
    @_inlineable
    ^~~~~~~~~~~~
    
/Users/johannes/devel/swift-nio/Sources/NIO/ByteBuffer-core.swift:363:14: error: instance method '_moveWriterIndex(to:)' is internal and cannot be referenced from an '@_inlineable' function
        self._moveWriterIndex(to: newIndex)
             ^
/Users/johannes/devel/swift-nio/Sources/NIO/ByteBuffer-core.swift:355:19: note: instance method '_moveWriterIndex(to:)' is not '@_versioned' or public
    mutating func _moveWriterIndex(to newIndex: Index) {
                  ^
/Users/johannes/devel/swift-nio/Sources/NIO/ByteBuffer-core.swift:483:14: error: instance method '_moveWriterIndex(to:)' is internal and cannot be referenced from an '@_inlineable' function
        self._moveWriterIndex(to: self._writerIndex + _toIndex(bytesWritten))
             ^
/Users/johannes/devel/swift-nio/Sources/NIO/ByteBuffer-core.swift:355:19: note: instance method '_moveWriterIndex(to:)' is not '@_versioned' or public
    mutating func _moveWriterIndex(to newIndex: Index) {
                  ^
/Users/johannes/devel/swift-nio/Sources/NIO/ByteBuffer-core.swift:354:5: error: '@_inlineable' attribute can only be applied to public declarations, but '_moveWriterIndex' is internal
    @_inlineable
    ^~~~~~~~~~~~
    
error: terminated(1): /Library/Developer/Toolchains/swift-4.1-RELEASE.xctoolchain/usr/bin/swift-build-tool -f /Users/johannes/devel/swift-nio/.build/debug.yaml main output:

so the cheapest way to fix this is probably to ignore this warning too in language mode 4.

@slavapestov
Copy link
Member

slavapestov commented Jun 19, 2018

#17314

@weissi
Copy link
Member Author

weissi commented Jun 19, 2018

thanks very much!

@weissi
Copy link
Member Author

weissi commented Jun 25, 2018

confirmed working now on very recent 4.2 snapshots.

@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
Labels
bug A deviation from expected or documented behavior Compiler
Projects
None yet
Development

No branches or pull requests

3 participants