Skip to content

Add Double and Int Convenience Properties #234

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 6 commits into from
Closed

Add Double and Int Convenience Properties #234

wants to merge 6 commits into from

Conversation

vermont42
Copy link
Contributor

Building on the work of @ahoppen on SR-11580, this PR adds convenience properties to get Double and Int values from FloatLiteralExprSyntax and IntegerLiteralExprSyntax, respectively.

The two new properties, floatingValue and intValue, use failable initializers of Double and Int: init?<S>(_ text: S) where S : StringProtocol and init?<S>(_ text: S, radix: Int) where S : StringProtocol. These initializers fail if the Strings passed in do not represent valid Doubles or Ints. The question arises: what should floatingValue and intValue return, if anything, if initialization fails? I considered the following possibilities, ultimately choosing the fourth.

  1. floatingValue could return Double.nan. This would make sense because, for example, "🍕.🌍" is truly not a number, floating-point or otherwise. The problem with this approach is that there is no corresponding Int.nan, which would prevent floatingValue and intValue from returning the same sort of value given invalid input. Decimal does have a nan value, so if intValue returned a Decimal rather than an Int, that would solve the problem, but I, as a consumer of the API, would find this counter-intuitive because "42" in a source file represents an Int, not a Decimal.

  2. The return types of floatingValue and intValue could be optional. If Double or Int initialization fails, floatingValue and intValue could therefore return nil. I preferred not to impose on API consumers the burden of unwrapping.

  3. The return types of floatingValue and intValue could be Result<Double, LiteralError> and Result<Int, LiteralError>, respectively. If Double or Int initialization fails, floatingValue and intValue could return .failure(.invalid). I preferred not to impose on API consumers the burden of switching on the returned value.

  4. The result of the Double or Int initialization can be force-unwrapped. This causes a crash if initialization fails. Crashing is considered harmful, though not by everyone. Force-unwrapping is the most convenient option for API consumers, which is why I chose it. I observe that force-unwrapping does appear elsewhere in SwiftSyntax, for example here, here, here, and here, and I suspect that the authors of this code made similar judgments about the convenience of force-unwrapping versus the crashing risk it imposes.

Comment on lines 7 to 11
testFloatingValue(text: "5.3_8", expectedValue: 5.38)
testFloatingValue(text: "12e3", expectedValue: 12000.0)
testFloatingValue(text: "32E1", expectedValue: 320.0)
testFloatingValue(text: "0xdEFACE.C0FFEEp+1", expectedValue: 29226397.507810354)
testFloatingValue(text: "0xaffab1e.e1fP-2", expectedValue: 46131911.72064209)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could have used the following approach to avoid repetition of the testFloatingValue() call:

[
  ("5.3_8", 5.38),
  ("12e3", 12000.0),
  ("32E1", 320.0),
  ("0xdEFACE.C0FFEEp+1", 29226397.507810354),
  ("0xaffab1e.e1fP-2", 46131911.72064209)
].forEach {
  testFloatingValue(text: $0.0, expectedValue: $0.1)
}

Although this approach would have been DRYer, I find that it makes failure diagnosis more difficult, so I did not use it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The testing approach seems good to me. One idea though (I’m not quite sure if it’s an improvement or not) would be to use the literal that you’re specifying in the String also in the Swift source code, i.e.\

testFloatingValue(text: "0xaffab1e.e1fP-2", expectedValue: 0xaffab1e.e1fP-2)

Mostly because my mental hex parsing skills aren't brilliant and I’m not quite sure if 0xaffab1e.e1fP-2 is really equal to 46131911.72064209.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. As an aside, this is my go-to source for fun hex words.

@akyrtzi
Copy link
Contributor

akyrtzi commented Aug 11, 2020

@vermont42 thanks for working on this! I would be fine with #4 (force-unwrap) since it can be considered a violation of tree invariants if an IntegerLiteralExprSyntax node doesn't actually contain an integer.

