Hook up -emit-interface-path to a simple AST printer #18224

jrose-apple · 2018-07-25T20:31:01Z

Next step after #18090 for module stability / textual interfaces. Still mostly just a dummy at this point, but we can start writing tests.

jrose-apple · 2018-07-25T20:31:09Z

@swift-ci Please test

jrose-apple · 2018-07-25T20:31:22Z

@CodaFi, the second commit is the one relevant to you, since you were looking at filesystem stuff.

swift-ci · 2018-07-25T20:45:26Z

Build failed
Swift Test OS X Platform
Git Sha - caafaeee0ecf2d4d9e950e7746c035d6ff4670f5

jrose-apple · 2018-07-25T20:46:47Z

@swift-ci Please test macOS

davidungar

Since I'm looking at this without experience, I found somethings that will only confuse newbies, I think. Not sure which thoughts you'll want to take.

davidungar · 2018-07-25T20:54:14Z

include/swift/AST/PrintOptions.h

@@ -424,6 +424,8 @@ struct PrintOptions {
    return result;
  }

+  static PrintOptions printTextualInterfaceFile();
+


Why the blank line here but not after 429?

My understanding is that those two are used by SourceKit in related ways. I could at least add a doc comment to mine, though, to encourage it in the future for the others.

davidungar · 2018-07-25T20:55:19Z

include/swift/Basic/FileSystem.h

-  std::error_code moveFileIfDifferent(const llvm::Twine &source,
-                                      const llvm::Twine &destination);
+
+/// Invokes \p action with a raw_ostream that refers to a temporary file,


Nice comment!

davidungar · 2018-07-25T20:57:58Z

lib/AST/ASTPrinter.cpp

@@ -64,6 +64,21 @@ void PrintOptions::clearSynthesizedExtension() {
  TransformContext.reset();
 }

+PrintOptions PrintOptions::printTextualInterfaceFile() {


The name of this function sounds like it does the printing, but it only computes options. I see that it follows the practice of the other two, but still...

davidungar · 2018-07-25T20:59:00Z

lib/Basic/FileSystem.cpp

@@ -29,6 +35,82 @@ namespace {
  };
 } // end anonymous namespace

+std::error_code swift::atomicallyWritingToFile(
+    StringRef outputPath, bool binaryMode,


Could binaryMode be a const, or is it changed inside of the function?

davidungar · 2018-07-25T21:00:47Z

lib/Basic/FileSystem.cpp

@@ -29,6 +35,82 @@ namespace {
  };
 } // end anonymous namespace

