Plumb through support for 'usable from inline' imports #16211

slavapestov · 2018-04-27T07:22:27Z

Progress on rdar://problem/39338239; we still need to infer this attribute though.

slavapestov · 2018-05-01T01:01:40Z

@swift-ci Please test

slavapestov · 2018-05-01T01:01:46Z

@swift-ci Please test source compatibility

shahmishal · 2018-05-01T01:50:23Z

@swift-ci Please test source compatibility

jrose-apple · 2018-05-01T16:03:59Z

Sorry, I have no idea what this is for. Maybe I'll talk to you about it in person.

slavapestov · 2018-05-01T22:52:55Z

@jrose-apple I renamed the attribute as you requested. Hopefully you have an idea of what this is for now that we've talked in person :-)

slavapestov · 2018-05-01T22:53:01Z

@swift-ci Please smoke test

slavapestov · 2018-05-01T22:53:06Z

@swift-ci Please smoke test

jrose-apple · 2018-05-01T23:15:54Z

include/swift/AST/Module.h

    Public,
-    Private
+
+    // Not @_exported only. Also includes @_usableFromInline.


Hm, if these categories already aren't mutually exclusive, I think I'd suggest having UsableFromInline also include Public, as we discussed (which might suggest calling it "ForLinking" or something instead).

jrose-apple · 2018-05-01T23:17:01Z

include/swift/AST/Module.h

  void
  getImportedModulesForLookup(SmallVectorImpl<ImportedModule> &imports) const;

+  /// Extension of the above hack. Identical to getImportedModulesForLookup()
+  /// for imported modules, otherwise also includes @usableFromInline imports.


Swift modules are also "imported modules" as far as AST goes, but either way it seems like an implementation detail that's not worth mentioning here. Those functions are virtual for a reason.

I meant "Clang imported modules" I guess. I'll change the doc comment in the next PR.

jrose-apple · 2018-05-01T23:17:49Z

include/swift/AST/Module.h

  ///
  /// \return True if the traversal ran to completion, false if it ended early
  ///         due to the callback.
  bool forAllVisibleModules(AccessPathTy topLevelAccessPath,
-                            llvm::function_ref<bool(ImportedModule)> fn);
+                            llvm::function_ref<bool(ImportedModule)> fn,
+                            bool includeLinkOnlyModules = false);


*sigh* Should this just take an ImportFilter at this point?

It's not quite an ImportFilter, because it's used to decide if we want to call getImportedModulesForLookup() vs getImportedModulesForLinking(). Since those two are only overridden by ClangModuleUnit and the comments allude to some caching they do, I suspect it could be refactored further, but maybe we can tackle this later.

jrose-apple · 2018-05-01T23:18:30Z

include/swift/AST/Module.h

