[ClangImporter] Add direct access for import-as-member types. #10956

jrose-apple · 2017-07-14T00:27:18Z

This avoids having to bring in all members (and extensions!) for an outer type just to look up a nested type. In the test case attached (reduced from the project in SR-5284), this actually led to a circular dependency between deserialization and the importer, which resulted in a compiler crash.

This is not a new problem, but it's more important with the release of Swift 4, where a number of Apple SDK types are now newly imported as member types. (The one in the original bug was NSView.AutoresizingMask, formerly NSAutoresizingMaskOptions.) Since we always use the Swift 4 name for cross-references, this affected everyone, even those still compiling in Swift 3 mode.

SR-5284 / rdar://problem/32926560

jrose-apple · 2017-07-14T00:27:25Z

@swift-ci Please test

jrose-apple · 2017-07-14T00:27:32Z

@swift-ci Please test source compatibility

swift-ci · 2017-07-14T00:32:07Z

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - 6dcd4542c8be3be857a53f39ec0c1b0edcb6ce3a
Test requested by - @jrose-apple

We use this to avoid circularity issues in serialization; we'd like to extend that to the Clang importer. This is only necessary because we can't look up a single member at a time, but it can still fix issues in the short term. This commit should have no effect on functionality.

swift-ci · 2017-07-14T00:44:26Z

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - 6dcd4542c8be3be857a53f39ec0c1b0edcb6ce3a
Test requested by - @jrose-apple

This avoids having to bring in all members (and extensions!) for an outer type just to look up a nested type. In the test case attached (reduced from the project in SR-5284), this actually led to a circular dependency between deserialization and the importer, which resulted in a compiler crash. This is not a new problem, but it's more important with the release of Swift 4, where a number of Apple SDK types are now newly imported as member types. (The one in the original bug was NSView.AutoresizingMask, formerly NSAutoresizingMaskOptions.) Since we always use the Swift 4 name for cross-references, this affected everyone, even those still compiling in Swift 3 mode. https://bugs.swift.org/browse/SR-5284

jrose-apple · 2017-07-14T01:01:29Z

@swift-ci Please test

jrose-apple · 2017-07-14T01:01:35Z

@swift-ci Please test source compatibility

swift-ci · 2017-07-14T01:01:53Z

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - 6dcd4542c8be3be857a53f39ec0c1b0edcb6ce3a
Test requested by - @jrose-apple

swift-ci · 2017-07-14T01:04:24Z

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - 6dcd4542c8be3be857a53f39ec0c1b0edcb6ce3a
Test requested by - @jrose-apple

jrose-apple · 2017-07-14T01:07:13Z

@swift-ci Please test source compatibility

jrose-apple · 2017-07-14T02:54:23Z

@swift-ci Please test Linux

swift-ci · 2017-07-14T02:57:34Z

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - 6faac2a3beb2b517480d4784d0d7eeae77876a99
Test requested by - @jrose-apple

jrose-apple · 2017-07-14T16:29:15Z

I've gotten this twice now but I didn't change anything there. Very confused.

19:58:26 
lib/libswiftSema.a(TypeCheckDecl.cpp.o):/home/buildnode/jenkins/workspace/swift-PR-Linux/branch-master/swift/lib/Sema/TypeCheckDecl.cpp:function (anonymous namespace)::DeclChecker::visitClassDecl(swift::ClassDecl*): error: undefined reference to 'swift::SerializedASTFile::getLanguageVersionBuiltWith() const'
19:58:26 
lib/libswiftSema.a(TypeCheckDecl.cpp.o):/home/buildnode/jenkins/workspace/swift-PR-Linux/branch-master/swift/lib/Sema/TypeCheckDecl.cpp:function (anonymous namespace)::DeclChecker::visitClassDecl(swift::ClassDecl*): error: undefined reference to 'swift::SerializedASTFile::getLanguageVersionBuiltWith() const'

@swift-ci Please clean test Linux

swift-ci · 2017-07-14T16:46:57Z

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - 6faac2a3beb2b517480d4784d0d7eeae77876a99
Test requested by - @jrose-apple

jrose-apple · 2017-07-14T17:10:08Z

