[SwiftCompiler] Add DiagnosticEngine bridging #41500

rintaro · 2022-02-21T19:47:32Z

SourceLoc and CharSourceRange bridging in Basic
New AST bridging. DiagID, DiagnosticArgument, DiagnosticFixIt, and DiagnosticEngine

Preparation for Regex literal parsing diagnostics. #41492

rintaro · 2022-02-21T19:49:24Z

@swift-ci Please smoke test

rintaro · 2022-02-21T22:09:35Z

@swift-ci Please smoke test

AnthonyLatsis · 2022-02-21T22:34:42Z

SwiftCompilerSources/Sources/Basic/SourceLoc.swift

+public struct CharSourceRange {
+  public var start: SourceLoc
+  public var byteLength: Int


Why are these not let?

In general, if a struct provides a member-wise initializer, there is no reason to make any of its properties let. Even if we make these let, others can

extension CharSourceRange { mutating func setByteLength(val: Int) { self = .init(start: start, byteLength: value) } }

Given this is Swift vs. C++, exposing opportunities for mutating public storage directly on an instance when it is not intended as publicly mutable sounds like an antipattern, in terms of how error-prone this can be in imperative code and when maintaining state integrity. In the extension it would at least be an "atomic" mutation. This is a personal opinion though.

I agree with @AnthonyLatsis here. Making properties let enforces a more stateless design of the code, which is good to keep the code simple. If a property is var, it adds unnecessary complexity.

OK, I changed them to let

eeckstein · 2022-02-23T17:03:16Z

SwiftCompilerSources/Sources/AST/DiagnosticEngine.swift

+//
+//===----------------------------------------------------------------------===//
+
+@_exported import ASTBridging


The API of DiagnosticEngine should be a pure swift API. So there should be no need to export bridging stuff

Removed @_exporeted

eeckstein · 2022-02-23T17:11:03Z

SwiftCompilerSources/Sources/AST/DiagnosticEngine.swift

+  init(int val: Int) {
+    self.init(kind: .int, value: .init(intValue: val))
+  }
+  init(uInt val: UInt) {


Do we really need UInt?

Not entirely sure.
Even if the C++ diagnostic ID declared to receive an uint argument, as far as I can read from the DiagnosticEngine C++ code, it accepts DiagnosticArgument(int) value too. So maybe it's not needed. Since we can add it later, I just removed it for now.

eeckstein · 2022-02-23T17:21:41Z

SwiftCompilerSources/Sources/AST/DiagnosticEngine.swift

+    var bridgedArgs: [BridgedDiagnosticArgument] = []
+    var bridgedFixIts: [BridgedDiagnosticFixIt] = []
+
+    var closure: () -> Void = {


Wow, this trick with the closure definitely needs a comment.

Added some comment

eeckstein · 2022-02-23T17:23:08Z

SwiftCompilerSources/Sources/AST/DiagnosticEngine.swift

+}
+
+public protocol DiagnosticArgument {
+  func withBridgedDiagnosticArgument(_ fn: (BridgedDiagnosticArgument) -> Void)


I guess it's unavoidable to make withBridgedDiagnosticArgument public.
I suggest you make it an underscored name (_withBridgedDiagnosticArgument) to indicate it's not part of the public API.

eeckstein · 2022-02-23T17:27:39Z

SwiftCompilerSources/Sources/Basic/SourceLoc.swift

+public struct CharSourceRange {
+  public var start: SourceLoc
+  public var byteLength: Int


I agree with @AnthonyLatsis here. Making properties let enforces a more stateless design of the code, which is good to keep the code simple. If a property is var, it adds unnecessary complexity.

eeckstein · 2022-02-23T17:41:31Z

SwiftCompilerSources/Sources/Basic/SourceLoc.swift

+    self.pointer = UnsafeRawPointer(pointer).assumingMemoryBound(to: UInt8.self)
+  }
+
+  public init?(bridged: BridgedSourceLoc) {


this shouldn't be a public API

In future, in other modules (i.e. Parse), we want to receiveBasic's Bridged* from C++
Like:

//-- Parse/Regex.swift import Basic // To be called from C++. func parseRegexLiteral(_ bridgedLoc: BridgedSourceLoc, _ length: Int, ....) { let sourceLoc = SourceLoc(bridged: bridgedLoc) ... }

So init(bridged:) needs to be public.

eeckstein · 2022-02-23T17:41:47Z

SwiftCompilerSources/Sources/Basic/SourceLoc.swift

+    self.init(pointer: pointer)
+  }
+
+  public var bridged: BridgedSourceLoc {


This file is in Basic.
var bridged is used in DiagnosticEngine.diagnose() in AST to pass the bridged values to DiagnosticEngine_diagnose() C function.
I can make this underscored, but it'll be inconsistent with StringRef or ArrayRef's withBridgedXXX

eeckstein · 2022-02-23T17:42:09Z

SwiftCompilerSources/Sources/Basic/SourceLoc.swift

+    self.byteLength = byteLength
+  }
+
+  public init?(bridged: BridgedCharSourceRange) {


This shouldn't be a public API

Same reason as SourceLoc

eeckstein · 2022-02-23T17:42:25Z

SwiftCompilerSources/Sources/Basic/SourceLoc.swift

+    self.init(start: start, byteLength: bridged.byteLength)
+  }
+
+  public var bridged: BridgedCharSourceRange {


Same as SourceLoc

eeckstein · 2022-02-23T17:50:09Z

SwiftCompilerSources/Sources/Basic/SourceLoc.swift

+//
+//===----------------------------------------------------------------------===//
+
+public struct SourceLoc {


I always found it confusing that we have multiple data types to describe a source location (in C++): SourceLoc and SILLocation.
I think we can do better on the swift side. We should unify AST.SourceLoc and SIL.Location and have only a single data type for that.

SourceLoc and SILLocation are very different type. SourceLoc is basically just a pointer, whereas SILLocation is a much more complex object. I'm not sure how we could unify them...

eeckstein · 2022-02-23T18:17:03Z

SwiftCompilerSources/Sources/AST/DiagnosticEngine.swift

+
+  public func diagnose(_ position: SourceLoc?,
+                       _ id: DiagID,
+                       _ args: [DiagnosticArgument],


Can you make this a var-arg parameter instead of an array?

I added a var-arg variant, but I still want Array one too. It's convenient when, for example, using Swift's enum as an diagnostic type.
For example:

struct ParserDiagnostic { enum Message { case expectedNumDigits(String, Int) case miscError(String) case otherError } let location: Location let message: Message } func reportDiag(diag: ParseError) { let location = getSourceLoc(location) let diagID: DiagID let arguments: [DiagnosticArguement] switch diag.message { case expectedNumDigits(let s, let i): diagID = .parse_regex_expected_num_digits arguments = [s, I] case ... ... } engine.diagnose(loc, diagID, arguments) }

Otherwise, we need to call DiagnositcEngine.diagnose() from each case

* 'SourceLoc' and 'CharSourceRange' bridging in Basic * New 'AST' bridging. 'DiagID', 'DiagnosticArgument', 'DiagnosticFixIt', and 'DiagnosticEngine'

eeckstein

lgtm, thanks!

rintaro · 2022-02-25T17:30:40Z

@swift-ci Please smoke test

rintaro mentioned this pull request Feb 21, 2022

[SwiftCompiler] Use bridged DiagnosticEngine to emit regex literal parsing #41492

Closed

rintaro force-pushed the swiftcompiler-ast branch from 80833c5 to 05d5e0c Compare February 21, 2022 22:09

AnthonyLatsis reviewed Feb 21, 2022

View reviewed changes

rintaro requested a review from eeckstein February 22, 2022 02:54

eeckstein requested changes Feb 23, 2022

View reviewed changes

eeckstein reviewed Feb 23, 2022

View reviewed changes

[SwiftCompiler] Add DiagnosticEngine bridging

4d9b65d

* 'SourceLoc' and 'CharSourceRange' bridging in Basic * New 'AST' bridging. 'DiagID', 'DiagnosticArgument', 'DiagnosticFixIt', and 'DiagnosticEngine'

rintaro force-pushed the swiftcompiler-ast branch from 05d5e0c to 4d9b65d Compare February 24, 2022 00:13

eeckstein approved these changes Feb 25, 2022

View reviewed changes

rintaro merged commit c9e3699 into swiftlang:main Feb 25, 2022

[SwiftCompiler] Add DiagnosticEngine bridging #41500

[SwiftCompiler] Add DiagnosticEngine bridging #41500

Conversation

rintaro commented Feb 21, 2022

Uh oh!

rintaro commented Feb 21, 2022

Uh oh!

rintaro commented Feb 21, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rintaro Feb 21, 2022 • 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!

AnthonyLatsis Feb 21, 2022 • 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!

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!

eeckstein Feb 23, 2022 • 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!

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!

rintaro Feb 24, 2022 • 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!

eeckstein left a comment

Choose a reason for hiding this comment

Uh oh!

rintaro commented Feb 25, 2022

Uh oh!

Uh oh!

rintaro Feb 21, 2022 •

edited

Loading

AnthonyLatsis Feb 21, 2022 •

edited

Loading

eeckstein Feb 23, 2022 •

edited

Loading

rintaro Feb 24, 2022 •

edited

Loading