Introduce Import Resolution as a Frontend Action #17645

CodaFi · 2018-06-29T23:44:19Z

Add ~~Name Binding~~ Import Resolution as a frontend action to make computing dependencies cheaper by no longer going through the type checker. Also includes corrections and additions to the diagnostics for misuse of -emit-reference-dependencies and -emit-dependencies when in parse-only contexts.

slavapestov · 2018-06-30T00:16:43Z

lib/Frontend/Frontend.cpp

@@ -884,6 +884,79 @@ void CompilerInstance::performParseOnly(bool EvaluateConditionals) {
         "Loaded a module during parse-only");
 }

+void CompilerInstance::performParseAndNameBindingOnly() {


Can this share code with any of the other actions?

I can probably fold it into performSema now, but we may not necessarily be calling performNameBinding in the future for this path.

Yeah, something about this feels a bit wrong to me too: the extent to which performParseOnly and the parsing-part of parseAndCheckTypes have already diverged seems regrettable, and this is just going to keep digging that hole by further duplicating and diverging. Can we refactor these paths without making them awful?

I'm emphatically with Graydon and Slava here. It already really bothers me that there were two main routines, performParseOnly and performSema. You would be doing the code base a great service if you could figure out how to factor these together so it were crystal clear which operations went with which alternative and which ones were common.

I think performParseOnly and performSema actually are significantly different, but performSema and this new operation are not. Please do make this a mode for performSema instead.

jrose-apple

My primary comment is that if you're going to do this, that means it's time to fix the name of the pass. :-) It should be "perform import resolution" or even just "resolve imports". Arguably the parts that handle operators don't even belong here; they just save a top-level AST walk. We could just as easily move that into early type checking, like we do with extension binding.

jrose-apple · 2018-07-02T16:21:58Z

lib/Frontend/Frontend.cpp

@@ -884,6 +884,79 @@ void CompilerInstance::performParseOnly(bool EvaluateConditionals) {
         "Loaded a module during parse-only");
 }