However, if we do force-unwrap we need to protect against the tree getting into such state in the first place. I think we can safely assume that the parser will not provide such a node (and if it does that's a parser bug), but another source of these are the APIs for creating syntax nodes programmatically. It is important to catch invariant breakage at the earliest source, so we should prevent a case of manually creating a IntegerLiteralExprSyntax node with a string that is not an integer.

That may require changing the python code on the swift repo side that does the code-generation for the programmatic APIs. e.g. maybe add a flag to indicate that the programmatic API for a node will be manually written on the SwiftSyntax side and avoid code-generating, then we write the functions manually and include the validation aspect.

textWithoutPrefix = String(text.suffix(from: digitsStartIndex))

let textWithoutPrefixOrUnderscores = textWithoutPrefix.filter {
$0 != "_"
Copy link
Member

@rintaro rintaro Aug 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically, _ at the start of each component is not valid. e.g. _12, 0x_12, 0._12 and 12e_3. But I think it's OK to accept them for now.
@akyrtzi What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine but adding a FIXME comment here would be good.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rintaro I observe that _12 and 0x_2 don't parse as IntegerLiteralExprSyntax or FloatLiteralExprSyntax, respectively. My properties therefore shouldn't get those sorts of input. Should the properties nonetheless fatalError() if the _ is the first character after the base prefix (or is the first character in the case of base 10)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As @akyrtzi said in another comment, we should prevent a case of creating syntax node with invalid literal text. If you do that, you don't need to check _ in this integerValue or floatingValue accessor.

Also, when you check them, you need to check not only for the integral part, but you also need to check the fractional part and the exponent part. That's not so simple to implement in this PR. So I suggest you to add a FIXME comment as @akyrtzi said.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say that for now it’s sufficient if we uphold the invariant that the contents will of a integer / float literal are always parsable from the integerValue resp. floatingValue properties, especially since the implementation is nice and short so far!

That should already be an improvement over the current behaviour and if we add a FIXME, we can come back to it and improve it later.

return Int(textWithoutPrefixOrUnderscores, radix: type.radix())!
}

private enum IntegerType {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like this type is overkill. It could be a single inner function returning a tuple of prefixLength and radix

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Comment on lines 14 to 13
private func testFloatingValue(text: String, expectedValue: Double) {
let digits = SyntaxFactory.makeFloatingLiteral(text)
let literalExpr = SyntaxFactory.makeFloatLiteralExpr(floatingDigits: digits)
XCTAssertEqual(literalExpr.floatingValue, expectedValue)
}
Copy link
Member

@rintaro rintaro Aug 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a stylistic request, could you move this testFloatingValue and testIntegerValue out of SyntaxConvenienceMethodsTests type? They are not "test" methods for XCTest and can be free functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@ahoppen
Copy link
Member

ahoppen commented Aug 12, 2020

However, if we do force-unwrap we need to protect against the tree getting into such state in the first place. I think we can safely assume that the parser will not provide such a node (and if it does that's a parser bug), but another source of these are the APIs for creating syntax nodes programmatically. It is important to catch invariant breakage at the earliest source, so we should prevent a case of manually creating a IntegerLiteralExprSyntax node with a string that is not an integer.

@akyrtzi I’ve just been digging around the generated code to give @vermont42 some advice on what needs to be modified. While doing so I have been thinking what the desired API would be. I would argue that making all methods that construct a integer/float literal return an Optional would be the cleanest solution, even though it means that these are the only types with a failable initialiser. What do you think?

@akyrtzi
Copy link
Contributor

akyrtzi commented Aug 12, 2020

I would argue that making all methods that construct a integer/float literal return an Optional would be the cleanest solution, even though it means that these are the only types with a failable initialiser. What do you think?

That makes sense to me 👍

Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very nice overall. The implementation turned out to be a lot easier than I thought it would be (I thought we would need to write our own integer literal parser). 🥳 I’ve just got a couple comments inline.

As for making sure that the IntegerLiteralExpr always contain a token that is a valid literal, I can give you a hand at modifying the Python code. I know it has a pretty steep learning curve. 😉

let floatingDigitsWithoutUnderscores = floatingDigits.text.filter {
$0 != "_"
}
return Double(floatingDigitsWithoutUnderscores)!
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn’t have thought that the Double initialiser can handle hex float literals.

I think it’s odd though, that the Double initialiser is able to handle hex literals while the Int initialiser is not. It’s not really part of this PR but it might be worth filing a bug report against the standard library to either support hex literals in both or neither of them.

let text = self.digits.text
let (prefixLength, radix) = prefixLengthAndRadix(text: text)
let digitsStartIndex = text.index(text.startIndex, offsetBy: prefixLength)
let textWithoutPrefix = String(text.suffix(from: digitsStartIndex))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t think you need the String initialiser wrapping text.suffix. It should be more efficient if the filter in the next line directly works on the Substring returned by suffix instead of copying the substring data to a new String buffer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

textWithoutPrefix = String(text.suffix(from: digitsStartIndex))

let textWithoutPrefixOrUnderscores = textWithoutPrefix.filter {
$0 != "_"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say that for now it’s sufficient if we uphold the invariant that the contents will of a integer / float literal are always parsable from the integerValue resp. floatingValue properties, especially since the implementation is nice and short so far!

That should already be an improvement over the current behaviour and if we add a FIXME, we can come back to it and improve it later.


switch String(text.prefix(nonDecimalPrefixLength)) {
case binaryPrefix:
return (nonDecimalPrefixLength, binaryRadix)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about using binaryPrefix.count instead of the nonDecimalPrefixLength variable? Same below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Comment on lines 7 to 11
testFloatingValue(text: "5.3_8", expectedValue: 5.38)
testFloatingValue(text: "12e3", expectedValue: 12000.0)
testFloatingValue(text: "32E1", expectedValue: 320.0)
testFloatingValue(text: "0xdEFACE.C0FFEEp+1", expectedValue: 29226397.507810354)
testFloatingValue(text: "0xaffab1e.e1fP-2", expectedValue: 46131911.72064209)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The testing approach seems good to me. One idea though (I’m not quite sure if it’s an improvement or not) would be to use the literal that you’re specifying in the String also in the Swift source code, i.e.\

testFloatingValue(text: "0xaffab1e.e1fP-2", expectedValue: 0xaffab1e.e1fP-2)

Mostly because my mental hex parsing skills aren't brilliant and I’m not quite sure if 0xaffab1e.e1fP-2 is really equal to 46131911.72064209.

@vermont42
Copy link
Contributor Author

Update

I have partially implemented the suggestion of upholding the invariant. Specifically, I modified the public init?(_ syntax: Syntax) { initializer to validate but am still figuring out how to modify the internal init(_ data: SyntaxData) { initializer.

The invariant code relies on the following non-breaking code changes in the main Swift repo. Do reviewers agree that, after this SwiftSyntax PR is good to go, I should PR the main Swift repo with the Python changes, wait for that to land, and then request that the SwiftSyntax PR be merged?

ExprNodes.py:

    Node('FloatLiteralExpr', kind='Expr',
         children=[
             Child('FloatingDigits', kind='FloatingLiteralToken'),
         ],
         must_uphold_invariant=True),     // NEW
    Node('IntegerLiteralExpr', kind='Expr',
         children=[
             Child('Digits', kind='IntegerLiteralToken'),
         ],
         must_uphold_invariant=True),     // NEW

Node.py:

class Node(object):
   [omitted]
    def __init__(self, name, description=None, kind=None, traits=None,
                 children=None, element=None, element_name=None,
                 element_choices=None, omit_when_empty=False,
                 must_uphold_invariant=False):     // NEW
       [omitted]
        self.must_uphold_invariant = must_uphold_invariant     // NEW

@vermont42
Copy link
Contributor Author

Update

I pushed a commit causing the internal init(_ data: SyntaxData) { initializer to perform validation and be failable.

Comment on lines 70 to 73
public extension ExprSyntaxProtocol {
var isValidLiteral: Bool {
if let integerLiteralExprSyntax = self as? IntegerLiteralExprSyntax {
return integerLiteralExprSyntax.potentialIntegerValue != nil
} else if let floatLiteralExprSyntax = self as? FloatLiteralExprSyntax {
return floatLiteralExprSyntax.potentialFloatingValue != nil
} else {
return false
}
}
Copy link
Member

@rintaro rintaro Aug 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isValidLiteral is too specific to these types. Could you make it more generic name like isValid, validate() or something?
Also, I think you should split this function for each types like:

extension IntegerLiteralExprSyntax {
  func validate() -> Bool { potentialIntegerValue != nil }
}
extension FloatLiteralExprSyntax {
  func _validate() -> Bool { potentialFloatingValue != nil }
}

This way, compiler can check that the validation function is actually implemented for every must_uphold_invariant node.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Following the isEmpty example in the API Design Guidelines, I chose to leave it as a computed property and spell it isValid.

Comment on lines 632 to 638
public static func makeBlankFloatLiteralExpr() -> FloatLiteralExprSyntax {
let data = SyntaxData.forRoot(RawSyntax.create(kind: .floatLiteralExpr,
layout: [
RawSyntax.missingToken(TokenKind.floatingLiteral("")),
], length: .zero, presence: .present))
return FloatLiteralExprSyntax(data)
return FloatLiteralExprSyntax(data)!
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this always crashes because .floatingLiteral token is constructed with an empty string.
IMO this function is completely useless in the first place. Could you avoid auto generating this and makeBlankIntegerLiteralExpr?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -626,15 +626,15 @@ public enum SyntaxFactory {
let raw = RawSyntax.createAndCalcLength(kind: SyntaxKind.floatLiteralExpr,
layout: layout, presence: SourcePresence.present)
let data = SyntaxData.forRoot(raw)
return FloatLiteralExprSyntax(data)
return FloatLiteralExprSyntax(data)!
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This crashes if the argument floatingDigits is invalid.
Could you make this makeFloatLiteralExpr and makeIntegerLiteralExpr failable (i.e. return Optional)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Getting the Python indentation in SyntaxFactory.swift.gyb right was tricky. 😅

Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looking really good. Congrats on getting the entire gyb-generation pipeline to work.

I’ve posted a couple comments on the handling of optionals inline but they should be easy to fix.

@@ -82,7 +86,7 @@ public enum SyntaxFactory {
}
% end

% if not node.is_base():
% if not node.is_base() and not node.must_uphold_invariant:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically, I don’t think it’s correct to remove the makeBlank method if a node must uphold an invariant.

In the future we might add invariants to other nodes as well that could have a valid blank content. One example that jumps to my mind would be string literals. For consistency, I would rather have this method return an Optional and return nil if the node kind has no valid blank representation.

@akyrtzi What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to step back a bit here, what is the use case for the makeBlank methods, why do we need them? When would I call a makeBlank method instead of explicitly creating the syntax node that I want?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Paging @harlanhaskins for my question.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh sorry! I think makeBlank was meant to be used for e.g. error recovery where we'd just fill in missing tokens with blank values in order to send a well-formed-but-invalid AST to semantic analysis.

@@ -156,7 +170,11 @@ public struct ${node.name}: ${base_type}Protocol, SyntaxHashable {
let raw = newChild?.raw ?? ${make_missing_swift_child(child)}
% end
let newData = data.replacingChild(raw, at: Cursor.${child.swift_name})
% if node.must_uphold_invariant:
return ${node.name}(newData)!
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We definitely shouldn‘t force unwrap here. The invariant will not be satisfied if the user passes in a child that does not satisfy the invariant (e.g. an invalid integer literal). Instead, this method should return an Optional.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -91,7 +91,11 @@ open class SyntaxRewriter {
if let newNode = visitAny(node._syntaxNode) { return newNode }
return Syntax(visit(node))
% else:
% if node.must_uphold_invariant:
let node = ${node.name}(data)!
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a comment like the following here to document why the use of the ! is safe here?

// We know that the SyntaxData is valid since we are walking a valid syntax tree and haven't modified the syntax data.  Thus the below initialiser will never return nil.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you forgot to add the comment ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry about that. Fixed.

@vermont42
Copy link
Contributor Author

I implemented @ahoppen's comment suggestions but will hold off on implementing the other suggestions in order to give @rintaro and @akyrtzi the opportunity to respond.

Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for yet another round and your persistence ;-)

I have looked through the code again. I’ve got two more nitpicks and just realised that setting an invalid text as the contents of an integer literal crashes. I’ve added some comments inline.

Other than that, I think we’re really close.

return Int(textWithoutPrefixOrUnderscores, radix: radix)
}

static func prefixLengthAndRadix(text: String) -> (Int, Int) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small nitpick after another round of review: I think this method should be private.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. (For real. 😅)

@@ -108,7 +123,11 @@ public struct ${node.name}: ${base_type}Protocol, SyntaxHashable {
return ${child.type_name}(childData!)
}
set(value) {
% if node.must_uphold_invariant:
self = with${child.name}(value)!
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just realised that this will crash when setting the digits of an integer literal to an invalid integer literal. This is definitely not ideal. I see four options on how to resolve this.
a) Maintain the crashing behaviour and add a comment to the setter that at least explains the crash
b) Remove the property setter on all types and add a set${child.name} method that throws if the invariant is not satisfied
c) Only replace the property setter by a function on types that have an invariant
d) Remove the setter altogether and only use the with${child.name} to modify types

From my point of view
a) leads to surprising crashes for the user so I don’t prefer it
b) offers a consistent API across all types that is safe but does not follow standard Swift conventions
c) maintains standard Swift conventions for most types but maybe makes the throwing setters less discoverable because the API is no longer consistent between Syntax types
d) removes a IMHO useful setter

I don’t have strong feelings between b) and c). @vermont42 What do you think is better?

Also: If you change the API, could you document it in Changelog.md and add a test that setting an invalid token as the text of an integer literal behaves correctly?

@akyrtzi: Do you have an opinion on this?

@@ -91,7 +91,11 @@ open class SyntaxRewriter {
if let newNode = visitAny(node._syntaxNode) { return newNode }
return Syntax(visit(node))
% else:
% if node.must_uphold_invariant:
let node = ${node.name}(data)!
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you forgot to add the comment ;-)

@vermont42
Copy link
Contributor Author

@ahoppen I removed the two setters and converted them into throwing functions, preferring that approach over converting all setters to throwing functions, notwithstanding the inconsistency, because throwing functions are less pleasant for callers to use than garden-variety setters and are therefore to be avoided when possible. I think some inconsistency between, on the one hand,FloatLiteralExprSyntax and IntegerLiteralExprSyntax, and, on the other, everything else is actually warranted because of the constraints on validity to which these two types, but not others, are subject. I also added the test you mentioned and updated the changelog.

I am unclear on what, if anything, still needs to be done about Blanks. I can either leave the current approach of not generating them or restore them and return nil.

Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one. I’ve got two minor comments inline that should be super easy to fix.

Regarding the Blank methods: I’m fine with the way it is right now. We can discuss what to do with them in a follow-up PR to get this one landed.

Could you also open a PR in the main Swift repos with your changes to swift_syntax_support? There’s no need to write a long description there, just reference this PR. Afterwards I can throw CI against your changes and we will see what she has to say.

Also: Before merging, could you squash your changes into one or two (or whatever seems reasonable to you) commits?

}

mutating public func set${child.name}(_ ${child.swift_name}: ${ret_type}) throws {
if let tokenSyntax = with${child.name}(${child.swift_name}) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think technically this variable shouldn’t be called tokenSyntax because the child doesn’t need to be a token (although it is in both of our cases). I would prefer something like childSyntax or something along those lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

tree = try! SyntaxParser.parse(source: source)
token = tree.firstToken!
do {
try integerExpr.setDigits(token)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there’s XCTAssertThrowsError to test that an error is thrown. This test would also pass if setDigits doesn’t throw.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

tree = try! SyntaxParser.parse(source: source)
token = tree.firstToken!
do {
try floatExpr.setFloatingDigits(token)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here. Use XCTAssertThrowsError.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

DougGregor and others added 3 commits September 14, 2020 14:26
remove IntegerType helper

move test functions

use hex literals in unit tests

uphold the invariant

remove extraneous initializer

make init(_ data: SyntaxData) failable when respecting invariant

fix isValidLiteral's use

rename isValidLiteral to isValid and make it more type-specific

remove makeBlankFloatLiteralExpr() and makeBlankIntegerLiteralExpr()

make return types of makeFloatLiteralExpr() and makeIntegerLiteralExpr() optional

add comments

use fatalError() rather than return nil

avoid force unwrap in withDigits and withFloatingDigits

add comment

makeprefixLengthAndRadix() private

make numeric-literal setters throw

update Changelog

garden readme

rename tokenSyntax to childSyntax

use XCTAssertThrowsError
@vermont42
Copy link
Contributor Author

vermont42 commented Sep 14, 2020

@ahoppen I squashed all but my first commit, which I reworded to Implements SR-11580. I'm not sure why Doug's commit appears in my commit history because I left that as pick.
Screen Shot 2020-09-14 at 3 04 00 PM

There were some conflicts with the the main repo's master branch, so I resolved those.

Please let me know if further git actions should be taken. 😅

Here is the PR to the main Swift repo: swiftlang/swift#33948

@ahoppen
Copy link
Member

ahoppen commented Sep 16, 2020

@ahoppen I squashed all but my first commit, which I reworded to Implements SR-11580. I'm not sure why Doug's commit appears in my commit history because I left that as pick.

I think Doug’s commit should disappear if you rebase your PR on the current master branch. That way you should also be able to remove the merge commit.

DougGregor and others added 3 commits September 16, 2020 16:36
remove IntegerType helper

move test functions

use hex literals in unit tests

uphold the invariant

remove extraneous initializer

make init(_ data: SyntaxData) failable when respecting invariant

fix isValidLiteral's use

rename isValidLiteral to isValid and make it more type-specific

remove makeBlankFloatLiteralExpr() and makeBlankIntegerLiteralExpr()

make return types of makeFloatLiteralExpr() and makeIntegerLiteralExpr() optional

add comments

use fatalError() rather than return nil

avoid force unwrap in withDigits and withFloatingDigits

add comment

makeprefixLengthAndRadix() private

make numeric-literal setters throw

update Changelog

garden readme

rename tokenSyntax to childSyntax

use XCTAssertThrowsError
@vermont42
Copy link
Contributor Author

At @ahoppen's request, I squashed my changes. Due to an off-by-one error in my interactive rebase, the extraneous commit shown below appeared in the PR's commit history. After a couple of hours tinkering with git commands, I realized that the cleanup task is beyond my ability to implement, as demonstrated by the current, polluted state of this PR's commit history. I am therefore closing this PR and raising a new one with a squashed, unpolluted commit history. I apologize for the obscuring of feedback on the about-to-be-closed PR that this action will cause. The new PR will reference this PR.

Screen Shot 2020-09-17 at 9 12 42 PM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants