SwiftSyntax Parser: expose parser diagnostics via C API. #22992

nkcsgexi · 2019-02-28T19:33:18Z

This allows SwiftSyntax to listen to emitted diagnostics during
parsing.

rdar://48439271

nkcsgexi · 2019-02-28T19:33:26Z

@swift-ci please smoke test

akyrtzi · 2019-02-28T23:55:27Z

include/swift-c/SyntaxParser/SwiftSyntaxParser.h

+  swiftparse_range_t range;
+  /// Represent the text for replacement.
+  const char* text;
+} swiftparse_fixit_t;


A bit better to name this swiftparse_diagnostic_fixit_t

akyrtzi · 2019-02-28T23:58:00Z

include/swift-c/SyntaxParser/SwiftSyntaxParser.h

+  const char* text;
+} swiftparse_fixit_t;
+
+enum swiftparser_diagnostic_severity {


For enums use this name convention: https://github.com/apple/swift/blob/8521015407016d93201be98db378217cbee8af37/tools/SourceKit/tools/sourcekitd/include/sourcekitd/sourcekitd.h#L305

akyrtzi · 2019-02-28T23:59:49Z

include/swift-c/SyntaxParser/SwiftSyntaxParser.h

+  swiftparser_diagnostic_severity_Note = 3,
+};
+
+/// This is for the client to provide an opaque pointer that the parser will


This comment seems incorrect, swiftparser_diagnostic_t is provided by the parser, not the client.

akyrtzi · 2019-03-01T00:01:42Z

include/swift-c/SyntaxParser/SwiftSyntaxParser.h

+
+/// This is for the client to provide an opaque pointer that the parser will
+/// associate with a diagnostic.
+/// This pointer is only valid when received from the argument of a


Bit better to re-phrase "this pointer is only valid to access from within the swiftparse_diagnostic_handler_t block

akyrtzi · 2019-03-01T00:06:12Z

include/swift-c/SyntaxParser/SwiftSyntaxParser.h

+typedef void* swiftparser_diagnostic_t;
+
+/// Invoked by the parser when a diagnostic is emitted.
+typedef void(^swiftparse_diagnostic_handler_t)(const swiftparser_diagnostic_t);


You are passing a swiftparser_diagnostic_t as const in the block but then the swiftparse_diagnostic_* functions accept a const-less swiftparser_diagnostic_t parameter, which would force to the clients to cast the const away from the variable before calling the functions. Try changing the definition of the type to typedef const void* swiftparser_diagnostic_t; instead.

tools/libSwiftSyntaxParser/libSwiftSyntaxParser.cpp

akyrtzi · 2019-03-01T00:36:19Z

tools/libSwiftSyntaxParser/libSwiftSyntaxParser.cpp

+swiftparse_diagnostic_get_fixit_at_index(swiftparser_diagnostic_t diag,
+                                         unsigned idx) {
+  auto allFixits = static_cast<DiagnosticDetail*>(diag)->AllFixits;
+  assert(allFixits.size() >= idx);


idx must be less than size() it will be buffer overflow if it is equal to size().

akyrtzi · 2019-03-01T00:47:18Z

tools/libSwiftSyntaxParser/libSwiftSyntaxParser.cpp

@@ -37,9 +37,26 @@ typedef swiftparse_trivia_piece_t CTriviaPiece;
 typedef swiftparse_syntax_kind_t CSyntaxKind;

 namespace {
+
+unsigned getByteOffset(SourceLoc Loc, SourceManager &SM, unsigned BufferID) {


Nitpick but I much prefer that static functions have static in front of them. Otherwise it is not obvious to me that the functions are static unless I check that they are contained in an anonymous namespace, which can be far apart from the function declaration. Same for the function below.

akyrtzi · 2019-03-01T00:48:52Z

tools/libSwiftSyntaxParser/libSwiftSyntaxParser.cpp

@@ -101,16 +128,6 @@ class CLibParseActions : public SyntaxParseActions {
    }
  }

-  void makeCRange(CRange &c_range, CharSourceRange range) {


You don't have to remove it, you can have this method delegate to the global static function, e.g.:

void makeCRange(CRange &c_range, CharSourceRange range) { return makeCRange(c_range, range, SM, BufferID); }

that way you don't have to update the callers and have to pass the extra parameters at the call sites.

akyrtzi · 2019-03-01T00:49:13Z

tools/libSwiftSyntaxParser/libSwiftSyntaxParser.cpp

@@ -179,8 +196,68 @@ class CLibParseActions : public SyntaxParseActions {
    return {result.length, result.node};
  }
 };
+
+swiftparser_diagnostic_severity getSeverity(DiagnosticKind Kind) {


Add static.

Returning an enum value is also subject to ABI and API concerns if we decide to add a severity kind in the future/do portability work later. Not sure how much that factors into the current design.

What are your concerns ? AFAIK for enums in stable C APIs you only have to make sure to set specific integer values to the enum values and make sure to not change those integers in future versions. If you do add a new enum, bump the API version macro. This is what we have been doing with the libclang C API.

So that handles the API concerns.

As for ABI, the underlying type of an enumeration is implementation-defined which means there’s a hypothetical world in which the addition of a case (say with a negative tag) causes the compiler to select a new layout and break everybody.

This is not an issue because:

This enum will just have a handful of values

This library is intended to be used with a matching SwiftSyntax (their syntax definitions must match), you can't just mix-and-match them arbitrarily.

CodaFi · 2019-03-01T15:31:51Z

Please consider adding an in/out parameter for the length of any C strings this API takes/returns. LLVM’s abstractions that map raw strings have an interesting relationship to NUL bytes, especially internal NUL bytes.

nkcsgexi · 2019-03-01T19:13:55Z

@CodaFi would it really be a problem? These null-terminated string segments will be used in the SwiftSyntax side to copy to a Swift string and discarded immediately afterwards.

nkcsgexi · 2019-03-01T21:22:48Z

@akyrtzi all comments are done

include/swift-c/SyntaxParser/SwiftSyntaxParser.h

test/Syntax/Parser/diags.swift

This allows SwiftSyntax to listen to emitted diagnostics during parsing. rdar://48439271

nkcsgexi · 2019-03-02T00:01:17Z

@swift-ci please smoke test

akyrtzi

Good job! 👍

nkcsgexi requested a review from akyrtzi February 28, 2019 19:33

akyrtzi requested changes Mar 1, 2019

View reviewed changes

nkcsgexi force-pushed the diagnostics-c-api branch from 2fb07d2 to 339d69d Compare March 1, 2019 21:22

akyrtzi requested changes Mar 1, 2019

View reviewed changes

SwiftSyntax Parser: expose parser diagnostics via C API.

a86f89d

This allows SwiftSyntax to listen to emitted diagnostics during parsing. rdar://48439271

nkcsgexi force-pushed the diagnostics-c-api branch from 339d69d to a86f89d Compare March 1, 2019 23:22

akyrtzi approved these changes Mar 2, 2019

View reviewed changes

nkcsgexi merged commit 740b476 into swiftlang:master Mar 2, 2019

SwiftSyntax Parser: expose parser diagnostics via C API. #22992

SwiftSyntax Parser: expose parser diagnostics via C API. #22992

Uh oh!

Conversation

nkcsgexi commented Feb 28, 2019

Uh oh!

nkcsgexi commented Feb 28, 2019

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!

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!

CodaFi commented Mar 1, 2019

Uh oh!

nkcsgexi commented Mar 1, 2019

Uh oh!

nkcsgexi commented Mar 1, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nkcsgexi commented Mar 2, 2019

Uh oh!

akyrtzi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!