+void CompilerInstance::performParseAndNameBindingOnly() {


I think performParseOnly and performSema actually are significantly different, but performSema and this new operation are not. Please do make this a mode for performSema instead.

Parse-only invocations do not support the proper creation of dependency files or reference dependency files because they have not yet run name binding. Ban these invocations by diagnostic and add a new diagnostic specifically for reference dependencies.

Introduces the -name-bind frontend action that is intended as an intermediary between the parse-only actions and a full typechecking pass. In this phase, module imports will be validated and resolved, making it possible to emit full make-style dependencies files among other things. Note that all information available to a parse-only pass is available to name binding, but because it does not continue-on to typecheck input files, full semantic information is not.

jrose-apple

Looks pretty good to me, only minor comments.

jrose-apple · 2018-07-13T18:04:24Z

lib/Frontend/Frontend.cpp

@@ -425,7 +425,21 @@ shouldImplicityImportSwiftOnoneSupportModule(CompilerInvocation &Invocation) {
  return Invocation.getFrontendOptions().isCreatingSIL();
 }

+void CompilerInstance::performParseAndResolveImportsOnly() {
+  performSemaUpTo(SourceFile::NameBound);


I'm personally not sure we need these helper functions, and think we could just add an argument to performSema (and deprecate performParseOnly later). But either way is okay with me.

jrose-apple · 2018-07-13T18:06:04Z

lib/Frontend/FrontendOptions.cpp

+  case ActionType::EmitBC:
+  case ActionType::EmitAssembly:
+  case ActionType::EmitObject:
+  case ActionType::EmitImportedModules:


I wonder if -emit-imported-modules should be demoted from an action later…

jrose-apple · 2018-07-13T18:06:38Z

lib/Frontend/FrontendOptions.cpp

@@ -227,6 +263,7 @@ bool FrontendOptions::canActionEmitObjCHeader(ActionType action) {
  case ActionType::REPL:
    return false;
  case ActionType::Parse:
+  case ActionType::ResolveImports:


Actually, I think Parse and ResolveImports belong in the "false" section for this one. It definitely needs a type-checked AST.

I'll submit another patch for that one.

jrose-apple · 2018-07-13T18:08:59Z

lib/FrontendTool/FrontendTool.cpp

@@ -929,8 +939,6 @@ static bool performCompile(CompilerInstance &Instance,
      debugFailWithCrash();
  }


Arguably these crashes should happen even in -parse or -resolve-imports.

For future reference: Had an offline chat about this. Filed SR-8252 about it.

davidungar · 2018-07-13T19:40:24Z

include/swift/AST/DiagnosticsFrontend.def

 ERROR(error_mode_cannot_emit_module_doc,none,
-  "this mode does not support emitting module documentation files", ())
+      "this mode does not support emitting module documentation files", ())


If you feel like it, it would be nice to actually include the referred-to mode in the error message. I understand if it's outside the scope of this change.

Most of these errors are already caught at the driver level, so checking for them in the frontend just helps test authors. That means there's not really a problem with talking about specific frontend arguments, which I'd normally be worried about.

davidungar · 2018-07-13T19:48:34Z

Someday, when I'm back into this code, I'll be wondering what is this new mode (up to name-binding) for? Is there a comment somewhere that explains? Could there be?

davidungar

The only significant thought I have is the comment about what the purpose of the new functionality is for. In other words, what would I be breaking if I messed this up in the future? When is the compiler invoked in this mode?? Otherwise, LGTM.

CodaFi · 2018-07-13T20:07:50Z

Is there a comment somewhere that explains?

There is not currently an up-to-date definition of what name binding is actually supposed to be doing (that is, you can find resources like the old and new LangRefs in the docs folder that describe what it certainly used to do).

Could there be?

Certainly. All these actions should be documented as much. Which interface would you like me to attach these comments to?

jrose-apple · 2018-07-13T20:35:06Z

Either @CodaFi or I will probably go back afterwards and replace the other instances of "NameBinding" with "ImportResolution" elsewhere in the compiler. That'd be a reasonable time to pump up documentation, say in SourceFile::ASTStage_t.

CodaFi · 2018-07-13T20:37:55Z

Alright, thanks for the feedback.

⛵️

@swift-ci please smoke test and merge

davidungar · 2018-07-13T22:03:31Z

Sounds good! - David

…

On Jul 13, 2018, at 1:35 PM, Jordan Rose ***@***.***> wrote: Either @CodaFi or I will probably go back afterwards and replace the other instances of "NameBinding" with "ImportResolution" elsewhere in the compiler. That'd be a reasonable place to beef up documentation, say in SourceFile::ASTStage_t. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub, or mute the thread.

CodaFi requested review from graydon, davidungar and jrose-apple June 29, 2018 23:44

slavapestov reviewed Jun 30, 2018

View reviewed changes

jrose-apple reviewed Jul 2, 2018

View reviewed changes

CodaFi added 2 commits July 13, 2018 10:56

CodaFi force-pushed the name-binding-arbitration branch from 032b053 to 575d6eb Compare July 13, 2018 17:56

CodaFi changed the title ~~Introduce Name Binding as a Frontend Action~~ Introduce Import Resolution as a Frontend Action Jul 13, 2018

Rename "Name Binding" action to "Resolve Imports"

2282065

CodaFi force-pushed the name-binding-arbitration branch from 575d6eb to 2282065 Compare July 13, 2018 18:01

jrose-apple reviewed Jul 13, 2018

View reviewed changes

CodaFi mentioned this pull request Jul 13, 2018

[Driver] Diagnose -emit-objc-header With Pre-Typechecking Actions #17937

Merged

davidungar reviewed Jul 13, 2018

View reviewed changes

davidungar approved these changes Jul 13, 2018

View reviewed changes

swift-ci merged commit 751ffa4 into swiftlang:master Jul 13, 2018

CodaFi deleted the name-binding-arbitration branch July 13, 2018 21:56

		@@ -929,8 +939,6 @@ static bool performCompile(CompilerInstance &Instance,
		debugFailWithCrash();
		}

Introduce Import Resolution as a Frontend Action #17645

Introduce Import Resolution as a Frontend Action #17645

Uh oh!

Conversation

CodaFi commented Jun 29, 2018 • 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!

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 left a comment

Choose a reason for hiding this comment

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!

davidungar commented Jul 13, 2018

Uh oh!

davidungar left a comment

Choose a reason for hiding this comment

Uh oh!

CodaFi commented Jul 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jrose-apple commented Jul 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CodaFi commented Jul 13, 2018

Uh oh!

davidungar commented Jul 13, 2018 via email

Uh oh!

Uh oh!

CodaFi commented Jun 29, 2018 •

edited

Loading

CodaFi commented Jul 13, 2018 •

edited

Loading

jrose-apple commented Jul 13, 2018 •

edited

Loading