Implement SE-0075: CanImport #11613

CodaFi · 2017-08-24T23:20:20Z

This implementation required a compromise between parser
performance and AST structuring. On the one hand, Parse
must be fast in order to keep things in the IDE zippy, on
the other we must hit the disk to properly resolve 'canImport'
conditions and inject members of the active clause into the AST.
Additionally, a Parse-only pass may not provide platform-specific
information to the compiler invocation and so may mistakenly
activate or de-activate branches in the if-configuration decl.

The compromise is to perform condition evaluation only when
continuing on to semantic analysis. This keeps the parser quick
and avoids the odd unpacking that parse does for active conditions
while still retaining the ability to see through to an active
condition when we know we're moving on to a more expensive phase like
semantic analysis anyways.

Resolves SR-1560 and any associated shadowing oddities from the first go around.

CodaFi · 2017-08-24T23:20:30Z

@swift-ci please test

CodaFi · 2017-08-24T23:29:43Z

Longer-term answers to this would involve pushing condition resolution further down into the stack. But we currently don't have a strong phase separation between name binding and parse.

swift-ci · 2017-08-25T00:33:52Z

Build failed
Swift Test OS X Platform
Git Sha - 3e41c5602ff33c33809783c6e7b2a8df8b78ffe6

rintaro · 2017-08-25T03:00:28Z

include/swift/Basic/LangOptions.h

@@ -43,6 +43,8 @@ namespace swift {
    Endianness,
    /// Runtime support (_ObjC or _Native)
    Runtime,
+    /// Conditional import
+    CanImport,
  };
  enum { NumPlatformConditionKind = 4 };


Could you add comments about why NumPlatformConditionKind does not count CanImport?
Or increment this, and update PlatformConditionValues declaration with NumPlatformConditionKind - 1 (with comment)

Is having NumPlatformConditionKind defined in the first place really necessary?

Not necessary, but I added this in 1f3c6622#diff-f544d77b7be49d30361f0f41d9f38244L275 because people forget to update this value.

My thinking is that even if people update this value, it's better to provide recommended bounds in powers of two.

rintaro · 2017-08-25T03:00:55Z

lib/Basic/LangOptions.cpp