*sigh* #10968, though I don't know why my PR is revealing the issue.

milseman

I'm a little out of my depth reviewing this, so hopefully Doug can provide feedback as well. I do have some questions.

milseman · 2017-07-14T17:15:44Z

include/swift/AST/Module.h

+  ///
+  /// This is a fast-path hack to avoid circular dependencies in deserialization
+  /// and the Clang importer.
+  ///


Is this used as a failable fast path? That is, if this succeeds we're fast and if it returns nullptr we go through slower lookups?

That's correct. It could probably be non-failable but since the eventual goal is to remove it altogether…no reason not to be cautious.

milseman · 2017-07-14T17:17:52Z

lib/Serialization/Deserialization.cpp

+          auto *containingFile =
+            dyn_cast<FileUnit>(dc->getModuleScopeContext());
+          if (containingFile) {
+            nestedType = containingFile->lookupNestedType(memberName, baseType);
          }


Can any of this logic be factored out into a separate function with early returns? Perhaps return when the nested type is actually found? I'm having a hard time understanding the logic here. I realized that refactoring this method is probably outside the scope of this PR.

That would certainly be reasonable; resolveCrossReference is getting huge. But yeah, this part is supposed to be a small tweak to what's already there.

milseman · 2017-07-14T17:20:55Z

test/Serialization/xref-nested-clang-type.swift

+// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk) -emit-module -o %t -import-objc-header %t/NestedClangTypes.h -pch-output-dir %t/PCHCache %s -module-name Lib -DNO_MODULE
+// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk) -typecheck -I %t -pch-output-dir %t/PCHCache -import-objc-header %t/NestedClangTypes.h %s -DCLIENT -verify
+
+#if CLIENT


If this PR is about fast paths, then should you be testing the value of the stats like NumNestedTypeShortcuts? Or is this a correctness test?

It's a correctness test in this case; master will crash with circular dependencies without the "fast path". Maybe I should call it a "direct path".

jrose-apple · 2017-07-14T17:28:19Z

Doug's well away on vacation by this point, so I'm unlikely to get a review from him. I'm not too worried about the serialization parts, though, and I trust you on the importer parts.

jrose-apple · 2017-07-14T17:43:55Z

@swift-ci Please test Linux

swift-ci · 2017-07-14T17:45:27Z

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - 6faac2a3beb2b517480d4784d0d7eeae77876a99
Test requested by - @jrose-apple

milseman

Alright, in that case I have more questions ;-)

milseman · 2017-07-14T17:52:03Z

lib/ClangImporter/ClangImporter.cpp

+    auto codeEnum = mutableBase->lookupDirect(name,/*ignoreNewExtensions*/true);
+    if (codeEnum.size() == 1 && isa<TypeDecl>(codeEnum.front()))
+      return cast<TypeDecl>(codeEnum.front());
+    if (codeEnum.size() > 1)


I'm not familiar with the role of Code in the importer. What does it mean for there to be multiple lookup results?

Ah, so this is about how we import error enums, which come out something like this:

struct MyError : _BridgedStoredNSError { static let _domain: String var rawValue: NSError @objc enum Code: Int { case bad case worse } static var bad: MyError { return NSError(domain: _domain, code: Code.bad.rawValue) } static var worse: MyError { return NSError(domain: _domain, code: Code.worse.rawValue) } }

However, it's possible we've just imported a struct that looks like an error enum struct (given the imperfect checks above), and that it's been extended with members named Code, possibly more than one. In that case we can't be sure this lookup is doing the right thing, and so give up on the "fast path".

milseman · 2017-07-14T17:52:08Z

lib/ClangImporter/ClangImporter.cpp

+
+  if (results.size() == 1)
+    return results.front();
+  return nullptr;


Could you add a comment here? What does it mean for there to be multiple results, and why should that trigger an import failure? Would this be a mismatch between what forEachDistinctName considers distinct and what this function is looking for?

It's probably impossible for this part of the code to come back with multiple results unless the developer screwed up, but if two types in the same module were both SwiftName'd into "Outer.Inner", you'd end up in this situation. It's always better to Not Crash.

I guess that's worth a comment.