+  virtual void getImportedModulesForLinking(
+      SmallVectorImpl<ModuleDecl::ImportedModule> &imports) const {
+    return getImportedModules(imports, ModuleDecl::ImportFilter::Public);
+    return getImportedModules(imports, ModuleDecl::ImportFilter::UsableFromInline);


Copy/paste error here. Should there have been a test that catches this, or is it just that this is overridden everywhere in practice?

I'm not sure what the error is -- the set of modules for linking includes public (@_exported) imports, and @_usableFromInline imports.

There's a return, so the second line never happens.

Oops, thanks. Yeah, I'll add a test. You need to have both an @_exported and @_usableFromInline import.

jrose-apple · 2018-05-01T23:28:49Z

lib/Sema/TypeCheckAttr.cpp

@@ -842,6 +843,7 @@ class AttributeChecker : public AttributeVisitor<AttributeChecker> {
    IGNORED_ATTR(Transparent)
    IGNORED_ATTR(WarnUnqualifiedAccess)
    IGNORED_ATTR(WeakLinked)
+    IGNORED_ATTR(UsableFromInlineImport)


Nitpick: This particular list is alphabetized; please keep it that way.

jrose-apple · 2018-05-01T23:29:44Z

lib/Serialization/ModuleFile.cpp

@@ -1666,6 +1675,9 @@ void ModuleFile::getImportDecls(SmallVectorImpl<Decl *> &Results) {
      if (Dep.isExported())
        ID->getAttrs().add(
            new (Ctx) ExportedAttr(/*IsImplicit=*/false));
+      if (Dep.isUsableFromInline())
+        ID->getAttrs().add(
+            new (Ctx) UsableFromInlineAttr(/*IsImplicit=*/false));


This should be UsableFromInlineImportAttr…and should probably be marked implicit because we don't want to show them to users at this time.

Sure. How can I test this?

test/IDE/print_clang_framework_with_overlay.swift uses swift-ide-test to print a Clang framework's imports; you should be able to do the same sort of thing for a Swift module.

ModuleDecl::forAllVisibleModules() now has a includeLinkOnlyModules parameter. This is intended to be used when computing the set of libraries to autolink.

Progress on <rdar://problem/39338239>; we still need to infer this attribute though.

slavapestov · 2018-05-02T00:56:45Z

@swift-ci Please smoke test

slavapestov · 2018-05-02T00:56:56Z

@swift-ci Please test

jrose-apple

Looks good enough for me. Inference part next, right?

jrose-apple · 2018-05-02T00:59:20Z

(This doc comment comment didn't get addressed though: https://github.com/apple/swift/pull/16211/files#r185358916)

swift-ci · 2018-05-02T00:59:52Z

Build failed
Swift Test OS X Platform
Git Sha - e8ca0db52333aab69919edec4de0545f2afda5b8

swift-ci · 2018-05-02T00:59:52Z

Build failed
Swift Test Linux Platform
Git Sha - e8ca0db52333aab69919edec4de0545f2afda5b8

slavapestov · 2018-05-02T01:05:18Z

@swift-ci Please test source compatibility

slavapestov · 2018-05-02T01:08:49Z

@swift-ci Please test source compatibility

…autolinking Plumb through support for 'usable from inline' imports (cherry picked from commit bb16ee0)

…able-vs-autolinking" This reverts commit bb16ee0, reversing changes made to a8d831f. It's not sufficient to solve the problem, and the choices were to do something more complicated, or just take a simple brute force approach. We're going with the latter.

slavapestov force-pushed the fix-inlinable-vs-autolinking branch 2 times, most recently from aad64a6 to e8ca0db Compare May 1, 2018 00:49

slavapestov changed the title ~~[WIP] Fix @inlinable -vs- autolinking~~ Plumb through support for 'usable from inline' imports May 1, 2018

slavapestov requested a review from jrose-apple May 1, 2018 04:10

slavapestov force-pushed the fix-inlinable-vs-autolinking branch from e8ca0db to 5d21079 Compare May 1, 2018 22:49

jrose-apple reviewed May 1, 2018

View reviewed changes

slavapestov added 2 commits May 1, 2018 17:53

AST: Support '@_usableFromInline import'

7539674

AST: Introduce ModuleDecl::ImportFilter::ForLinking

cfa0ab3

ModuleDecl::forAllVisibleModules() now has a includeLinkOnlyModules parameter. This is intended to be used when computing the set of libraries to autolink.

slavapestov force-pushed the fix-inlinable-vs-autolinking branch from 5d21079 to 36e045e Compare May 2, 2018 00:54

Serialization: Support '@_usableFromInline import'

db44012

Progress on <rdar://problem/39338239>; we still need to infer this attribute though.

slavapestov force-pushed the fix-inlinable-vs-autolinking branch from 36e045e to db44012 Compare May 2, 2018 00:56

jrose-apple approved these changes May 2, 2018

View reviewed changes

slavapestov merged commit bb16ee0 into swiftlang:master May 2, 2018

slavapestov mentioned this pull request May 2, 2018

Autolink libraries referenced from default arguments and @inlinable bodies [4.2] #16301

Closed

jrose-apple mentioned this pull request May 2, 2018

Infer imports for linking #16317

Closed

jrose-apple pushed a commit to jrose-apple/swift that referenced this pull request May 3, 2018

Merge pull request swiftlang#16211 from slavapestov/fix-inlinable-vs-…

ccf11e0

…autolinking Plumb through support for 'usable from inline' imports (cherry picked from commit bb16ee0)

jrose-apple pushed a commit to jrose-apple/swift that referenced this pull request May 3, 2018

Merge pull request swiftlang#16211 from slavapestov/fix-inlinable-vs-…

3d9b67a

…autolinking Plumb through support for 'usable from inline' imports (cherry picked from commit bb16ee0)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Plumb through support for 'usable from inline' imports #16211

Plumb through support for 'usable from inline' imports #16211

slavapestov commented Apr 27, 2018 •

edited

Loading

slavapestov commented May 1, 2018

slavapestov commented May 1, 2018

shahmishal commented May 1, 2018

jrose-apple commented May 1, 2018

slavapestov commented May 1, 2018

slavapestov commented May 1, 2018

slavapestov commented May 1, 2018

jrose-apple May 1, 2018

slavapestov May 1, 2018

jrose-apple May 1, 2018

slavapestov May 2, 2018

jrose-apple May 1, 2018

slavapestov May 2, 2018

jrose-apple May 1, 2018

slavapestov May 1, 2018

jrose-apple May 1, 2018

slavapestov May 1, 2018

jrose-apple May 1, 2018

slavapestov May 1, 2018

jrose-apple May 1, 2018

slavapestov May 1, 2018

jrose-apple May 2, 2018

slavapestov May 2, 2018

slavapestov commented May 2, 2018

slavapestov commented May 2, 2018

jrose-apple left a comment

jrose-apple commented May 2, 2018

swift-ci commented May 2, 2018

swift-ci commented May 2, 2018

slavapestov commented May 2, 2018

slavapestov commented May 2, 2018

Plumb through support for 'usable from inline' imports #16211

Plumb through support for 'usable from inline' imports #16211

Conversation

slavapestov commented Apr 27, 2018 • edited Loading

slavapestov commented May 1, 2018

slavapestov commented May 1, 2018

shahmishal commented May 1, 2018

jrose-apple commented May 1, 2018

slavapestov commented May 1, 2018

slavapestov commented May 1, 2018

slavapestov commented May 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slavapestov commented May 2, 2018

slavapestov commented May 2, 2018

jrose-apple left a comment

Choose a reason for hiding this comment

jrose-apple commented May 2, 2018

swift-ci commented May 2, 2018

swift-ci commented May 2, 2018

slavapestov commented May 2, 2018

slavapestov commented May 2, 2018

slavapestov commented Apr 27, 2018 •

edited

Loading