@@ -98,6 +98,10 @@ checkPlatformConditionSupported(PlatformConditionKind Kind, StringRef Value,
  case PlatformConditionKind::Runtime:
    return contains(SupportedConditionalCompilationRuntimes, Value,
                    suggestions);
+    case PlatformConditionKind::CanImport:


nit: indent

harlanhaskins · 2017-08-25T17:10:19Z

include/swift/Basic/LangOptions.h

@@ -332,8 +333,7 @@ namespace swift {
    }

  private:
-    llvm::SmallVector<std::pair<PlatformConditionKind, std::string>,
-                      NumPlatformConditionKind>
+    llvm::SmallVector<std::pair<PlatformConditionKind, std::string>, 4>


It's a bit unfortunate to lose the named constant here...

Maybe, but I'm not really sure what it buys us outside of this one place.

CodaFi · 2017-08-25T18:16:38Z

@harlanhaskins Does not doing condition resolution in parse-only passes affect libSyntax at all?

CodaFi · 2017-08-26T17:05:14Z

@swift-ci test

benlangmuir

We're going to have to do something slightly different for syntactic rename. Syntactic rename is "parse-only", but we do want to resolve #if conditions accurately, and are passing in command-line arguments so that we can do that. I'm fine with hitting the disk for any canImports during rename.

CodaFi · 2017-08-28T17:28:30Z

@benlangmuir So then, what's the best way to detect that in a parse-only pass?

benlangmuir · 2017-08-28T17:32:08Z

@CodaFi Could we rename yourParseOnly bit to something like ShouldEvaluateIfConfigConditions and set it explicitly in the rename code?

CodaFi · 2017-08-28T17:55:40Z

I will add a flag to performParseOnly as a stopgap, but we have to plan for a future where evaluation occurs in a later phase.

This implementation required a compromise between parser performance and AST structuring. On the one hand, Parse must be fast in order to keep things in the IDE zippy, on the other we must hit the disk to properly resolve 'canImport' conditions and inject members of the active clause into the AST. Additionally, a Parse-only pass may not provide platform-specific information to the compiler invocation and so may mistakenly activate or de-activate branches in the if-configuration decl. The compromise is to perform condition evaluation only when continuing on to semantic analysis. This keeps the parser quick and avoids the unpacking that parse does for active conditions while still retaining the ability to see through to an active condition when we know we're moving on to semantic analysis anyways.

CodaFi · 2017-08-28T22:35:40Z

@swift-ci please smoke test

CodaFi · 2017-08-28T22:40:47Z

tools/SourceKit/lib/SwiftLang/SwiftDocSupport.cpp

@@ -1366,7 +1366,7 @@ SourceFile *SwiftLangSupport::getSyntacticSourceFile(
    Error = "Compiler invocation set up failed";
    return nullptr;
  }
-  ParseCI.performParseOnly();
+  ParseCI.performParseOnly(/*EvaluateConditionals*/true);


@benlangmuir Is this correct? Should this be a parameter to this function? Its callers are the 'syntactic rename' and the 'find rename ranges' actions.

Seems like a fine stopgap to me.

benlangmuir · 2017-08-29T16:49:00Z

I will add a flag to performParseOnly as a stopgap, but we have to plan for a future where evaluation occurs in a later phase.

@CodaFi just to clarify: when you say "in a later phase", will this be something we can run independently of sema/type-checking? That's going to be a requirement for syntactic rename.

benlangmuir · 2017-08-29T16:58:19Z

lib/Parse/ParseIfConfig.cpp

@@ -331,6 +332,8 @@ class ValidateIfConfigCondition :
        DiagName = "architecture"; break;
      case PlatformConditionKind::Endianness:
        DiagName = "endianness"; break;
+      case PlatformConditionKind::CanImport:
+        DiagName = "import conditional"; break;


Is this testable?

As of now, no. There's no validation of the module name being performed, so there's no way this will get hit (see the FIXME).

Okay, just checking.

benlangmuir

LGTM

CodaFi · 2017-08-29T19:40:44Z

@benlangmuir I originally wanted to do this in name binding but now I think making the configurations a kind of DeclContext and teaching lookup about how to properly find these is the right way to model the problem.

If Syntactic Rename needs this, and the cost of hitting the disk isn't a problem, would it be possible to use the semantic tree instead? Evaluating conditions in the AST is a bona-fide semantic operation that doesn't belong in Parse.

CodaFi · 2017-08-29T19:44:52Z

An option might be to have this notion of the "name bound tree" for the point at which the AST is well-formed syntactically and has gone through condition resolution and name binding but has not yet been typechecked.

benlangmuir · 2017-08-29T19:48:47Z

@CodaFi why do we need to conflate if-config condition evaluation with name binding? We don't need name-binding to evaluate the conditions, right? It seems like what we want for syntactic rename is to walk the syntax tree but evaluate and track the if-config context as we go.

CodaFi · 2017-08-29T19:52:45Z

Because condition resolution can introduce arbitrary names into scopes. That requires re-binding declrefs that are currently being eagerly bound by Parse. Currently, that's how Syntactic Rename is even finding these names: Parse is hoisting the active set of declarations and statements into the right scope and then binding them later on which is strange because it means our parser has its own understanding of scope.

My idea, post ASTScope, is to scrap all of that and teach lookup to walk through these conditions.

CodaFi · 2017-08-29T20:01:59Z

Or if we start from a distinguished syntactic tree, whatever the reverse of the LegacyASTTransformer is could resolve these conditions

benlangmuir · 2017-08-29T20:12:43Z

@CodaFi syntactic rename shouldn't need name binding in theory. It gets its list of locations from the indexer not by looking at the AST. We only parse so that we can figure out argument labels, determine active-vs-inactive, etc. If parsing no longer puts active code into scopes or no longer binds names itself then I think we should adapt to that rather than require name binding. CC @nathawes in case I'm missing something.

CodaFi · 2017-08-31T01:16:58Z

@swift-ci please test and merge

CodaFi · 2017-08-31T03:18:02Z

Failures are definitely unrelated.

@swift-ci please smoke test and merge

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

jrose-apple · 2017-09-07T21:31:06Z

Mentioning this as rdar://problem/27382987 to hopefully help Apple's automated systems to track the PR.

CodaFi requested review from rintaro, benlangmuir and DougGregor August 24, 2017 23:20

rintaro reviewed Aug 25, 2017

View reviewed changes

CodaFi force-pushed the refined-imported-goods branch 3 times, most recently from 8b7343b to f39eb16 Compare August 25, 2017 07:17

harlanhaskins reviewed Aug 25, 2017

View reviewed changes

CodaFi force-pushed the refined-imported-goods branch from f39eb16 to 50de421 Compare August 25, 2017 21:01

CodaFi changed the title ~~[WIP] Implement SE-0075: CanImport~~ Implement SE-0075: CanImport Aug 25, 2017

benlangmuir requested changes Aug 28, 2017

View reviewed changes

jessesquires mentioned this pull request Aug 28, 2017

[85] Issue #85 - August 31, 2017 SwiftWeekly/swiftweekly.github.io#303

Closed

CodaFi force-pushed the refined-imported-goods branch from 50de421 to a88af81 Compare August 28, 2017 22:34

CodaFi force-pushed the refined-imported-goods branch from a88af81 to 75a83da Compare August 28, 2017 22:35

CodaFi commented Aug 28, 2017

View reviewed changes

benlangmuir reviewed Aug 29, 2017

View reviewed changes

benlangmuir approved these changes Aug 29, 2017

View reviewed changes

swift-ci merged commit fa76c56 into swiftlang:master Aug 31, 2017

CodaFi deleted the refined-imported-goods branch August 31, 2017 04:29

SD10 mentioned this pull request Sep 1, 2017

[86] Issue #86 - Sept 7, 2017 SwiftWeekly/swiftweekly.github.io#305

Closed

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>

CodaFi mentioned this pull request Aug 25, 2017

[SR-1560] Implement support for SE-0075: Adding a Build Configuration Import Test #44169

Closed

Implement SE-0075: CanImport #11613

Implement SE-0075: CanImport #11613

Uh oh!

Conversation

CodaFi commented Aug 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CodaFi commented Aug 24, 2017

Uh oh!

CodaFi commented Aug 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

swift-ci commented Aug 25, 2017

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!

CodaFi Aug 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CodaFi commented Aug 25, 2017

Uh oh!

CodaFi commented Aug 26, 2017

Uh oh!

benlangmuir left a comment

Choose a reason for hiding this comment

Uh oh!

CodaFi commented Aug 28, 2017

Uh oh!

benlangmuir commented Aug 28, 2017

Uh oh!

CodaFi commented Aug 28, 2017

Uh oh!

CodaFi commented Aug 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benlangmuir commented Aug 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CodaFi Aug 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benlangmuir left a comment

Choose a reason for hiding this comment

Uh oh!

CodaFi commented Aug 29, 2017

Uh oh!

CodaFi commented Aug 29, 2017

Uh oh!

benlangmuir commented Aug 29, 2017

Uh oh!

CodaFi commented Aug 29, 2017

Uh oh!

CodaFi commented Aug 29, 2017

Uh oh!

benlangmuir commented Aug 29, 2017

Uh oh!

CodaFi commented Aug 31, 2017

Uh oh!

CodaFi commented Aug 31, 2017

Uh oh!

jrose-apple commented Sep 7, 2017

CodaFi commented Aug 24, 2017 •

edited

Loading

CodaFi commented Aug 24, 2017 •

edited

Loading

CodaFi Aug 25, 2017 •

edited

Loading

benlangmuir commented Aug 29, 2017 •

edited

Loading

CodaFi Aug 29, 2017 •

edited

Loading