SE-0075 Implement canImport #5778

CodaFi · 2016-11-14T21:12:01Z

SE-0075 requests that we offer a build-time check for the importability of modules. This check, however, is particularly expensive so we can no longer resolve every condition statically. Conditions that are not essential to the health of the parser are left unevaluated and deferred to a new phase post-parse called "Condition Resolution". Condition Resolution will also skip functions that have been deferred to keep Code Completion speedy; resolution can be done on-demand.

Resolves SR-1560.

CodaFi · 2016-11-14T21:14:09Z

@jrose-apple for Parse, @DougGregor for Sema.

Come On Down

📯 📯 📯

CodaFi · 2016-11-14T21:17:59Z

lib/Sema/NameBinding.cpp

+              // Evaluate conditions until we find the active clause.
+              if (!clause.Cond || evaluateCondition(clause.Cond)) {
+                for (auto &e : clause.Elements) {
+                  if (auto innerStmt = e.dyn_cast<Stmt *>()) {


I will try to un-pyramid-of-doom this mess.

jrose-apple

Preliminary comments on your WIP.

jrose-apple · 2016-11-15T16:34:57Z

include/swift/AST/Decl.h

@@ -1895,32 +1890,24 @@ class IfConfigDecl : public Decl {
  /// An array of clauses controlling each of the #if/#elseif/#else conditions.
  /// The array is ASTContext allocated.
  ArrayRef<IfConfigDeclClause> Clauses;
+  SourceLoc StartLoc;


Why bother storing this separately?

At one point I had caused destructive updates to the clause array, but that was ridiculous and I reverted it. Going to revert this part too.

jrose-apple · 2016-11-15T16:37:08Z

lib/Parse/ParseDecl.cpp

@@ -2177,7 +2173,7 @@ ParserStatus Parser::parseDecl(ParseDeclOptions Flags,
  }

  if (auto SF = CurDeclContext->getParentSourceFile()) {
-    if (!getScopeInfo().isInactiveConfigBlock()) {
+    if (!(Flags & PD_InIfConfig)) {


Can you add a comment mentioning that other attributes will be handled later?

jrose-apple · 2016-11-15T16:42:21Z

lib/Parse/ParseDecl.cpp

@@ -5092,6 +5089,9 @@ ParserResult<ClassDecl> Parser::parseDeclClass(SourceLoc ClassLoc,
    Scope S(this, ScopeKind::ClassBody);
    ParseDeclOptions Options(PD_HasContainerType | PD_AllowDestructor |
                             PD_InClass);
+    if (Flags & PD_InIfConfig) {
+      Options |= PD_InIfConfig;


Hm. The old flag was used for immediate children of the #if, no? Using it this way is definitely changing the meaning. Is everywhere else that checks it equipped to handle that?

(The answer might be "yes"; just checking.)

There was no flag for this before. It used to be a function of "scope", but I felt it was inappropriate for scope to ask the question "is this a condition" let alone "is this an inactive condition" like it did before.

jrose-apple · 2016-11-15T16:43:06Z

lib/Parse/ParseStmt.cpp

-                      ConditionalBlockKind ==
-                        BraceItemListKind::InactiveConditionalBlock);
-  }
+  initScope.emplace(this,


This whole trick was just because we were conditionally constructing the scope. It's okay to just do it normally now instead.

jrose-apple · 2016-11-15T16:44:26Z

lib/Parse/ParseStmt.cpp

-      // Add the #if block itself as a TLCD if necessary
-      if (Kind == BraceItemListKind::TopLevelCode) {
+      // Add the #if block itself as a TLCD if necessary.  They will be
+      // flattened away in namebinding.


Nitpick: "name binding".

(which, as a pass, needs a new name anyway; maybe "import resolution"? but that's for later)

jrose-apple · 2016-11-15T17:24:30Z

lib/Sema/NameBinding.cpp

+        }
+      } else if (AbstractFunctionDecl *FD = dyn_cast<AbstractFunctionDecl>(D)) {
+        // For functions, transform the statements in the body.
+        // FIXME: This can be deferred until type checking.


Yeah, I think we do need to figure out how to do this. Code completion doesn't parse function bodies unless it needs to, but this will probably fault them all in eagerly.

@DougGregor Any thoughts on this? I was thinking of either exposing this class in a separate file (ConditionResolution.cpp?) or adding a kind of decl that closes over the resolution procedure for AbstractFunctionDecls so TC can just un-thunk-ify on the spot.

jrose-apple · 2016-11-15T17:25:53Z

lib/Sema/NameBinding.cpp

+    SmallVector<Decl *, 8> scratch;
+    auto processedD = resolveConditionalClauses(d, Binder, scratch);
+    assert(scratch.empty());
+    SF.Decls.push_back(processedD);


TopLevelCodeDecls definitely need to stay in source order.

jrose-apple · 2016-11-15T17:26:09Z

test/ClangImporter/enum-error.swift

@@ -91,6 +91,7 @@ func testError() {
  } catch is TestError {
  } catch {}

+  print(testErrorNSError)


Unrelated change?

Related to the test/decl/var/usage.swift regression.

jrose-apple · 2016-11-15T17:26:42Z

test/IDE/print_ast_tc_decls.swift

-// PASS_PRINT_AST: #elseif
-// PASS_PRINT_AST: #else
-// PASS_PRINT_AST: #endif
+// PASS_PRINT_AST-NOT: #if


I don't think this is correct either. It's important to have the document structure in place for SourceKit and other tools.

Hmph, re-inserting IfConfigStmts should do the trick, then.

jrose-apple · 2016-11-15T17:27:33Z

test/decl/var/usage.swift

-  let abc = 42    // no warning.
-  var mut = 18    // no warning.
+  let abc = 42    // expected-warning{{initialization of immutable value 'abc' was never used; consider replacing with assignment to '_' or removing it}}.
+  var mut = 18    // expected-warning{{initialization of variable 'mut' was never used; consider replacing with assignment to '_' or removing it}}.


Just to make it clear, this is a regression.

The usage clause is inactive, why shouldn't we warn here exactly?

Because it might be active under another build configuration, and there's no way to silence it otherwise.

jrose-apple · 2016-12-14T17:24:17Z

Hm. I didn't think about tying it to delaying a whole function body, but that might be good enough…although we still need to actually do that during normal compilation. (IIRC we currently only do it for code completion.)

I don't like the idea of Parse being able to import modules, but the benefit of having parseDelayedDecl Just Work might outweigh that.

Please do add some test cases that show #if canImport(…) in function bodies, in top-level contexts under -parse-as-library, and in closure bodies used to initialize a variable.

CodaFi · 2016-12-14T19:07:44Z

@jrose-apple How do the tests look to you now?

jrose-apple

Thanks, but you're still missing

#if at the top level of a -parse-as-library file.
#if in the initializer of a property.
#if in a non-primary file (which I forgot to ask for but which is worth testing)

jrose-apple · 2016-12-14T19:16:32Z

test/Parse/ConditionalCompilation/can_import.swift

+  }
+
+  /// Create an instance initialized to `value`.
+  @_transparent


Nitpick: please don't use @_transparent in tests where it's not relevant.

Implemented in the naive way by way of the current ASTContext’s ‘LoadedModules’. In theory this list should be in one-to-one correspondence with the definition of “importable” as any module that is required for compilation is loaded *before* we start parsing the file. This means we don’t have to load the module, just check that it’s in the set of loaded modules. Note that this approach will most likely fall apart if submodules are introduced and are lazily loaded. At that point this will need to be refactored into a format that defers the checking of conditions sometime before or during typechecking (after namebinding).

This completely removes Parse’s ability to make any judgement calls about compilation conditions, instead the parser-relevant parts of ‘evaluateConditionalCompilationExpr’ have been moved into ‘classifyConditionalCompilationExpr’ where they exist to make sure only decls that we want to parse actually parse later. The condition-evaluation parts have been moved into NameBinding in the form of a Walker that evaluates and collapses IfConfigs. This walker is meant as an homage to PlaygroundLogger. It should probably be factored out into a common walker at some point in the future.

CodaFi · 2016-12-14T20:00:16Z

OK, patched.

jrose-apple · 2016-12-14T20:31:17Z

Thanks! This is the property one I'm thinking of, sorry.

let value: Int = {
#if canImport(Swift)
  error() // expected-error {{blah blah blah}}
#else
  error() // should not happen
#endif
}()

An unfortunately necessary thing to delay defrosting function bodies as long as we can.

CodaFi · 2016-12-14T20:39:48Z

test/Parse/ConditionalCompilation/can_import.swift

+  var _description: String
+#endif
+
+  public let magicConstant: Int = {


@jrose-apple Is this what you had in mind?

Yep, thanks!

jrose-apple · 2016-12-14T21:08:14Z

And since Doug already approved this I think that means it's good to go. :-)

@swift-ci Please test

CodaFi · 2016-12-14T21:08:35Z

@swift-ci please smoke test.

CodaFi · 2016-12-14T21:52:26Z

Full test happening here for OS X

CodaFi · 2016-12-14T21:57:33Z

Thanks for sticking through. I'm going to prepare the optimization patch for Doug and update the CHANGELOG in future PRs.

⛵️

…oods" This reverts commit 8e4e857, reversing changes made to eaa7cb9.

This reverts the contents of swiftlang#5778 and replaces it with a far simpler implementation of condition resolution along with canImport. When combined with the optimizations in swiftlang#6279 we get the best of both worlds with a performance win and a simpler implementation.

* <swiftlang/swift#5778> * <swiftlang/swift#6279> * <swiftlang/swift#6547> * <swiftlang/swift#11613>

CodaFi changed the title ~~[WIP][SR-0075][1/2] Transfer Build Config Clause Resolution To NameBinding~~ [WIP][SE-0075][1/2] Transfer Build Config Clause Resolution To NameBinding Nov 14, 2016

CodaFi assigned jrose-apple and DougGregor Nov 14, 2016

CodaFi commented Nov 14, 2016

View reviewed changes

CodaFi force-pushed the fine-imported-goods branch 2 times, most recently from 37b2b17 to 4423b19 Compare November 15, 2016 02:46

jrose-apple reviewed Nov 15, 2016

View reviewed changes

CodaFi force-pushed the fine-imported-goods branch 9 times, most recently from 0ce3c44 to 4a12d3d Compare November 22, 2016 03:54

CodaFi changed the title ~~[WIP][SE-0075][1/2] Transfer Build Config Clause Resolution To NameBinding~~ SE-0075 Implement canImport Nov 22, 2016

CodaFi force-pushed the fine-imported-goods branch 12 times, most recently from ac91a4c to 82405d6 Compare November 25, 2016 02:12

rintaro mentioned this pull request Dec 14, 2016

Port _stdlib_pipe to Windows #6240

Closed

CodaFi force-pushed the fine-imported-goods branch from 524f584 to a7706d3 Compare December 14, 2016 19:06

jrose-apple reviewed Dec 14, 2016

View reviewed changes

CodaFi added 3 commits December 14, 2016 14:59

Extract common code into StmtTransformer

72beb9d

CodaFi force-pushed the fine-imported-goods branch from a7706d3 to 848edbe Compare December 14, 2016 19:59

CodaFi force-pushed the fine-imported-goods branch from 848edbe to 0320e1e Compare December 14, 2016 20:37

Add condition resolution as a new phrase post-parse

cededef

An unfortunately necessary thing to delay defrosting function bodies as long as we can.

CodaFi force-pushed the fine-imported-goods branch from 0320e1e to cededef Compare December 14, 2016 20:39

CodaFi commented Dec 14, 2016

View reviewed changes

CodaFi merged commit 8e4e857 into swiftlang:master Dec 14, 2016

CodaFi deleted the fine-imported-goods branch December 14, 2016 21:57

CodaFi mentioned this pull request Dec 14, 2016

[Swift 4.1?] Add Arbitrary instances for EVERYTHING typelift/SwiftCheck#166

Open

4 tasks

jrose-apple added a commit to jrose-apple/swift that referenced this pull request Dec 20, 2016

Revert "Merge pull request swiftlang#5778 from CodaFi/fine-imported-g…

080cf3f

…oods" This reverts commit 8e4e857, reversing changes made to eaa7cb9.

CodaFi mentioned this pull request Jan 4, 2017

Simplify Condition Resolution #6547

Merged

benrimmington added a commit to swiftlang/swift-evolution that referenced this pull request Sep 3, 2017

[SE-0075] Implemented (Swift 4.1)

087d3fa

* <swiftlang/swift#5778> * <swiftlang/swift#6279> * <swiftlang/swift#6547> * <swiftlang/swift#11613>

SE-0075 Implement canImport #5778

SE-0075 Implement canImport #5778

Uh oh!

Conversation

CodaFi commented Nov 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CodaFi commented Nov 14, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrose-apple 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!

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!

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 Dec 14, 2016

Uh oh!

CodaFi commented Dec 14, 2016

Uh oh!

jrose-apple left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CodaFi commented Dec 14, 2016

Uh oh!

jrose-apple commented Dec 14, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrose-apple commented Dec 14, 2016

Uh oh!

CodaFi commented Dec 14, 2016

Uh oh!

CodaFi commented Dec 14, 2016

Uh oh!

CodaFi commented Dec 14, 2016

Uh oh!

Uh oh!

CodaFi commented Nov 14, 2016 •

edited

Loading