+std::error_code swift::atomicallyWritingToFile(
+    StringRef outputPath, bool binaryMode,
+    llvm::function_ref<void(llvm::raw_pwrite_stream &)> action) {


I wanted to check the correctness of this function for my review, but it is so long that I doubt my ability to do so.
If you were to break it up into smaller pieces, I could review it with confidence.

davidungar · 2018-07-25T21:05:45Z

lib/FrontendTool/FrontendTool.cpp

+/// filesystem errors and ignores empty output paths.
+static bool atomicallyWritingToTextFile(
+    StringRef outputPath, DiagnosticEngine &diags,
+    llvm::function_ref<bool(llvm::raw_pwrite_stream &)> action) {
  if (outputPath.empty())
    return false;


The comment does say that this function ignores empty output paths, but that calling convention does not seem as robust as one that would create an error for an empty output path. But, I don't know the context.

davidungar · 2018-07-25T21:08:41Z

lib/FrontendTool/FrontendTool.cpp

+      swift::atomicallyWritingToFile(outputPath, /*binary*/false,
+                                     [&](llvm::raw_pwrite_stream &out) {
+    actionFailed = action(out);
+  });


I think it might read a little more clearly with an early return before the if (EC):

if (!actionFailed) return false;

Then, you could move the return true; down to the last line of the function.

Hm, and have actionFailed default to true? Yeah, that makes sense.

Oops, no, that's not quite right. actionFailed and EC are independent.

Sigh... You are right. If the action fails (action returns true--BTW action return convention (true == error) ought to also be in the Doxygen, sigh) the file is still written. Is this right? Should the file be removed if action returns true?

I think yes, it's a temporary file and it would clutter up the user's directory.

(None of our actions currently fail, so this is kind of a moot point. Another option would be to take that part out.)

If we don't remove the file, at least we could include an assertion that the action succeeds with a FIXME about removing the file. Or something...

Oh, wait, hang on. Are you talking about removing the temporary file or the final output file? As written the code always cleans up the temporary file, but does not delete the final output file if there's an error in writing. I figured that was a safer option given Clang's mentioning "you can write to this file but not to the directory that contains it", although it does sometimes mean you've produced erroneous data.

If line 326 above evaluates to true, whatever file is written by atomicallyWritingToFile is left on the file system, whichever one that is. That’s what I saw.

Right, okay. That's intended but I could see how we might prefer otherwise.

davidungar · 2018-07-25T21:10:42Z

lib/FrontendTool/FrontendTool.cpp

+  return actionFailed;
+}
+
+static bool printAsObjCIfNeeded(StringRef outputPath, ModuleDecl *M,


The name says "IfNeeded" but the function has no "if" test in it. I presume that it has to do with an empty output path? Some sort of reformulation could be clearer.

Same for printModuleInterfaceIfNeeded below.

I didn't change the name of the function, only the implementation. I guess "atomicallyWritingToTextFile" should also have an "IfNeeded" or something?

My preference would be for an empty output path to be an error, and to only call the function with a non-empty output path. But one could instead append "IgnoringEmptyOutputPath" to the function name.

That's definitely too verbose—it makes the call site too hard to read.

I can push the empty string checks back out of these functions, but it was your refactoring of performCompile that put them inside to begin with. :-/

Thanks for asking. It's a question of centralizing the invariants. If there is only one call to printAsObjC, then I would put the test at the call site. If there are two, I would put the test in printAsObjCIfNeeded and have just test and call printAsObjC (or whatever). Same for emitModuleInterface[IfNeeded]. Does that make sense?

There's only one call to all of these (they were all inline in performCompile at one point), so I'll put it outside, then.

Oops, I take it back. There are two calls. Hm. So you're saying it's better to have the test in printAsObjCIfNeeded rather than sink it down to the helper function, since we haven't thought of a way to encode it in the helper function's name without me thinking it's too verbose?

Yes, that’s right. One place, so someday we can’t change one and miss the other. Tx

davidungar · 2018-07-25T21:11:47Z

lib/FrontendTool/TextualInterfaceGeneration.cpp

+using namespace swift;
+
+bool swift::emitModuleInterface(raw_ostream &out, ModuleDecl *M) {
+  auto printOptions = PrintOptions::printTextualInterfaceFile();


davidungar · 2018-07-25T21:14:37Z

lib/Serialization/Serialization.cpp

+  std::error_code EC = swift::atomicallyWritingToFile(outputPath,
+                                                      /*binary*/true,
+                                                      action);
+  if (EC) {


Not a big deal, but slightly better IMO to flip this with an early return if !EC.

davidungar · 2018-07-25T21:18:27Z

PS: Always a pleasure reviewing your code. Your care inspires me to look for perfection.

CodaFi · 2018-07-26T22:08:16Z

include/swift/Basic/FileSystem.h

+///
+/// In the latter case, the file at \p source is deleted. If an error occurs,
+/// the file at \p source will still be present at \p source.
+std::error_code moveFileIfDifferent(const llvm::Twine &source,


Yay, more things to upstream!

We'll want more complexity soon, but this is a start.

jrose-apple · 2018-08-01T21:35:06Z

Rebased, feedback incorporated.

@swift-ci Please smoke test

davidungar · 2018-08-01T22:11:37Z

include/swift/AST/PrintOptions.h

+  /// Retrieve the set of options suitable for stable textual interfaces.
+  ///
+  /// This is a format that will be parsed again later, so the output must be
+  /// consistent and well-formed.


davidungar · 2018-08-01T22:17:42Z

lib/Basic/FileSystem.cpp

+
+  fs::file_status status;
+  (void)fs::status(outputPath, status);
+  if (fs::exists(status)) {


Probably reads slightly better if you flip the "if" and use an early return.

Oh, of course. This is left over from before I pulled it out into its own function. Easy enough.

davidungar · 2018-08-01T22:18:58Z

lib/Basic/FileSystem.cpp

+
+    // Don't use a temporary if the output is a special file. This handles
+    // things like '-o /dev/null'
+    if (!fs::is_regular_file(status))


If you flip the if above, this turns into return fs::is_regular_file(status)

davidungar · 2018-08-01T22:19:58Z

lib/Basic/FileSystem.cpp

+
+  // Create a temporary file path.
+  // Insert -%%%%%%%% before the extension (if any), and because some tools
+  // (noticeable, clang's own GlobalModuleIndex.cpp) glob for build


Typo? "noticeably" instead of "noticeable"?

I guess so. It's not my typo; that bit is copied verbatim. They probably meant "notably".

davidungar · 2018-08-01T22:20:53Z

lib/Basic/FileSystem.cpp

+/// If the result is an error, the write won't succeed at all, and the calling
+/// operation should bail out early.
+static llvm::ErrorOr<bool>
+canUseTemporaryForWrite(const StringRef outputPath) {


I like that const

davidungar · 2018-08-01T22:23:10Z

lib/Basic/FileSystem.cpp

+  // Create a temporary file path.
+  // Insert -%%%%%%%% before the extension (if any), and because some tools
+  // (noticeable, clang's own GlobalModuleIndex.cpp) glob for build
+  // artifacts, also append .tmp.


Good comment, but I'm left wondering what would happen as a result of the tool appending .tmp if you didn't add the percents. Is this string just arbitrary? Does anything else count on it?

It's a mktemp-like pattern, described in the documentation for llvm::sys::fs::createUniqueFile.

davidungar · 2018-08-01T22:25:56Z

lib/Basic/FileSystem.cpp

+  tempPath = outputPath.drop_back(outputExtension.size());
+  tempPath += "-%%%%%%%%";
+  tempPath += outputExtension;
+  tempPath += ".tmp";


I wish we had Swift's string interpolation, sigh.

This replaces the use of a Clang utility function that was inexplicably a non-static member function of CompilerInstance. It would be nice to sink this all the way to LLVM and share the implementation across both projects, but the Clang implementation does a handful of things we don't need, and it's hard to justify including them in an LLVM-level interface. (I stared at llvm/Support/FileSystem.h for a long time before giving up.) Anyway, Serialization and FrontendTool both get their atomic writes now without depending on Clang, and without duplicating the scaffolding around the Clang API. We should probably adopt this for all our output files. No functionality change.

No functionality change.

jrose-apple · 2018-08-01T23:41:51Z

@swift-ci Please smoke test

davidungar · 2018-08-02T00:44:14Z

lib/Basic/FileSystem.cpp

+  if (std::error_code error = canUseTemporary.getError())
+    return error;
+
+  Optional<std::string> temporaryPath;


I like how the refactoring is going! When I see a declaration, followed by a block that computes it, I wonder if it works better as a separate function. But I'm not sure I see that factoring here would be comprehensible, even though the code wants it.

Yeah. It's almost llvm::ErrorOr<Optional<std::string>> but trying to work with that type (and trying to name the function that would be extracted) seems worse than any of the alternatives. tryToOpenTemporaryFile represents the best compromise I came up with.

davidungar

This is looking pretty good! There's still a lot going on in swift::atomicallyWritingToFile but I don't see a sensible decomposition.

jrose-apple requested review from davidungar and CodaFi July 25, 2018 20:31

davidungar reviewed Jul 25, 2018

View reviewed changes

CodaFi reviewed Jul 26, 2018

View reviewed changes

Hook up -emit-interface-path to a simple AST printer

e8e0584

We'll want more complexity soon, but this is a start.

jrose-apple force-pushed the a-journey-of-a-thousand-miles branch from caafaee to aacc7ae Compare August 1, 2018 21:34

davidungar reviewed Aug 1, 2018

View reviewed changes

jrose-apple added 2 commits August 1, 2018 16:41

[Frontend] Factor out textual interface generation into its own file

0204561

No functionality change.

jrose-apple force-pushed the a-journey-of-a-thousand-miles branch from aacc7ae to 0204561 Compare August 1, 2018 23:41

davidungar reviewed Aug 2, 2018

View reviewed changes

davidungar approved these changes Aug 2, 2018

View reviewed changes

jrose-apple merged commit ae04c67 into swiftlang:master Aug 2, 2018

jrose-apple deleted the a-journey-of-a-thousand-miles branch August 2, 2018 01:37

Hook up -emit-interface-path to a simple AST printer #18224

Hook up -emit-interface-path to a simple AST printer #18224

Uh oh!

Conversation

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

Uh oh!

Uh oh!

jrose-apple commented Jul 25, 2018

Uh oh!

jrose-apple commented Jul 25, 2018

Uh oh!

swift-ci commented Jul 25, 2018

Uh oh!

jrose-apple commented Jul 25, 2018

Uh oh!

davidungar 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!

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!

davidungar commented Jul 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrose-apple commented Aug 1, 2018

Uh oh!

jrose-apple commented Jul 25, 2018 •

edited

Loading