Ah, if not also a test

milseman

LGTM, with a request for a couple comments for future-reader clarity

These are also imported as local types, so they can have the same issues as the previous commit.

(both precompiled and non-precompiled)

Unrelated to the previous commits, but good to Not Crash.

jrose-apple · 2017-07-17T19:01:44Z

@swift-ci Please test

swift-ci · 2017-07-17T19:02:09Z

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - 6faac2a3beb2b517480d4784d0d7eeae77876a99
Test requested by - @jrose-apple

swift-ci · 2017-07-17T19:21:31Z

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - 6faac2a3beb2b517480d4784d0d7eeae77876a99
Test requested by - @jrose-apple

…for-Clang [ClangImporter] Add direct access for import-as-member types.

DougGregor

Late to the party, but LGTM

jrose-apple requested review from milseman and DougGregor July 14, 2017 00:27

jrose-apple force-pushed the lookupNestedType-for-Clang branch from 6dcd454 to 6faac2a Compare July 14, 2017 01:01

milseman reviewed Jul 14, 2017

View reviewed changes

milseman approved these changes Jul 14, 2017

View reviewed changes

jrose-apple added 3 commits July 17, 2017 11:52

[ClangImporter] Add fast-path lookup for error code enums as well.

bc460a2

These are also imported as local types, so they can have the same issues as the previous commit.

[test] Test previous commits with bridging headers too.

ac567ac

(both precompiled and non-precompiled)

[test] Add a test for two types import-as-member'd with the same name.

3d42828

Unrelated to the previous commits, but good to Not Crash.

jrose-apple force-pushed the lookupNestedType-for-Clang branch from 6faac2a to 3d42828 Compare July 17, 2017 18:53

jrose-apple merged commit 2dbae8e into swiftlang:master Jul 17, 2017

jrose-apple deleted the lookupNestedType-for-Clang branch July 17, 2017 21:02

jrose-apple added a commit to jrose-apple/swift that referenced this pull request Jul 17, 2017

Merge pull request swiftlang#10956 from jrose-apple/lookupNestedType-…

121ebb0

…for-Clang [ClangImporter] Add direct access for import-as-member types.

jrose-apple mentioned this pull request Jul 17, 2017

[Serialization] Use the correct module for the nested type fast path. #11018

Merged

DougGregor reviewed Jul 24, 2017

View reviewed changes

belkadan mentioned this pull request Jul 14, 2017

[SR-5284] Reproducible segfault at swift::ModuleFile::getTypeChecked #47859

Closed

[ClangImporter] Add direct access for import-as-member types. #10956

[ClangImporter] Add direct access for import-as-member types. #10956

Uh oh!

Conversation

jrose-apple commented Jul 14, 2017

Uh oh!

jrose-apple commented Jul 14, 2017

Uh oh!

jrose-apple commented Jul 14, 2017

Uh oh!

swift-ci commented Jul 14, 2017

Uh oh!

swift-ci commented Jul 14, 2017

Uh oh!

jrose-apple commented Jul 14, 2017

Uh oh!

jrose-apple commented Jul 14, 2017

Uh oh!

swift-ci commented Jul 14, 2017

Uh oh!

swift-ci commented Jul 14, 2017

Uh oh!

jrose-apple commented Jul 14, 2017

Uh oh!

jrose-apple commented Jul 14, 2017

Uh oh!

swift-ci commented Jul 14, 2017

Uh oh!

jrose-apple commented Jul 14, 2017

Uh oh!

swift-ci commented Jul 14, 2017

Uh oh!

jrose-apple commented Jul 14, 2017

Uh oh!

milseman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrose-apple commented Jul 14, 2017

Uh oh!

jrose-apple commented Jul 14, 2017

Uh oh!

swift-ci commented Jul 14, 2017

Uh oh!

milseman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

milseman left a comment

Choose a reason for hiding this comment

Uh oh!

jrose-apple commented Jul 17, 2017

Uh oh!

swift-ci commented Jul 17, 2017

Uh oh!

swift-ci commented Jul 17, 2017

Uh oh!

DougGregor left a comment

Choose a reason for hiding this comment

Uh oh!